Conversation
📝 WalkthroughWalkthroughRefactors nested conditionals into compound guards across many parsers and core modules, derives Changes
Sequence Diagram(s)(Skipped — changes are refactor/CI/tooling-focused and do not introduce a new multi-component sequential feature.) 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/parsers/parse_package_json.rs (1)
23-37:⚠️ Potential issue | 🔴 CriticalSame rustfmt over-indentation; CI
cargo fmt --checkfails.🛠️ Proposed fix
- if let Some(scripts) = json.get("scripts") - && let Some(scripts_obj) = scripts.as_object() { - for (name, cmd) in scripts_obj { - tasks.push(Task { - name: name.clone(), - file_path: path.clone(), - definition_type: TaskDefinitionType::PackageJson, - runner: runner.clone(), - source_name: name.clone(), - description: cmd.as_str().map(|s| s.to_string()), - shadowed_by: None, - disambiguated_name: None, - }); - } - } + if let Some(scripts) = json.get("scripts") + && let Some(scripts_obj) = scripts.as_object() + { + for (name, cmd) in scripts_obj { + tasks.push(Task { + name: name.clone(), + file_path: path.clone(), + definition_type: TaskDefinitionType::PackageJson, + runner: runner.clone(), + source_name: name.clone(), + description: cmd.as_str().map(|s| s.to_string()), + shadowed_by: None, + disambiguated_name: None, + }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/parse_package_json.rs` around lines 23 - 37, The code block in parse_package_json.rs is over-indented causing cargo fmt --check to fail; reformat the if-let chain and its body to match rustfmt style (align the `if let Some(scripts) = json.get("scripts") && let Some(scripts_obj) = scripts.as_object()` and the following block braces), and fix indentation of the `for (name, cmd) in scripts_obj { ... }` and the `tasks.push(Task { ... })` initializer (which includes fields like name, file_path, definition_type, runner, source_name, description, shadowed_by, disambiguated_name). After adjusting indentation, run cargo fmt to ensure the file is properly formatted.src/parsers/parse_pom_xml.rs (1)
87-141:⚠️ Potential issue | 🔴 CriticalSame rustfmt over-indentation across the entire collapsed block; CI
cargo fmt --checkfails.The whole inner body (L89–141) is still indented to the old double-
if letdepth and must shift one level left after collapsing. Runningcargo fmt --allreformats it; the structure should become:if let Some(build_node) = root.children().find(|n| n.has_tag_name("build")) && let Some(plugins_node) = build_node.children().find(|n| n.has_tag_name("plugins")) { for plugin in plugins_node.children().filter(|n| n.has_tag_name("plugin")) { // ... body indented one level shallower than current } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/parse_pom_xml.rs` around lines 87 - 141, The block using the combined `if let` (matching `build_node` and `plugins_node` found from `root.children()`) is over-indented after collapsing the condition; fix by placing the opening brace on its own line after the combined condition for `if let Some(build_node) = root.children().find(|n| n.has_tag_name("build")) && let Some(plugins_node) = build_node.children().find(|n| n.has_tag_name("plugins"))` and shift the entire inner body (the `for plugin in plugins_node.children()...` loop and its contents that produce `Task` entries) one level left so it aligns as shown in the review, then run `cargo fmt --all` or `cargo fmt --check` to ensure rustfmt accepts the change.src/parsers/parse_pyproject_toml.rs (3)
16-34:⚠️ Potential issue | 🟠 MajorFix the rustfmt failure flagged by CI.
The pipeline reports
cargo fmt --all -- --checkfailures for both this UV-scripts block (lines 15–43) and the poe-tasks block at line 71. The let-chain bodies are using cascading 4-space indentation (if let ... \n && let ... \n && let ... { ... }), which rustfmt rewrites to single-step indentation with the opening brace on its own line. Please runcargo fmt --alland amend.Expected shape (apply analogously to the poe block at lines 71–107):
🎨 Proposed format
- if let Some(project) = toml.get("project") - && let Some(scripts) = project.get("scripts") - && let Some(scripts_table) = scripts.as_table() - && (cfg!(test) || check_path_executable("uv").is_some()) { - for (name, cmd) in scripts_table { - let description = cmd.as_str().map(|s| format!("python script: {}", s)); - - tasks.push(Task { - name: name.clone(), - file_path: path.to_path_buf(), - definition_type: TaskDefinitionType::PyprojectToml, - runner: TaskRunner::PythonUv, - source_name: name.clone(), - description, - shadowed_by: None, - disambiguated_name: None, - }); - } - } + if let Some(project) = toml.get("project") + && let Some(scripts) = project.get("scripts") + && let Some(scripts_table) = scripts.as_table() + && (cfg!(test) || check_path_executable("uv").is_some()) + { + for (name, cmd) in scripts_table { + let description = cmd.as_str().map(|s| format!("python script: {}", s)); + + tasks.push(Task { + name: name.clone(), + file_path: path.to_path_buf(), + definition_type: TaskDefinitionType::PyprojectToml, + runner: TaskRunner::PythonUv, + source_name: name.clone(), + description, + shadowed_by: None, + disambiguated_name: None, + }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/parse_pyproject_toml.rs` around lines 16 - 34, Run rustfmt to fix the formatting: reformat the let-chain if conditions so the opening brace is on its own line and indentation matches rustfmt style (e.g. transform the cascading indents used in the if let chain that checks toml.get("project") -> project.get("scripts") -> scripts.as_table() -> (cfg!(test) || check_path_executable("uv").is_some())). Update the same formatting for the poe-tasks block mentioned later; then run cargo fmt --all and amend the commit. Locate the block that constructs Task with TaskRunner::PythonUv in parse_pyproject_toml.rs and the corresponding poe-tasks block and let rustfmt rewrite the brace and indentation to the canonical style.
45-67:⚠️ Potential issue | 🔴 CriticalOperator-precedence bug: missing parentheses change the gating semantics for Poetry scripts.
On line 51 the condition is:
if cfg!(test) || check_path_executable("poetry").is_some() && poetry_lock_exists {Because
&&binds tighter than||, this parses as:cfg!(test) || (check_path_executable("poetry").is_some() && poetry_lock_exists)Compare this with the parallel UV (line 19) and Poe (line 79) branches, which correctly keep the
(cfg!(test) || check_path_executable(...).is_some())group parenthesized. Judging from the tests (e.g. the comment "Create a poetry.lock file so that the poetry scripts branch is triggered" intest_parse_both_uv_and_poetry_scripts) and the symmetry with the other branches, the intended condition was:(cfg!(test) || check_path_executable("poetry").is_some()) && poetry_lock_existsUnder the new code, in test builds the
poetry_lock_existscheck is bypassed entirely (sincecfg!(test)short-circuits the||), which silently weakens the gating that the existing tests are written to assert. In non-test builds the two forms happen to be equivalent, so this won't fail at runtime — but it will mask regressions in tests and is almost certainly not what was intended by a clippy-driven refactor.🔧 Proposed fix
- if let Some(scripts) = poetry.get("scripts") - && let Some(scripts_table) = scripts.as_table() { - let poetry_lock_exists = path - .parent() - .map(|dir| dir.join("poetry.lock").exists()) - .unwrap_or(false); - if cfg!(test) || check_path_executable("poetry").is_some() && poetry_lock_exists { + if let Some(scripts) = poetry.get("scripts") + && let Some(scripts_table) = scripts.as_table() + { + let poetry_lock_exists = path + .parent() + .map(|dir| dir.join("poetry.lock").exists()) + .unwrap_or(false); + if (cfg!(test) || check_path_executable("poetry").is_some()) && poetry_lock_exists {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/parse_pyproject_toml.rs` around lines 45 - 67, The conditional that gates adding Poetry scripts uses cfg!(test), check_path_executable("poetry"), and poetry_lock_exists but lacks parentheses so && binds tighter than ||; update the if condition in parse_pyproject_toml.rs (the block that inspects poetry.get("scripts") and iterates scripts_table) to parenthesize the test or executable check together so that the existence of poetry.lock (poetry_lock_exists) is required in addition to either cfg!(test) or check_path_executable("poetry").is_some(); adjust the condition around cfg!(test) and check_path_executable("poetry") accordingly to match the semantics used for the UV/Poe branches.
71-107: 🧹 Nitpick | 🔵 TrivialReadability: nested
if letinside a let-chain is hard to follow.The poe block combines (a) a let-chain
if let Some(poe) = ... && let Some(poe_section) = ..., (b) an innerif let Some(tasks_table) = if let Some(inner) = poe_section.get("tasks") { inner.as_table() } else { poe_section.as_table() }, and (c) a trailing&& (cfg!(test) || check_path_executable("poe").is_some())— all in one expression. Lifting the table selection into alet(or a small helper) makes the gating obvious and matches the style of the other two branches.♻️ Suggested shape
if let Some(tool) = toml.get("tool") && let Some(poe_section) = tool.get("poe") { let tasks_table = poe_section .get("tasks") .and_then(|inner| inner.as_table()) .or_else(|| poe_section.as_table()); if let Some(tasks_table) = tasks_table && (cfg!(test) || check_path_executable("poe").is_some()) { for (name, task_def) in tasks_table { // ... } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/parse_pyproject_toml.rs` around lines 71 - 107, The nested let-chain and inline conditional that selects tasks_table makes the poe parsing hard to read; refactor by extracting the table selection into a separate let binding (e.g., let tasks_table = poe_section.get("tasks").and_then(|t| t.as_table()).or_else(|| poe_section.as_table());) then use a simple if let Some(tasks_table) = tasks_table && (cfg!(test) || check_path_executable("poe").is_some()) to gate the for loop that pushes Task entries (symbols to locate: poe_section, tasks_table, check_path_executable, Task, TaskRunner::PythonPoe); this keeps the logic identical but much clearer and consistent with the other branches.src/task_discovery.rs (1)
820-851:⚠️ Potential issue | 🟠 MajorSame fmt + MSRV concern as the GitHub Actions block; also consider extracting the closure.
cargo fmtis failing here (pipeline error at line 822) — the body is indented several levels too deep after the wrapped&& let Some(extension) = … && extension == "sh"head. After runningcargo fmt --all, this whole branch may format more cleanly as afilter_map/foroverentries.flatten().filter(...)to avoid the awkward let-chain entirely:♻️ Optional alternative without let-chains
fn discover_shell_script_tasks(dir: &Path, discovered: &mut DiscoveredTasks) { - if let Ok(entries) = fs::read_dir(dir) { - for entry in entries.flatten() { - let path = entry.path(); - if path.is_file() - && let Some(extension) = path.extension() - && extension == "sh" { - // Use file stem (without extension) for task name and disambiguation - let name = path - .file_stem() - .unwrap_or_default() - .to_string_lossy() - .to_string(); - ... - } - } - } + let Ok(entries) = fs::read_dir(dir) else { return }; + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_file() || path.extension().is_none_or(|ext| ext != "sh") { + continue; + } + let name = path.file_stem().unwrap_or_default().to_string_lossy().to_string(); + let source_name = path.file_name().unwrap_or_default().to_string_lossy().to_string(); + discovered.tasks.push(Task { /* … */ }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/task_discovery.rs` around lines 820 - 851, The let-chain condition causes cargo fmt/MSRV issues; replace the nested if-let with an iterator filter_map or filter + for to avoid the chained `&& let` and keep formatting stable: iterate over entries.flatten(), map each entry to its path, then use path.is_file() and path.extension().and_then(|e| e.to_str()) == Some("sh") to select scripts, build the task using the same fields (name from file_stem, source_name from file_name, TaskDefinitionType::ShellScript, TaskRunner::ShellScript, shadowed_by via check_shadowing(&name)) and push into discovered.tasks; optionally extract the task-construction block into a small helper/closure (e.g., a function that returns Option<Task>) to keep the loop concise and readable.
♻️ Duplicate comments (1)
src/mcp/allowlist.rs (1)
38-41:⚠️ Potential issue | 🟠 MajorSame fmt failures + let-chain MSRV concern as
src/allowlist.rs.Pipeline errors at lines 36 and 57. The Deny branch and the triple-chained Task branch mirror the patterns in
src/allowlist.rs; the same simplifications (entry.scope == AllowScope::Deny, and anis_some_andfor theTaskbranch) apply here and would remove the let-chain dependency on Rust 1.88 entirely. After applying, runcargo fmt --all.Also applies to: 58-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/allowlist.rs` around lines 38 - 41, Replace the let-chain patterns in src/mcp/allowlist.rs: change the "if let AllowScope::Deny = entry.scope && self.path_matches(&task.file_path, &entry.path, true)" style to a plain equality check (entry.scope == AllowScope::Deny) combined with the path match, and for the triple-chained Task branch (the block around lines 58-62) replace the nested/multi-condition pattern with an Option::is_some_and(...) check on the Task option to remove the let-chain dependency; after making these changes run cargo fmt --all to fix formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/allowlist.rs`:
- Around line 90-96: The AllowScope::Task match arm uses a chained let-binding
inside an && expression which is hard to read and breaks fmt/MSRV; replace the
chained condition with a flattened nested check: first test
path_matches(&task.file_path, &entry.path, false), then use an explicit if let
Some(ref tasks) = entry.tasks (or if let Some(tasks) = &entry.tasks) and inside
that check tasks.contains(&task.name) and return Ok((true, false)) when all
conditions hold; update the AllowScope::Task arm accordingly referencing
AllowScope::Task, path_matches, entry.tasks, task.file_path, and task.name.
- Around line 71-74: The if-statement uses a let-chain (if let AllowScope::Deny
= entry.scope && path_matches(...)) which fails fmt and requires Rust ≥1.88;
change it to a non-let-chain form using the PartialEq on AllowScope (e.g.,
compare entry.scope == AllowScope::Deny combined with
path_matches(&task.file_path, &entry.path, true)) so the condition is equivalent
but fmt-friendly and MSRV-safe, and apply the same swap in the analogous check
in the MCP allowlist code (the entry.scope / path_matches usage).
In `@src/commands/init.rs`:
- Around line 73-77: The collapsed conditional using "if let Some(parent) =
config_path.parent() && !parent.exists()" has its block over-indented; reduce
the block body (the fs::create_dir_all(...) call and its map_err(...) handling)
to a single indentation level under the if to satisfy rustfmt. Apply the same
correction for similar collapsed if-let && patterns in the functions handling
parse_cmake, parse_package_json, parse_pom_xml, parse_gradle, and the mcp
command so their blocks are not doubly-indented, then run "cargo fmt --all" to
normalize formatting.
In `@src/commands/mcp.rs`:
- Around line 172-176: The block using the combined condition "if let
Some(parent) = config_path.parent() && !parent.exists()" is over-indented — the
body (the fs::create_dir_all(...) call and its map_err closure referencing
editor.name()) should be indented only one block level under the if, not at the
deeper nested-if level; fix by reformatting the block so the lines after the
condition align with standard rust indentation (move fs::create_dir_all(...) and
.map_err(...) left to the same indentation level directly under the if-let
condition) so rustfmt/cargo fmt --check passes.
In `@src/mcp/dto.rs`:
- Around line 88-89: Merge the two adjacent derive attributes into one by adding
Default into the existing #[derive(Debug, Clone, PartialEq, Serialize,
Deserialize, JsonSchema)] list so it becomes a single #[derive(...)] attribute
(remove the separate #[derive(Default)]); update the derive on the struct that
currently has those two lines to restore idiomatic single-derive formatting and
satisfy cargo fmt --check.
In `@src/mcp/server.rs`:
- Around line 320-323: Rustfmt is failing for the let-chain conditional; update
the if-let chain so the opening brace is on its own line and the return is
dedented. Specifically, in the block that checks cache.as_ref(),
entry.cached_at.elapsed() and self.task_cache_ttl (referencing variables cache,
entry, cached_at, task_cache_ttl and the returned entry.discovered.clone()),
rewrite the conditional so the `{` is on a new line and the `return
entry.discovered.clone();` is indented one level inside the block to match
rustfmt expectations.
In `@src/parsers/parse_cmake.rs`:
- Around line 58-61: The if-let chain is mis-indented and fails cargo fmt;
change the block around comment_pattern.captures(target_block) so it conforms to
rustfmt by either putting the entire guard on one line (if let
Some(comment_captures) = comment_pattern.captures(target_block) && let
Some(comment) = comment_captures.get(1) { ... }) or by switching to nested if
lets (if let Some(comment_captures) = comment_pattern.captures(target_block) {
if let Some(comment) = comment_captures.get(1) { description =
comment.as_str().to_string(); } }) while referencing comment_pattern, captures,
target_block, comment_captures, comment and description to locate the code.
In `@src/parsers/parse_gradle.rs`:
- Around line 134-135: Replace the unnecessary allocations for
description_single_quote_pattern and description_double_quote_pattern in
parse_gradle.rs by using &'static str literals (remove the .to_string()) since
they are only passed to format! as &str; update any variable types/uses if
annotated to accept &str so the rest of the code (e.g., where format!(...)
consumes these patterns) continues to compile without allocating a String.
- Around line 138-166: The three nested `if let` chains that extract task
descriptions (the blocks building Regex with task_pattern +
description_single_quote_pattern, task_pattern +
description_double_quote_pattern, and the kotlin_pattern variable) have
over-indented bodies causing cargo fmt failures; collapse each multi-clause `if
let Ok(regex) = ... && let Some(caps) = regex.captures(content) && let
Some(desc) = caps.get(1)` so that the returned `Some(desc.as_str().to_string())`
is indented one level inside the `if` (not triple-nested), ensuring the `if`
body aligns with the canonical formatting, then run `cargo fmt --all` (or `cargo
fmt -- --check` to verify) to apply canonical formatting.
In `@src/parsers/parse_pyproject_toml.rs`:
- Around line 16-19: Add an explicit minimum Rust version to Cargo.toml by
adding rust-version = "1.88" under the [package] section so the repository
documents the MSRV required for let-chain usage (used in parse_pyproject_toml.rs
with the let-chain expression), ensuring CI/builds on older toolchains fail
clearly; update the Cargo.toml [package] metadata accordingly and commit the
change.
In `@src/parsers/parse_travis_ci.rs`:
- Line 37: Replace the redundant allocations where
config_map.get(Value::String("...".to_string())) is used: change those calls to
config_map.get("jobs") (and the other string keys used similarly) to avoid
creating a new String each call; locate uses of config_map.get with
Value::String in parse_travis_ci.rs (e.g., the pattern if let
Some(Value::Mapping(jobs_map)) =
config_map.get(Value::String("jobs".to_string()))) and replace the
Value::String("...".to_string()) argument with a plain &str like "jobs" (repeat
for the other occurrences noted).
---
Outside diff comments:
In `@src/parsers/parse_package_json.rs`:
- Around line 23-37: The code block in parse_package_json.rs is over-indented
causing cargo fmt --check to fail; reformat the if-let chain and its body to
match rustfmt style (align the `if let Some(scripts) = json.get("scripts") &&
let Some(scripts_obj) = scripts.as_object()` and the following block braces),
and fix indentation of the `for (name, cmd) in scripts_obj { ... }` and the
`tasks.push(Task { ... })` initializer (which includes fields like name,
file_path, definition_type, runner, source_name, description, shadowed_by,
disambiguated_name). After adjusting indentation, run cargo fmt to ensure the
file is properly formatted.
In `@src/parsers/parse_pom_xml.rs`:
- Around line 87-141: The block using the combined `if let` (matching
`build_node` and `plugins_node` found from `root.children()`) is over-indented
after collapsing the condition; fix by placing the opening brace on its own line
after the combined condition for `if let Some(build_node) =
root.children().find(|n| n.has_tag_name("build")) && let Some(plugins_node) =
build_node.children().find(|n| n.has_tag_name("plugins"))` and shift the entire
inner body (the `for plugin in plugins_node.children()...` loop and its contents
that produce `Task` entries) one level left so it aligns as shown in the review,
then run `cargo fmt --all` or `cargo fmt --check` to ensure rustfmt accepts the
change.
In `@src/parsers/parse_pyproject_toml.rs`:
- Around line 16-34: Run rustfmt to fix the formatting: reformat the let-chain
if conditions so the opening brace is on its own line and indentation matches
rustfmt style (e.g. transform the cascading indents used in the if let chain
that checks toml.get("project") -> project.get("scripts") -> scripts.as_table()
-> (cfg!(test) || check_path_executable("uv").is_some())). Update the same
formatting for the poe-tasks block mentioned later; then run cargo fmt --all and
amend the commit. Locate the block that constructs Task with
TaskRunner::PythonUv in parse_pyproject_toml.rs and the corresponding poe-tasks
block and let rustfmt rewrite the brace and indentation to the canonical style.
- Around line 45-67: The conditional that gates adding Poetry scripts uses
cfg!(test), check_path_executable("poetry"), and poetry_lock_exists but lacks
parentheses so && binds tighter than ||; update the if condition in
parse_pyproject_toml.rs (the block that inspects poetry.get("scripts") and
iterates scripts_table) to parenthesize the test or executable check together so
that the existence of poetry.lock (poetry_lock_exists) is required in addition
to either cfg!(test) or check_path_executable("poetry").is_some(); adjust the
condition around cfg!(test) and check_path_executable("poetry") accordingly to
match the semantics used for the UV/Poe branches.
- Around line 71-107: The nested let-chain and inline conditional that selects
tasks_table makes the poe parsing hard to read; refactor by extracting the table
selection into a separate let binding (e.g., let tasks_table =
poe_section.get("tasks").and_then(|t| t.as_table()).or_else(||
poe_section.as_table());) then use a simple if let Some(tasks_table) =
tasks_table && (cfg!(test) || check_path_executable("poe").is_some()) to gate
the for loop that pushes Task entries (symbols to locate: poe_section,
tasks_table, check_path_executable, Task, TaskRunner::PythonPoe); this keeps the
logic identical but much clearer and consistent with the other branches.
In `@src/task_discovery.rs`:
- Around line 820-851: The let-chain condition causes cargo fmt/MSRV issues;
replace the nested if-let with an iterator filter_map or filter + for to avoid
the chained `&& let` and keep formatting stable: iterate over entries.flatten(),
map each entry to its path, then use path.is_file() and
path.extension().and_then(|e| e.to_str()) == Some("sh") to select scripts, build
the task using the same fields (name from file_stem, source_name from file_name,
TaskDefinitionType::ShellScript, TaskRunner::ShellScript, shadowed_by via
check_shadowing(&name)) and push into discovered.tasks; optionally extract the
task-construction block into a small helper/closure (e.g., a function that
returns Option<Task>) to keep the loop concise and readable.
---
Duplicate comments:
In `@src/mcp/allowlist.rs`:
- Around line 38-41: Replace the let-chain patterns in src/mcp/allowlist.rs:
change the "if let AllowScope::Deny = entry.scope &&
self.path_matches(&task.file_path, &entry.path, true)" style to a plain equality
check (entry.scope == AllowScope::Deny) combined with the path match, and for
the triple-chained Task branch (the block around lines 58-62) replace the
nested/multi-condition pattern with an Option::is_some_and(...) check on the
Task option to remove the let-chain dependency; after making these changes run
cargo fmt --all to fix formatting.
🪄 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: bb33fb3a-841c-48f5-bc3e-e76c5c52e1fa
📒 Files selected for processing (19)
.github/workflows/rust.ymlsrc/allowlist.rssrc/commands/init.rssrc/commands/list.rssrc/commands/mcp.rssrc/mcp/allowlist.rssrc/mcp/dto.rssrc/mcp/server.rssrc/parsers/parse_cmake.rssrc/parsers/parse_github_actions.rssrc/parsers/parse_gradle.rssrc/parsers/parse_makefile.rssrc/parsers/parse_package_json.rssrc/parsers/parse_pom_xml.rssrc/parsers/parse_pyproject_toml.rssrc/parsers/parse_taskfile.rssrc/parsers/parse_travis_ci.rssrc/task_discovery.rssrc/types.rs
| AllowScope::Task => { | ||
| if path_matches(&task.file_path, &entry.path, false) { | ||
| if let Some(ref tasks) = entry.tasks { | ||
| if tasks.contains(&task.name) { | ||
| if path_matches(&task.file_path, &entry.path, false) | ||
| && let Some(ref tasks) = entry.tasks | ||
| && tasks.contains(&task.name) { | ||
| return Ok((true, false)); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Triple let-chain is hard to follow; flatten it.
Beyond the fmt failure (pipeline error at line 90) and MSRV-1.88 requirement, the binding let Some(ref tasks) = entry.tasks chained inside && with two other booleans is not particularly readable. A small refactor avoids the let-chain altogether:
♻️ Suggested simplification
AllowScope::Task => {
- if path_matches(&task.file_path, &entry.path, false)
- && let Some(ref tasks) = entry.tasks
- && tasks.contains(&task.name) {
- return Ok((true, false));
- }
+ let task_listed = entry
+ .tasks
+ .as_ref()
+ .is_some_and(|tasks| tasks.contains(&task.name));
+ if task_listed && path_matches(&task.file_path, &entry.path, false) {
+ return Ok((true, false));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AllowScope::Task => { | |
| if path_matches(&task.file_path, &entry.path, false) { | |
| if let Some(ref tasks) = entry.tasks { | |
| if tasks.contains(&task.name) { | |
| if path_matches(&task.file_path, &entry.path, false) | |
| && let Some(ref tasks) = entry.tasks | |
| && tasks.contains(&task.name) { | |
| return Ok((true, false)); | |
| } | |
| } | |
| } | |
| } | |
| AllowScope::Task => { | |
| let task_listed = entry | |
| .tasks | |
| .as_ref() | |
| .is_some_and(|tasks| tasks.contains(&task.name)); | |
| if task_listed && path_matches(&task.file_path, &entry.path, false) { | |
| return Ok((true, false)); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Rust CI
[error] 90-90: cargo fmt --all -- --check failed. Formatting diff detected in allowlist.rs for AllowScope::Task branch (indentation/brace placement). Run 'cargo fmt --all' to apply rustfmt formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/allowlist.rs` around lines 90 - 96, The AllowScope::Task match arm uses a
chained let-binding inside an && expression which is hard to read and breaks
fmt/MSRV; replace the chained condition with a flattened nested check: first
test path_matches(&task.file_path, &entry.path, false), then use an explicit if
let Some(ref tasks) = entry.tasks (or if let Some(tasks) = &entry.tasks) and
inside that check tasks.contains(&task.name) and return Ok((true, false)) when
all conditions hold; update the AllowScope::Task arm accordingly referencing
AllowScope::Task, path_matches, entry.tasks, task.file_path, and task.name.
| let description_single_quote_pattern = r"description\s+'([^']*)'".to_string(); | ||
| let description_double_quote_pattern = r#"description\s+"([^"]*)""#.to_string(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop the unnecessary .to_string() allocations.
These are &'static str literals that are only ever interpolated into format!(...), which accepts &str. Allocating a String here is wasteful and looks like a clippy nudge worth taking while you're cleaning lints.
♻️ Proposed refactor
- let description_single_quote_pattern = r"description\s+'([^']*)'".to_string();
- let description_double_quote_pattern = r#"description\s+"([^"]*)""#.to_string();
+ let description_single_quote_pattern = r"description\s+'([^']*)'";
+ let description_double_quote_pattern = r#"description\s+"([^"]*)""#;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let description_single_quote_pattern = r"description\s+'([^']*)'".to_string(); | |
| let description_double_quote_pattern = r#"description\s+"([^"]*)""#.to_string(); | |
| let description_single_quote_pattern = r"description\s+'([^']*)'"; | |
| let description_double_quote_pattern = r#"description\s+"([^"]*)""#; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/parse_gradle.rs` around lines 134 - 135, Replace the unnecessary
allocations for description_single_quote_pattern and
description_double_quote_pattern in parse_gradle.rs by using &'static str
literals (remove the .to_string()) since they are only passed to format! as
&str; update any variable types/uses if annotated to accept &str so the rest of
the code (e.g., where format!(...) consumes these patterns) continues to compile
without allocating a String.
|
|
||
| // Extract jobs from the configuration | ||
| if let Some(Value::Mapping(jobs_map)) = config_map.get(&Value::String("jobs".to_string())) { | ||
| if let Some(Value::Mapping(jobs_map)) = config_map.get(Value::String("jobs".to_string())) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — clippy::needless_borrow fix applied uniformly.
Equivalent behavior since serde_yaml::mapping::Index is implemented for Value.
Optional: Each call still allocates a new String via "…".to_string(). If you wanted a follow-up clippy/perf pass, serde_yaml::Mapping::get also accepts &str directly, e.g. config_map.get("jobs"), which avoids the per-call allocation entirely. Not in scope for this PR.
Also applies to: 60-60, 64-64, 69-69, 133-133, 138-138, 144-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/parse_travis_ci.rs` at line 37, Replace the redundant allocations
where config_map.get(Value::String("...".to_string())) is used: change those
calls to config_map.get("jobs") (and the other string keys used similarly) to
avoid creating a new String each call; locate uses of config_map.get with
Value::String in parse_travis_ci.rs (e.g., the pattern if let
Some(Value::Mapping(jobs_map)) =
config_map.get(Value::String("jobs".to_string()))) and replace the
Value::String("...".to_string()) argument with a plain &str like "jobs" (repeat
for the other occurrences noted).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mcp/server.rs (1)
2620-2625:⚠️ Potential issue | 🟡 MinorEmpty test body — this test now passes unconditionally.
After removing the placeholder
assert!(true),test_concurrency_limit_in_task_startcontains only comments, so it asserts nothing and just inflates the green-test count. The behavior it claims to cover is already exercised bytest_concurrency_limit_enforcementagainst theJobManagerdirectly.Either delete this test or wire it up to actually drive
task_startagainst a server with a constrainedJobManagerConfig(e.g., via a newnew_with_allowlist_and_job_manager-style test hook) so the concurrency error path throughtask_startitself is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/server.rs` around lines 2620 - 2625, The empty test test_concurrency_limit_in_task_start should either be removed or implemented to actually exercise the task_start path under a constrained JobManager; implement it by creating a server instance configured with a low-concurrency JobManagerConfig (add/use a test helper like new_with_allowlist_and_job_manager or new_with_job_manager_config) and drive task_start RPCs until the JobManager returns the concurrency error, then assert the expected error is returned; alternatively, delete the empty test if coverage via test_concurrency_limit_enforcement is sufficient.Makefile (1)
1-1: 🧹 Nitpick | 🔵 TrivialAdd
lintto the.PHONYdeclaration.The new
linttarget produces no file namedlint, so it should be declared phony to guarantee the recipe always runs (and to prevent any future file collision from silently skipping it). Several existing targets (format,_builder,inspect_mcp,test_long) are also missing from.PHONY— worth fixing in the same spot.Proposed change
-.PHONY: build tests tests_integration test_unit test_noinit test_mcp test_zsh test_bash test_fish test_pwsh run install builder publish +.PHONY: build lint tests tests_integration test_unit test_noinit test_mcp test_zsh test_bash test_fish test_pwsh run install _builder builder publish format inspect_mcp test_longAlso applies to: 13-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Update the .PHONY declaration to include the new lint target and other non-file-producing targets to avoid accidental skipping: add lint plus format, _builder, inspect_mcp, and test_long to the existing .PHONY list so targets like build/tests/run/install/builder/publish remain correct and the lint recipe always executes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/rust.yml:
- Around line 26-32: The cache key in the GitHub Actions step (the `key` field
currently set to `${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}`)
does not include the Rust toolchain version and can reuse artifacts built with a
different rustc; update the `key` to also hash the rust-toolchain file (or
Cargo.toml rust-version) so toolchain bumps invalidate the cache—specifically
modify the `key` expression to include `hashFiles('rust-toolchain.toml')` (or
`hashFiles('**/Cargo.toml')` targeting rust-version) alongside the existing
Cargo.lock hash to ensure `target/` and registry caches are version-aware.
In `@Cargo.toml`:
- Line 4: Update the Cargo.toml rust-version field from the patch-pinned value
to a major.minor only value: change the rust-version declaration (the
"rust-version" entry in Cargo.toml) from "1.91.1" to "1.91" so Cargo's version
check accepts Rust 1.91.0 while your rust-toolchain.toml continues to pin the
exact build for CI/contributors.
In `@rust-toolchain.toml`:
- Around line 1-3: The toolchain is pinned to an exact patch via channel =
"1.91.1"; decide whether you want exact-patch or minor pinning and update the
channel string accordingly (e.g., change channel = "1.91.1" to channel = "1.91"
to allow patch updates), and ensure components = ["rustfmt","clippy"] remains
unchanged; update the channel value in the rust toolchain manifest where the
[toolchain] section and channel key are defined.
In `@src/parsers/parse_pyproject_toml.rs`:
- Around line 46-69: The boolean expression around cfg!(test),
check_path_executable("poetry") and poetry_lock_exists has ambiguous precedence;
change the guard so the poetry_lock_exists check is applied unconditionally by
rewriting the condition to: poetry_lock_exists && (cfg!(test) ||
check_path_executable("poetry").is_some()), and update the if in
parse_pyproject_toml.rs (the block that builds Task with
TaskRunner::PythonPoetry) accordingly; if you intentionally want tests to bypass
the lock check instead, add explicit parentheses to document the intent (e.g.,
cfg!(test) || (check_path_executable("poetry").is_some() && poetry_lock_exists))
and ensure test_parse_poetry_scripts still creates poetry.lock if you keep the
lock requirement.
- Around line 73-82: The nested if-let expression used to compute tasks_table
inside the let-chain is hard to read; refactor by first resolving the tasks
table into a local variable (e.g., let tasks_table_opt =
poe_section.get("tasks").and_then(|inner| inner.as_table()).or_else(||
poe_section.as_table());) and then use a simple let-chain that checks
tasks_table_opt.is_some() and (cfg!(test) ||
check_path_executable("poe").is_some()) to bind tasks_table; update references
to use tasks_table (from tasks_table_opt.unwrap()/as_ref()) and keep symbols
poe_section, tasks_table, and check_path_executable unchanged.
---
Outside diff comments:
In `@Makefile`:
- Line 1: Update the .PHONY declaration to include the new lint target and other
non-file-producing targets to avoid accidental skipping: add lint plus format,
_builder, inspect_mcp, and test_long to the existing .PHONY list so targets like
build/tests/run/install/builder/publish remain correct and the lint recipe
always executes.
In `@src/mcp/server.rs`:
- Around line 2620-2625: The empty test test_concurrency_limit_in_task_start
should either be removed or implemented to actually exercise the task_start path
under a constrained JobManager; implement it by creating a server instance
configured with a low-concurrency JobManagerConfig (add/use a test helper like
new_with_allowlist_and_job_manager or new_with_job_manager_config) and drive
task_start RPCs until the JobManager returns the concurrency error, then assert
the expected error is returned; alternatively, delete the empty test if coverage
via test_concurrency_limit_enforcement is sufficient.
🪄 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: df943a8c-44a7-4bc5-af46-a6ae853f12e8
📒 Files selected for processing (25)
.github/workflows/integration.yml.github/workflows/rust.ymlCargo.tomlMakefilerust-toolchain.tomlsrc/allowlist.rssrc/commands/init.rssrc/commands/list.rssrc/commands/mcp.rssrc/mcp/allowlist.rssrc/mcp/dto.rssrc/mcp/server.rssrc/parsers/parse_cmake.rssrc/parsers/parse_github_actions.rssrc/parsers/parse_gradle.rssrc/parsers/parse_justfile.rssrc/parsers/parse_package_json.rssrc/parsers/parse_pom_xml.rssrc/parsers/parse_pyproject_toml.rssrc/parsers/parse_taskfile.rssrc/parsers/parse_travis_ci.rssrc/prompt.rssrc/task_discovery.rstests/Dockerfile.buildertests/docker_unit/Dockerfile
| uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5 | ||
| with: | ||
| path: | | ||
| ~/.cargo/registry | ||
| ~/.cargo/git | ||
| target | ||
| key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cache key should also key on the Rust toolchain version.
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} does not include the rustc version. When you bump rust-toolchain.toml from 1.91.1 to a newer version in a future PR, the cached target/ will be reused even though it was built by an older rustc, which can cause confusing rebuild diagnostics or, worse, stale artifacts. Consider hashing rust-toolchain.toml (or the Cargo.toml rust-version) into the cache key.
Proposed change
- key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
+ key: ${{ runner.os }}-cargo-${{ hashFiles('rust-toolchain.toml', '**/Cargo.lock') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5 | |
| with: | |
| path: | | |
| ~/.cargo/registry | |
| ~/.cargo/git | |
| target | |
| key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} | |
| uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5 | |
| with: | |
| path: | | |
| ~/.cargo/registry | |
| ~/.cargo/git | |
| target | |
| key: ${{ runner.os }}-cargo-${{ hashFiles('rust-toolchain.toml', '**/Cargo.lock') }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/rust.yml around lines 26 - 32, The cache key in the GitHub
Actions step (the `key` field currently set to `${{ runner.os }}-cargo-${{
hashFiles('**/Cargo.lock') }}`) does not include the Rust toolchain version and
can reuse artifacts built with a different rustc; update the `key` to also hash
the rust-toolchain file (or Cargo.toml rust-version) so toolchain bumps
invalidate the cache—specifically modify the `key` expression to include
`hashFiles('rust-toolchain.toml')` (or `hashFiles('**/Cargo.toml')` targeting
rust-version) alongside the existing Cargo.lock hash to ensure `target/` and
registry caches are version-aware.
| [package] | ||
| name = "dela" | ||
| version = "0.0.6" | ||
| rust-version = "1.91.1" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Cargo rust-version field convention patch version MSRV
💡 Result:
The rust-version field in Cargo.toml specifies the Minimum Supported Rust Version (MSRV), the minimum Rust toolchain version supported by the package. It is an optional [package] key that Cargo uses to error on compilation if the toolchain is too old (unless --ignore-rust-version is used). Format: A bare semantic version number with at least one component (major, major.minor, or major.minor.patch), no semver operators or pre-release identifiers. Official docs use "1.56" (major.minor). Full patch like "1.56.1" is allowed but not shown in examples. Convention for patch version: Official Cargo documentation does not mandate including the patch level; major.minor is the standard and sufficient. Cargo treats rust-version with caret-like compatibility internally for resolution (e.g., "^package.rust-version" for deps), but the declared value is exact for checking the user's toolchain (>= declared version). Projects commonly use major.minor (e.g., stable-2 like 1.75 for current stable minus 2), updating minor when raising MSRV, often as a minor SemVer change in the crate (not breaking). Use tools like cargo-msrv to verify the actual MSRV matches the declared value. Support expectations: Dependencies should have at least one compatible version; Cargo resolver prefers compatible ones (with resolver.incompatible-rust-versions = "fallback" default in edition 2024). Policy recommendation: Define a support window (e.g., latest stable, or N-2 minors), test with cargo-msrv verify.
Citations:
- 1: https://doc.rust-lang.org/cargo/reference/rust-version.html
- 2: https://docs.rust-lang.org/cargo/reference/manifest.html
- 3: https://doc.rust-lang.org/stable/cargo/reference/rust-version.html
- 4: http://doc.rust-lang.org/cargo/reference/rust-version.html
- 5: https://docs.rust-lang.org/cargo/reference/rust-version.html
- 6: MSRV-dependent dependency version resolution rust-lang/cargo#9930
- 7: https://doc.rust-lang.org/cargo/reference/resolver.html?highlight=version
- 8: Set and verify all MSRVs in CI rust-lang/cargo#12654
- 9: MSRV policy apollographql/router#1887
- 10: https://doc.rust-lang.org/cargo/reference/semver.html?highlight=edition
- 11: https://crates.io/crates/cargo-msrv
Use rust-version = "1.91" (major.minor only, no patch version).
Including the patch version (1.91.1) causes cargo to reject users on Rust 1.91.0, even though the crate compiles fine on that version. The rust-version field checks toolchain >= declared version, so 1.91.0 < 1.91.1 incorrectly flags compliant users. The Cargo documentation convention and standard practice is major.minor only; the rust-toolchain.toml already pins the exact build version for contributors and CI.
Proposed change
-rust-version = "1.91.1"
+rust-version = "1.91"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rust-version = "1.91.1" | |
| rust-version = "1.91" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 4, Update the Cargo.toml rust-version field from the
patch-pinned value to a major.minor only value: change the rust-version
declaration (the "rust-version" entry in Cargo.toml) from "1.91.1" to "1.91" so
Cargo's version check accepts Rust 1.91.0 while your rust-toolchain.toml
continues to pin the exact build for CI/contributors.
| [toolchain] | ||
| channel = "1.91.1" | ||
| components = ["rustfmt", "clippy"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Toolchain pin is fine; consider whether you want exact-patch or minor pinning.
channel = "1.91.1" will cause rustup to install/use exactly that patch version for every contributor, which is great for CI reproducibility but means contributors locally won't pick up patch fixes (e.g., a hypothetical 1.91.2) without a manifest bump. If you'd prefer to allow patch updates while still pinning the minor toolchain, channel = "1.91" is the common alternative. As-is is acceptable — just flagging the trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-toolchain.toml` around lines 1 - 3, The toolchain is pinned to an exact
patch via channel = "1.91.1"; decide whether you want exact-patch or minor
pinning and update the channel string accordingly (e.g., change channel =
"1.91.1" to channel = "1.91" to allow patch updates), and ensure components =
["rustfmt","clippy"] remains unchanged; update the channel value in the rust
toolchain manifest where the [toolchain] section and channel key are defined.
| if let Some(poe) = toml.get("tool") | ||
| && let Some(poe_section) = poe.get("poe") | ||
| { | ||
| // If there is a nested "tasks" key, use that table; otherwise, use the poe_section table directly | ||
| if let Some(tasks_table) = if let Some(inner) = poe_section.get("tasks") { | ||
| inner.as_table() | ||
| } else { | ||
| poe_section.as_table() | ||
| } && (cfg!(test) || check_path_executable("poe").is_some()) | ||
| { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nested if let ... else as the bound expression of an outer let-chain is hard to read.
if let Some(tasks_table) = if let Some(inner) = poe_section.get("tasks") {
inner.as_table()
} else {
poe_section.as_table()
} && (cfg!(test) || check_path_executable("poe").is_some())It parses, but the visual grouping of the inner if let { … } else { … } against the outer && is easy to misread. Extracting the table resolution removes the ambiguity and keeps the let-chain shallow.
♻️ Suggested refactor
- if let Some(poe) = toml.get("tool")
- && let Some(poe_section) = poe.get("poe")
- {
- // If there is a nested "tasks" key, use that table; otherwise, use the poe_section table directly
- if let Some(tasks_table) = if let Some(inner) = poe_section.get("tasks") {
- inner.as_table()
- } else {
- poe_section.as_table()
- } && (cfg!(test) || check_path_executable("poe").is_some())
- {
+ if let Some(poe) = toml.get("tool")
+ && let Some(poe_section) = poe.get("poe")
+ {
+ // If there is a nested "tasks" key, use that table; otherwise, use the poe_section table directly
+ let tasks_table = poe_section
+ .get("tasks")
+ .and_then(|inner| inner.as_table())
+ .or_else(|| poe_section.as_table());
+ if let Some(tasks_table) = tasks_table
+ && (cfg!(test) || check_path_executable("poe").is_some())
+ {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/parse_pyproject_toml.rs` around lines 73 - 82, The nested if-let
expression used to compute tasks_table inside the let-chain is hard to read;
refactor by first resolving the tasks table into a local variable (e.g., let
tasks_table_opt = poe_section.get("tasks").and_then(|inner|
inner.as_table()).or_else(|| poe_section.as_table());) and then use a simple
let-chain that checks tasks_table_opt.is_some() and (cfg!(test) ||
check_path_executable("poe").is_some()) to bind tasks_table; update references
to use tasks_table (from tasks_table_opt.unwrap()/as_ref()) and keep symbols
poe_section, tasks_table, and check_path_executable unchanged.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/parsers/parse_pyproject_toml.rs (1)
77-82: 🧹 Nitpick | 🔵 TrivialRefactor nested bound expression in let-chain for readability.
This is still difficult to scan quickly. Consider resolving
tasks_tablefirst, then applying the executable guard in a separate let-chain.♻️ Suggested refactor
- if let Some(tasks_table) = if let Some(inner) = poe_section.get("tasks") { - inner.as_table() - } else { - poe_section.as_table() - } && (cfg!(test) || check_path_executable("poe").is_some()) + let tasks_table = poe_section + .get("tasks") + .and_then(|inner| inner.as_table()) + .or_else(|| poe_section.as_table()); + + if let Some(tasks_table) = tasks_table + && (cfg!(test) || check_path_executable("poe").is_some()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/parse_pyproject_toml.rs` around lines 77 - 82, The nested let-chain that computes tasks_table mixes extracting the table from poe_section and an executable check (check_path_executable("poe")) making it hard to read; refactor parse_pyproject_toml.rs by first binding tasks_table in its own let (e.g., let tasks_table = if let Some(inner) = poe_section.get("tasks") { inner.as_table() } else { poe_section.as_table() };), then use a separate if-let / let-chain to check tasks_table.is_some() && (cfg!(test) || check_path_executable("poe").is_some()) before entering the block — keep the same symbols (poe_section, tasks_table, check_path_executable) and preserve original behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/parsers/parse_pyproject_toml.rs`:
- Around line 77-82: The nested let-chain that computes tasks_table mixes
extracting the table from poe_section and an executable check
(check_path_executable("poe")) making it hard to read; refactor
parse_pyproject_toml.rs by first binding tasks_table in its own let (e.g., let
tasks_table = if let Some(inner) = poe_section.get("tasks") { inner.as_table() }
else { poe_section.as_table() };), then use a separate if-let / let-chain to
check tasks_table.is_some() && (cfg!(test) ||
check_path_executable("poe").is_some()) before entering the block — keep the
same symbols (poe_section, tasks_table, check_path_executable) and preserve
original behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5a239c2-dec3-4514-9d79-230a97bd8e0b
📒 Files selected for processing (1)
src/parsers/parse_pyproject_toml.rs
1 participant
Summary by CodeRabbit
Chores
Refactor
Tests