Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMoves DiscoveredTaskDefinitions into src/types.rs with BTreeMap storage and insert/get APIs; updates discovery modules and support functions to use insert; rewrites many tests to use get_first/get_all; refactors list verbose reporting; adds tests and regenerates cargo_crap baseline and project plan checklist. ChangesDiscoveredTaskDefinitions Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/task_discovery/github_actions.rs (1)
106-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord actual workflow files and statuses here.
Now that verbose reporting reads from
definitions.files, inserting one syntheticParsedentry for.github/workflowsonly whenall_tasksis non-empty loses real source information. Repos that only haveworkflow.yml/custom/workflows/*.yml, or repos where every workflow fails to parse, end up with inaccurate or missing GitHub Actions definition reporting.Possible fix
fn discover_github_actions_tasks( dir: &Path, discovered: &mut DiscoveredTasks, ) -> anyhow::Result<()> { let mut workflow_files = Vec::new(); + let workflows_parent = dir.join(".github").join("workflows"); - let workflows_dir = dir.join(".github").join("workflows"); + let workflows_dir = workflows_parent.clone(); if workflows_dir.exists() && workflows_dir.is_dir() { match fs::read_dir(&workflows_dir) { Ok(entries) => { @@ - if workflow_files.is_empty() { - return Ok(()); - } + if workflow_files.is_empty() { + discovered.definitions.insert(TaskDefinitionFile { + path: workflows_parent.clone(), + definition_type: TaskDefinitionType::GitHubActions, + status: TaskFileStatus::NotFound, + }); + return Ok(()); + } let mut all_tasks = Vec::new(); let mut errors = Vec::new(); - let workflows_parent = dir.join(".github").join("workflows"); for file_path in workflow_files { match parse_github_actions(&file_path) { Ok(mut tasks) => { + discovered.definitions.insert(TaskDefinitionFile { + path: file_path.clone(), + definition_type: TaskDefinitionType::GitHubActions, + status: TaskFileStatus::Parsed, + }); let source = ComposedDefinitionSource::composed(workflows_parent.clone(), file_path); for task in &mut tasks { @@ - Err(error) => errors.push(format!( - "Failed to parse workflow file {:?}: {}", - file_path, error - )), + Err(error) => { + discovered.definitions.insert(TaskDefinitionFile { + path: file_path.clone(), + definition_type: TaskDefinitionType::GitHubActions, + status: TaskFileStatus::ParseError(error.to_string()), + }); + errors.push(format!( + "Failed to parse workflow file {:?}: {}", + file_path, error + )); + } } } @@ - if !all_tasks.is_empty() { - discovered.definitions.insert(TaskDefinitionFile { - path: workflows_parent, - definition_type: TaskDefinitionType::GitHubActions, - status: TaskFileStatus::Parsed, - }); + if !all_tasks.is_empty() { discovered.tasks.extend(all_tasks); }🤖 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/github_actions.rs` around lines 106 - 112, Currently the code inserts a single synthetic TaskDefinitionFile for workflows_parent when all_tasks is non-empty, which loses per-file provenance and status; change this to record each actual workflow file and its parse status instead: for each workflow path found (instead of only using workflows_parent) create and insert a TaskDefinitionFile with the real path, TaskDefinitionType::GitHubActions, and the correct TaskFileStatus (Parsed for successfully parsed files, Failed or whatever failure status you track for parse errors), and only extend discovered.tasks with the parsed tasks (all_tasks) — update the logic around all_tasks, discovered.definitions.insert, TaskDefinitionFile, workflows_parent, and TaskFileStatus::Parsed to iterate per-file and preserve real file-level statuses.
🤖 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 `@cargo_crap_baseline.json`:
- Around line 8-11: The execute function in ./src/commands/list.rs has excessive
cyclomatic complexity and zero test coverage; refactor it by extracting smaller,
well-named helper functions (e.g., parse_list_args, collect_packages,
filter_packages, compute_package_metrics, and format_list_output) to move
independent match arms, loops, and conditional logic out of execute, reduce
branching inside execute to high-level orchestration, and then add unit tests
for each helper plus focused integration tests for execute (using mocks/stubs
for IO and external dependencies) to cover all branches and error paths so the
overall cyclomatic and CRAP metrics fall below the threshold.
In `@src/task_discovery.rs`:
- Line 18: Run rustfmt (e.g., cargo fmt) on the file to fix the formatting
issues flagged by CI; specifically ensure the use statement importing Task,
TaskDefinitionFile, and DiscoveredTaskDefinitions and any changed assertions are
formatted according to rustfmt rules so cargo fmt --all will pass (this will
resolve the failing cargo fmt --all -- --check CI step).
In `@src/types.rs`:
- Around line 259-260: The file src/types.rs is not formatted and is failing
cargo fmt --all -- --check; run rustfmt (or cargo fmt --all) to reformat
src/types.rs (fix spacing, alignment for struct/enum/impl/function declarations
within types.rs), stage the reformatted file and update the commit so CI passes;
ensure any local rustfmt configuration is honored and that cargo fmt --all --
--check succeeds before pushing.
- Around line 133-147: Make the internal multi-valued storage private and
provide explicit accessors: change the pub field files in
DiscoveredTaskDefinitions to private (remove pub), keep insert as-is, replace
the current get method with a get_first that returns Option<&TaskDefinitionFile>
if you still need the first item, and add two new public accessors such as pub
fn get_all(&self, definition_type: &TaskDefinitionType) ->
Option<&[TaskDefinitionFile]> (or Option<&Vec<TaskDefinitionFile>>) to return
all definitions for a given type and pub fn iter(&self) -> impl Iterator<Item =
(&TaskDefinitionType, &[TaskDefinitionFile])> (or a suitable concrete iterator)
to iterate entries; update callers of DiscoveredTaskDefinitions::get to use
get_first/get_all/iter as appropriate. Ensure function/type names referenced:
DiscoveredTaskDefinitions, files, insert, get (rename), get_all, iter.
---
Outside diff comments:
In `@src/task_discovery/github_actions.rs`:
- Around line 106-112: Currently the code inserts a single synthetic
TaskDefinitionFile for workflows_parent when all_tasks is non-empty, which loses
per-file provenance and status; change this to record each actual workflow file
and its parse status instead: for each workflow path found (instead of only
using workflows_parent) create and insert a TaskDefinitionFile with the real
path, TaskDefinitionType::GitHubActions, and the correct TaskFileStatus (Parsed
for successfully parsed files, Failed or whatever failure status you track for
parse errors), and only extend discovered.tasks with the parsed tasks
(all_tasks) — update the logic around all_tasks, discovered.definitions.insert,
TaskDefinitionFile, workflows_parent, and TaskFileStatus::Parsed to iterate
per-file and preserve real file-level statuses.
🪄 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: 8112844a-4e2b-40bb-9193-dee3ecec06ad
📒 Files selected for processing (12)
cargo_crap_baseline.jsondev_docs/project_plan.mdsrc/commands/list.rssrc/task_discovery.rssrc/task_discovery/docker_compose.rssrc/task_discovery/github_actions.rssrc/task_discovery/gradle.rssrc/task_discovery/justfile.rssrc/task_discovery/npm.rssrc/task_discovery/python.rssrc/task_discovery/support.rssrc/types.rs
Comment on lines
+8
to
+11
| "line": 22, | ||
| "cyclomatic": 43.0, | ||
| "coverage": 0.0, | ||
| "crap": 380.0 | ||
| "crap": 1892.0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
High CRAP score remains after refactoring.
The execute function in ./src/commands/list.rs still has a CRAP score of 1892.0 with cyclomatic complexity of 43. While the PR refactored verbose reporting to reduce duplication, the core execute function remains highly complex with zero test coverage.
Consider breaking down the execute function into smaller helper functions and adding unit tests to reduce this risk metric below the typical threshold of 30.
🤖 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 `@cargo_crap_baseline.json` around lines 8 - 11, The execute function in
./src/commands/list.rs has excessive cyclomatic complexity and zero test
coverage; refactor it by extracting smaller, well-named helper functions (e.g.,
parse_list_args, collect_packages, filter_packages, compute_package_metrics, and
format_list_output) to move independent match arms, loops, and conditional logic
out of execute, reduce branching inside execute to high-level orchestration, and
then add unit tests for each helper plus focused integration tests for execute
(using mocks/stubs for IO and external dependencies) to cover all branches and
error paths so the overall cyclomatic and CRAP metrics fall below the threshold.
Comment on lines
259
to
260
|
|
||
|
|
There was a problem hiding this comment.
Run rustfmt on this file before merge.
cargo fmt --all -- --check is already failing here, so this file is currently blocking CI.
🤖 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/types.rs` around lines 259 - 260, The file src/types.rs is not formatted
and is failing cargo fmt --all -- --check; run rustfmt (or cargo fmt --all) to
reformat src/types.rs (fix spacing, alignment for struct/enum/impl/function
declarations within types.rs), stage the reformatted file and update the commit
so CI passes; ensure any local rustfmt configuration is honored and that cargo
fmt --all -- --check succeeds before pushing.
…it accessors for iteration and retrieval
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/commands/list.rs`:
- Around line 36-38: The current verbose output uses file.path.file_name()
(assigned to file_name) which drops parent directories and causes
duplicate-looking entries; change the logic that computes file_name so it prints
a relative path instead (e.g., compute a path relative to the workspace/current
working directory via Path::strip_prefix or similar and fall back to the full
path if strip_prefix fails) and use that relative-path string wherever file_name
is used in the verbose definition output (references: discovered.definitions,
file.path, and the file_name binding).
- Around line 951-967: The test currently saves original_dir and only resets cwd
after execute/assert succeed, which leaks the temp_dir on panic; replace that
manual restore with a RAII guard that captures original_dir when created (e.g.,
CwdGuard or scopeguard) and restores it in Drop so the working directory is
always reset even if execute(...) or assert! panics; locate the setup around
original_dir, temp_dir and the execute(true, "never") call in this test and
instantiate the guard immediately after changing to temp_dir.path() so Drop will
call std::env::set_current_dir(original_dir) unconditionally.
In `@src/commands/mcp.rs`:
- Around line 491-530: Test cleanup (restoring CWD and HOME) can be skipped if
the test panics or an assertion fails; wrap the original_dir and old_home
capture in an RAII guard so restoration always runs. Implement a small guard
struct (e.g., TestEnvGuard) that stores old_dir: Option<PathBuf> and old_home:
Option<String> and implements Drop to restore with std::env::set_current_dir /
set_var / remove_var, then create an instance at the start of
test_execute_init_cursor before changing the current dir/HOME and remove the
manual restore block at the end; keep assertions (assert!(result.is_ok()) and
expected_path.exists()) after calling execute so the guard still runs on panic.
🪄 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: a0ced326-ed51-47e0-babb-71107dd36721
📒 Files selected for processing (7)
cargo_crap_baseline.jsonsrc/commands/list.rssrc/commands/mcp.rssrc/main.rssrc/prompt.rssrc/task_discovery.rssrc/types.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 `@cargo_crap_baseline.json`:
- Around line 1205-1212: DiscoveredTaskDefinitions::get_all currently has no
direct test coverage; add a focused unit test that constructs a
DiscoveredTaskDefinitions instance with multiple task definitions (including at
least two definitions sharing the same task key/name and some unique ones),
calls DiscoveredTaskDefinitions::get_all, and asserts the returned collection
shape and contents (count, grouping of multiple definitions under the same key,
and that each definition's fields match the inputs). Place the test in the
appropriate test module (e.g., a #[cfg(test)] mod tests in the same file or a
unit test under tests/) and include cases for empty and multi-definition
retrieval to lock the grouping contract for get_all.
🪄 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: eea4c2ca-c0f3-43e3-9721-7279fe7b4172
📒 Files selected for processing (3)
cargo_crap_baseline.jsonsrc/commands/list.rssrc/commands/mcp.rs
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
Refactor
Tests
Chores