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:
📝 WalkthroughWalkthroughRefactors task discovery into a registry-based TaskDiscovery trait, adds per-runner discovery modules, centralizes support and disambiguation logic, and updates orchestration to run registered discoverers and apply name disambiguation. ChangesTask Discovery Registry & Modularization
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/list command
participant Registry as registry::registered_discoveries()
participant Discoverer as TaskDiscovery implementations
participant Aggregator as DiscoveredTasks
CLI->>Registry: request discoverers
CLI->>Discoverer: invoke discover(dir, &mut Aggregator)
Discoverer->>Aggregator: append tasks/errors/definitions
CLI->>Aggregator: process_task_disambiguation(&mut)
Aggregator-->>CLI: return DiscoveredTasks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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_discovery/cmake.rs`:
- Around line 26-29: The error arm in the match currently constructs and returns
Err("Error parsing CMakeLists.txt") even though the caller discards the Result;
either propagate the real error or remove the unused Err. Update the Err(error)
branch in src/task_discovery/cmake.rs (the branch that calls
handle_discovery_error(error, cmake_path, TaskDefinitionType::CMake,
discovered)) to one of two fixes: (A) propagate the original error by returning
a Result::Err (e.g., return Err(error.into()) or map it to the function's error
type) so callers can handle it, or (B) simplify the path by removing the
redundant Err(...) and just call handle_discovery_error(...) and return/continue
(making the function behave as a void/() error-reporting path). Choose the
option consistent with the surrounding API and apply to the Err(error) arm only.
- Around line 15-31: discover_cmake_tasks currently returns early when
CMakeLists.txt is missing instead of recording a NotFound status; update
discover_cmake_tasks to push a DiscoveredTask/TaskFileStatus::NotFound entry
into the provided discovered (DiscoveredTasks) with TaskDefinitionType::CMake
(using cmake_path) before returning Ok(()), following the same pattern used by
other discovery modules; ensure the function still returns Ok(()) after
recording the NotFound status so callers get a consistent discovered.definitions
entry for missing files.
In `@src/task_discovery/disambiguation.rs`:
- Line 61: Extract the literal 3 used to compute prefix_length into a named
constant (e.g., MIN_PREFIX_LEN) to avoid the magic number; update the
computation let prefix_length = std::cmp::min(3, short_name.len()) to use that
constant, define MIN_PREFIX_LEN near the top of the module or impl where
disambiguation logic lives, and ensure any tests or other references use the
constant to keep behavior consistent.
- Around line 49-82: generate_runner_prefix currently slices short_name by byte
indices which can panic on multi-byte UTF-8 characters; change all byte-based
slicing to character-based operations: compute lengths with
short_name.chars().count(), build prefixes with
short_name.chars().take(n).collect::<String>() (use
chars().next().unwrap().to_string() for single_char), and iterate length over
character counts rather than byte lengths so prefixes and the loop bounds use
character counts; keep the existing fallback that appends numeric suffixes to
the original short_name.
- Around line 117-133: format_ambiguous_task_error currently omits entries where
Task.disambiguated_name is None; update the loop in format_ambiguous_task_error
to either assert all matching_tasks have disambiguated_name at the start (e.g.,
panic or debug_assert if you prefer) or, preferably, fall back inside the loop
by formatting a fallback line using the task's base name (e.g., task.name or
task.base_name()), task.runner.short_name(), and
task.definition_path().display() when disambiguated_name is None so no matching
task is silently omitted from the error message.
In `@src/task_discovery/github_actions.rs`:
- Around line 76-112: The code currently sets
discovered.definitions.github_actions unconditionally only when any tasks parsed
and always uses workflows_parent (dir/.github/workflows), which is wrong for
zero files and for workflows located elsewhere; change the logic so that if
workflow_files is empty you leave discovered.definitions.github_actions as None,
and when setting discovered.definitions.github_actions after successful parses
compute and store the actual path(s) of the source: for a single parsed file use
the file_path.parent(), for multiple files compute their common parent (or fall
back to the nearest shared ancestor), and set TaskDefinitionFile.path to that
computed directory and TaskDefinitionType::GitHubActions with status
TaskFileStatus::Parsed; keep the existing error collection and task augmentation
(parse_github_actions, ComposedDefinitionSource::composed,
ComposedDefinitionSource.apply_to_task, check_shadowing) unchanged except for
using the actual file parent(s) when creating the TaskDefinitionFile.
In `@src/task_discovery/make.rs`:
- Around line 109-126: The current handling in the
collect_makefile_tasks_recursive caller sets first_error when any included file
fails, which causes the entire root Makefile to be marked ParseError and
discards successfully parsed tasks; change this to either treat include parse
errors as non-fatal warnings by removing the assignment to first_error in the
error branch (keep pushing into include_errors) or make it conditional (set
first_error only if collected_tasks is empty for this root). Update the block
around collect_makefile_tasks_recursive where include_errors and first_error are
updated (referencing symbols collect_makefile_tasks_recursive, include_errors,
first_error, collected_tasks) so that include parse failures do not override
successful task discovery unless no tasks were parsed, or alternatively add a
config flag to control fatal-vs-warning behavior if you prefer configurability.
- Around line 11-15: The discover implementation currently ignores a Result from
discover_makefile_tasks; since discover_makefile_tasks only ever returns Ok(())
and writes errors directly into discovered.errors, change
discover_makefile_tasks' signature from -> Result<(), String> to -> () and
remove any Result construction inside it, then update the TaskDiscovery impl for
MakefileDiscovery::discover to call discover_makefile_tasks(dir, discovered)
without discarding a Result; ensure any places referencing its Result type are
updated accordingly (function name: discover_makefile_tasks, trait impl: impl
TaskDiscovery for MakefileDiscovery, method: discover, and the
DiscoveredTasks.errors usage).
In `@src/task_discovery/registry.rs`:
- Around line 42-49: The helper function run_discovery is dead code; either
remove it or add a doc comment explaining its purpose/intent (e.g., reserved for
tests or external callers) so its presence is justified. If you remove it,
delete the pub(crate) fn run_discovery(...) definition and any
#[allow(dead_code)] attribute; if you keep it, replace the attribute with a
concise /// doc comment describing why run_discovery exists and how it relates
to TaskDiscovery and DiscoveredTasks (for example: used by tests or FFI),
ensuring the comment references TaskDiscovery::discover and DiscoveredTasks so
future maintainers understand its purpose.
In `@src/task_discovery/taskfile.rs`:
- Around line 98-104: The current call to
traversal.traversal_state.mark_visited(current_source.definition_path())
implements a global-per-path memo that prevents legitimate re-inclusion under
different namespaces/flatten modes; replace this with recursion-stack cycle
detection: change the visitation protocol used in the traversal (the code around
traversal.traversal_state and VisitState) so that entering a node marks it as
"visiting" on the current recursion stack and exiting unmarks it (push on enter,
pop on exit), and only treat a path as a cycle/error when it is already marked
"visiting" (not when it was previously fully visited). Update the logic that
currently branches on VisitState::AlreadyVisited/VisitState::New to instead
detect VisitState::Visiting -> handle cycle, otherwise push mark Visiting at
start of recursion and remove/unmark (or mark Visited) when returning so later
independent branches can revisit the same definition_path().
In `@src/task_discovery/turbo.rs`:
- Around line 313-320: The function looks_like_turbo_config_path erroneously
classifies scoped package names like "@scope/pkg" as filesystem paths; update
looks_like_turbo_config_path to exclude entries that start with '@' from the
"contains('/' or '\\')" path check so scoped names are treated as package names
(e.g., add a condition so the contains('/') and contains('\\') checks only apply
when !extend_entry.starts_with('@')). Keep the existing absolute/path-start/dot
checks intact and only adjust the conditions that treat slashes as path
indicators so that scoped packages proceed to package index lookup.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55d1a392-f070-4578-a020-5a66b46c7053
📒 Files selected for processing (18)
dev_docs/project_plan.mdsrc/task_discovery.rssrc/task_discovery/cmake.rssrc/task_discovery/disambiguation.rssrc/task_discovery/docker_compose.rssrc/task_discovery/github_actions.rssrc/task_discovery/gradle.rssrc/task_discovery/justfile.rssrc/task_discovery/make.rssrc/task_discovery/maven.rssrc/task_discovery/npm.rssrc/task_discovery/python.rssrc/task_discovery/registry.rssrc/task_discovery/shell_scripts.rssrc/task_discovery/support.rssrc/task_discovery/taskfile.rssrc/task_discovery/travis_ci.rssrc/task_discovery/turbo.rs
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a46dd54-0697-4361-ae08-47a3e9c59210
📒 Files selected for processing (7)
src/commands/list.rssrc/task_discovery.rssrc/task_discovery/cmake.rssrc/task_discovery/disambiguation.rssrc/task_discovery/make.rssrc/task_discovery/registry.rssrc/task_discovery/turbo.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/task_discovery/disambiguation.rs (1)
21-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake disambiguated names globally unique, not just group-local.
used_prefixesonly dedupes within one base-name group, and the shadowed-task pass starts from an empty set every time. That still allowstestto becometest-mwhen another task is already literally namedtest-m, which leaves the original ambiguous task with no unique selector. Reserve every existingtask.name/disambiguated_namefirst, then choose the first suffix whose full"{name}-{prefix}"is globally unused.🤖 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_discovery/disambiguation.rs` around lines 21 - 47, The disambiguation currently only ensures uniqueness within each base-name group by using a per-group used_prefixes and restarting for the shadowed-task pass; instead reserve all existing task.name and task.disambiguated_name values globally first (iterate discovered.tasks and insert each name/disambiguated_name into a global HashSet of reserved_full_names), then when generating a runner_prefix via generate_runner_prefix (used previously as used_prefixes), test the full candidate string format!("{}-{}", task.name, prefix) against the global reserved set and keep trying prefixes until you find one not present, insert that full name into the global set, and assign it to task.disambiguated_name; apply this for both the group collision loop (tasks_by_name) and the shadowed-by loop so no produced "{name}-{prefix}" ever collides with any existing name.
🤖 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_discovery/make.rs`:
- Around line 18-30: The code currently only checks dir.join("Makefile") and
returns NotFound; update the discovery in src/task_discovery/make.rs to search
the GNU make filename order ["GNUmakefile", "makefile", "Makefile"] and use the
first existing path found; when a file is found, call set_definition with a
TaskDefinitionFile whose path is that discovered PathBuf, definition_type
TaskDefinitionType::Makefile and appropriate status, otherwise keep the NotFound
behavior. Locate the existing makefile_path logic and replace the single check
with an ordered loop/iterator over the three names so the discovered path is
recorded correctly.
- Around line 86-119: collect_makefile_tasks_recursive currently parses all
tasks then processes includes, which enforces first-definition-wins; instead
process file items in original order so later definitions override earlier ones.
Change the routine that calls parse_makefile::parse and
parse_makefile::extract_include_directives to iterate the makefile's top-level
items in source order (or reconstruct order) and for each item: if it's a task,
call current_source.apply_to_task(task) and insert/replace into collected_tasks
(use a name->index map rather than a HashSet seen_task_names so you can
overwrite previous entries); if it's an include, resolve via
current_source.resolve_child and recurse with ComposedDefinitionSource::composed
as you already do. Keep using collect_makefile_tasks_recursive, include_errors,
traversal_state, and preserve existing error wrapping when recursing.
---
Duplicate comments:
In `@src/task_discovery/disambiguation.rs`:
- Around line 21-47: The disambiguation currently only ensures uniqueness within
each base-name group by using a per-group used_prefixes and restarting for the
shadowed-task pass; instead reserve all existing task.name and
task.disambiguated_name values globally first (iterate discovered.tasks and
insert each name/disambiguated_name into a global HashSet of
reserved_full_names), then when generating a runner_prefix via
generate_runner_prefix (used previously as used_prefixes), test the full
candidate string format!("{}-{}", task.name, prefix) against the global reserved
set and keep trying prefixes until you find one not present, insert that full
name into the global set, and assign it to task.disambiguated_name; apply this
for both the group collision loop (tasks_by_name) and the shadowed-by loop so no
produced "{name}-{prefix}" ever collides with any existing name.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59b4170b-b1ea-4c84-a884-d31ecc1e2d33
📒 Files selected for processing (3)
src/task_discovery.rssrc/task_discovery/disambiguation.rssrc/task_discovery/make.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/task_discovery/make.rs (1)
90-123:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve
includeordering when resolving duplicate targets.This still parses every target in
current_sourcebefore following anyinclude, thenseen_task_nameskeeps the first definition forever. That makes duplicate resolution effectively first-definition-wins, but included makefiles are read in place: if the root definestest, then laterincludes another file that also definestest, the included definition should win. Here it never can, sodefinition_path()and later disambiguation can point at the wrong task. Please collect top-level items in source order and replace earlier entries when a later definition of the same target is encountered. Adding a regression test for “target before include, same target redefined in include” would lock this down.🤖 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_discovery/make.rs` around lines 90 - 123, The current implementation parses all tasks first then processes includes, causing first-definition-wins; instead process top-level entries in source order (tasks and include directives interleaved) so later definitions replace earlier ones. Change collect_makefile_tasks_recursive to obtain an ordered sequence (or merge parse_makefile::parse results with parse_makefile::extract_include_directives by their positions) and iterate that sequence: when you encounter a task, use a map seen_task_index: HashMap<String, usize> to check if the task name already exists in collected_tasks and replace the existing entry at that index (and update any definition_path via current_source.apply_to_task), otherwise push and record its index; when you encounter an include, recurse into collect_makefile_tasks_recursive as before so included tasks can override prior entries. Also add a regression test for “target before include, same target redefined in include” to ensure the included definition wins.
🤖 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_discovery/make.rs`:
- Around line 20-23: The discovery currently picks the first path where
path.exists(), which can match directories named like makefiles; change the
filter to require a real file by replacing the exists() check with is_file() so
MAKEFILE_NAMES.iter().map(|name| dir.join(name)).find(...) only returns actual
makefiles (the let Some(makefile_path) binding).
---
Duplicate comments:
In `@src/task_discovery/make.rs`:
- Around line 90-123: The current implementation parses all tasks first then
processes includes, causing first-definition-wins; instead process top-level
entries in source order (tasks and include directives interleaved) so later
definitions replace earlier ones. Change collect_makefile_tasks_recursive to
obtain an ordered sequence (or merge parse_makefile::parse results with
parse_makefile::extract_include_directives by their positions) and iterate that
sequence: when you encounter a task, use a map seen_task_index: HashMap<String,
usize> to check if the task name already exists in collected_tasks and replace
the existing entry at that index (and update any definition_path via
current_source.apply_to_task), otherwise push and record its index; when you
encounter an include, recurse into collect_makefile_tasks_recursive as before so
included tasks can override prior entries. Also add a regression test for
“target before include, same target redefined in include” to ensure the included
definition wins.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f47e3ad-5980-4e77-8a57-3fb45050db5b
📒 Files selected for processing (2)
src/task_discovery.rssrc/task_discovery/make.rs
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/task_discovery/make.rs`:
- Around line 20-30: The code currently treats any failure to read a directory
as TaskFileStatus::NotFound; update discover_makefile_tasks (and the similar
block around the 74-89 range) to explicitly handle fs::read_dir errors: call
fs::read_dir(dir) and match the Result—if Ok, proceed as before; if Err(e), call
set_definition with TaskFileStatus::NotReadable and include the error (or its
kind) in any logs/metadata so permission/I/O failures aren't collapsed into
NotFound; keep using find_makefile_path and set_definition as the insertion
points to set the correct TaskFileStatus.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5860acbd-6410-4358-914d-8ed403864f48
📒 Files selected for processing (3)
src/commands/list.rssrc/task_discovery.rssrc/task_discovery/make.rs
This was referenced May 18, 2026
Merged
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
1 participant
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 by CodeRabbit
Documentation
Refactor
New Features
Bug Fixes