Conversation
…h structured error handling across all parsers
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR systematically refactors the codebase's error handling from string-based errors ( ChangesError Handling Refactor: anyhow and typed errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (1)
219-228: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove redundant
.to_string()call on&str.The parameter
erris already a&str, which has a.starts_with()method. Calling.to_string()creates an unnecessary heap allocation.♻️ Proposed refactor
let mut handle_error = |err: &str| { - if err - .to_string() - .starts_with("dela: command or task not found") - { + if err.starts_with("dela: command or task not found") { writeln!(stderr_file, "{}", err).unwrap(); } else { writeln!(stderr_file, "Error: {}", err).unwrap();🤖 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/main.rs` around lines 219 - 228, The closure handle_error currently calls err.to_string().starts_with(...), which allocates unnecessarily because err is already a &str; replace that expression with err.starts_with("dela: command or task not found") in the handle_error closure (the rest of the closure that writes to stderr_file and unwraps should remain unchanged) so no heap allocation occurs when checking the prefix.src/parsers/parse_github_actions.rs (1)
24-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize formatting in this block to pass CI.
cargo fmt --all -- --checkis failing on this file; run rustfmt so the style check passes.As per coding guidelines follow coding style guidelines documented in dev_docs/style.md when writing Rust code.
🤖 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/parsers/parse_github_actions.rs` around lines 24 - 49, The formatting in the block around workflow_map, workflow_name, and jobs does not conform to rustfmt; run rustfmt (or `cargo fmt --all`) on the file and reformat the match arms and chained method calls so they match the project's Rust style (e.g., consistent indentation, spacing around match arms, and line breaks for the chained `.get(...)`/`.and_then(...)` calls), then re-run `cargo fmt --all -- --check` to ensure CI passes.
🤖 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/mcp.rs`:
- Around line 4-5: The #[allow(unused_imports)] attribute applied to the use
anyhow::anyhow import is unnecessary because the anyhow! macro is used
throughout this module; remove the #[allow(unused_imports)] annotation and keep
the use anyhow::anyhow import as-is so the macro usage remains valid and the
code is clearer (look for the use statement importing anyhow::anyhow and the
multiple anyhow! macro invocations).
In `@src/commands/run_command.rs`:
- Line 236: The test currently sets let result: anyhow::Result<()> = Err(...)
which bypasses run_command::execute; replace that synthetic Err with an actual
invocation of the function under test (call run_command::execute(...) with the
same arguments/ctx the test is exercising and assign its return to result), so
the test exercises argument parsing and execution; ensure you use the correct
execute symbol (execute or run_command::execute) and propagate/await its Result
into the existing result variable instead of fabricating an error.
In `@src/mcp/job_manager.rs`:
- Around line 1-2: Remove the incorrect attribute by deleting the
#[allow(unused_imports)] annotation that precedes the use anyhow::anyhow; import
line so the macro usage is treated normally; leave the use anyhow::anyhow;
statement intact (since anyhow::anyhow is referenced throughout the file) and
run a build to confirm no unused-import suppression is needed.
In `@src/parsers/errors.rs`:
- Around line 1-26: The file fails rustfmt; reformat the Rust source that
defines the DelaParseError enum so it matches project style: run `cargo fmt
--all` (or `cargo fmt --all -- --check` locally to verify), update the file
containing the `DelaParseError` enum so spacing/indentation and import ordering
follow rustfmt rules, then stage and commit the formatted changes so CI passes.
In `@src/parsers/parse_docker_compose.rs`:
- Around line 35-40: The parse function in src/parsers/parse_docker_compose.rs
is failing the formatter check; run rustfmt (e.g., cargo fmt --all) and reformat
this file so it complies with project style rules. Specifically, format the
parse function and surrounding code (including the read_to_string and
serde_yaml::from_str lines) to satisfy `cargo fmt --all -- --check`, then stage
the updated file for commit.
In `@src/parsers/parse_justfile.rs`:
- Around line 42-45: The current code always wraps indent_error in
DelaParseError::Syntax which causes duplicated “Syntax error” text when
indent_error is already DelaParseError::Syntax; change the return to match on
indent_error: if it's DelaParseError::Syntax(inner_msg) build and return
DelaParseError::Syntax(format!("{}: {}", file_name, inner_msg)), otherwise build
and return DelaParseError::Syntax(format!("{}: {}", file_name,
indent_error.to_string())). This uses the DelaParseError::Syntax variant and the
indent_error/inner_msg values to preserve the original inner message instead of
double-wrapping.
In `@src/parsers/parse_taskfile.rs`:
- Around line 189-193: The load_taskfile function's formatting drifts from
project rustfmt rules causing CI failure; run rustfmt (cargo fmt --all) and
reformat the block around the load_taskfile function (the function returning
Result<Taskfile, DelaParseError> that reads the file and deserializes with
serde_yaml::from_str) so the code adheres to the repository's style (e.g.,
spacing, indentation, and semicolons), then commit the formatting-only changes.
- Around line 152-153: Change the return type of extract_include_directives from
anyhow::Result<Vec<TaskfileInclude>> to Result<Vec<TaskfileInclude>,
DelaParseError> and ensure errors from load_taskfile are propagated as
DelaParseError (use ? if load_taskfile already returns DelaParseError or
map_err(Into::into)/map_err(|e| DelaParseError::from(e)) to convert otherwise);
keep all other parsing logic the same so the parser layer preserves its typed
DelaParseError API contract.
In `@src/parsers/parse_travis_ci.rs`:
- Around line 30-32: The match arm that returns
Err(DelaParseError::Syntax("Travis CI YAML is not a mapping".to_string())); is
misformatted across multiple lines; run rustfmt (cargo fmt --all) or apply the
repository's Rust formatting rules to normalize this block in
src/parsers/parse_travis_ci.rs so the error return follows the project's style
(e.g., a single-line return or properly indented multiline), ensuring the match
arm for the fallback case remains Err(DelaParseError::Syntax(...)) and commits
the formatted change.
In `@src/task_discovery/maven.rs`:
- Around line 27-28: The returned Err currently discards the original parser
error; update the return to preserve the parser details by including the
original error when returning from this block (same place where
handle_discovery_error(...) is called). Concretely, keep the call to
handle_discovery_error(error, pom_path, TaskDefinitionType::MavenPom,
discovered) and change the Err(...) to wrap or format the original error (e.g.,
include error in the message or use anyhow::Context to add "Error parsing
pom.xml" while preserving the source error) so callers can inspect the concrete
parse failure for pom_path.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 219-228: The closure handle_error currently calls
err.to_string().starts_with(...), which allocates unnecessarily because err is
already a &str; replace that expression with err.starts_with("dela: command or
task not found") in the handle_error closure (the rest of the closure that
writes to stderr_file and unwraps should remain unchanged) so no heap allocation
occurs when checking the prefix.
In `@src/parsers/parse_github_actions.rs`:
- Around line 24-49: The formatting in the block around workflow_map,
workflow_name, and jobs does not conform to rustfmt; run rustfmt (or `cargo fmt
--all`) on the file and reformat the match arms and chained method calls so they
match the project's Rust style (e.g., consistent indentation, spacing around
match arms, and line breaks for the chained `.get(...)`/`.and_then(...)` calls),
then re-run `cargo fmt --all -- --check` to ensure CI passes.
🪄 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: 816aa165-aa4b-4020-9bde-6d63fec260c9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
Cargo.tomlsrc/allowlist.rssrc/commands/allow_command.rssrc/commands/configure_shell.rssrc/commands/get_command.rssrc/commands/init.rssrc/commands/list.rssrc/commands/mcp.rssrc/commands/run.rssrc/commands/run_command.rssrc/config.rssrc/main.rssrc/mcp/allowlist.rssrc/mcp/job_manager.rssrc/mcp/server.rssrc/parsers/errors.rssrc/parsers/mod.rssrc/parsers/parse_cmake.rssrc/parsers/parse_docker_compose.rssrc/parsers/parse_github_actions.rssrc/parsers/parse_gradle.rssrc/parsers/parse_justfile.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/parsers/parse_turbo_json.rssrc/prompt.rssrc/runner.rssrc/runners/runners_pyproject_toml.rssrc/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/support.rssrc/task_discovery/taskfile.rssrc/task_discovery/travis_ci.rssrc/task_discovery/turbo.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/run_command.rs (1)
233-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
test_run_command_with_argsnow has a flaky expectation.This test now executes a real command but still asserts failure, so outcome depends on host tooling/runtime instead of deterministic test logic.
Proposed deterministic fix
- let result = super::execute("test"); - assert!( - result.is_err(), - "Command execution should fail in test environment" - ); + let result = super::execute(r#"test "unterminated"#); + assert!(result.is_err(), "Should fail when arguments cannot be parsed"); + assert!( + result + .unwrap_err() + .to_string() + .contains("Failed to parse args"), + "Error should come from argument parsing" + );🤖 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/commands/run_command.rs` around lines 233 - 237, The test test_run_command_with_args is flaky because it calls super::execute("test") which runs a real command; change the test to be deterministic by either (a) invoking super::execute with a guaranteed-failing command name (e.g., "nonexistent_cmd") and assert result.is_err() and specific error text, or (b) refactor to inject or mock the command executor used by execute and in the test provide a stubbed executor that returns a controlled error/success, then assert on that controlled result; update references to super::execute and the assert!(result.is_err()) accordingly so the test no longer relies on host tooling.
🤖 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.
Outside diff comments:
In `@src/commands/run_command.rs`:
- Around line 233-237: The test test_run_command_with_args is flaky because it
calls super::execute("test") which runs a real command; change the test to be
deterministic by either (a) invoking super::execute with a guaranteed-failing
command name (e.g., "nonexistent_cmd") and assert result.is_err() and specific
error text, or (b) refactor to inject or mock the command executor used by
execute and in the test provide a stubbed executor that returns a controlled
error/success, then assert on that controlled result; update references to
super::execute and the assert!(result.is_err()) accordingly so the test no
longer relies on host tooling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bf62b9f-db40-4cf9-87a4-f96efed6b5cf
📒 Files selected for processing (8)
src/commands/run_command.rssrc/main.rssrc/parsers/errors.rssrc/parsers/parse_docker_compose.rssrc/parsers/parse_github_actions.rssrc/parsers/parse_taskfile.rssrc/parsers/parse_travis_ci.rssrc/task_discovery/maven.rs
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/init.rs`:
- Around line 52-54: The current fs::read_to_string call in init (the read into
`content` using `config_path`) should treat a NotFound error as empty content
instead of returning an error; change the read logic to match/map the result and
convert Err(e) where e.kind() == std::io::ErrorKind::NotFound into
Ok(String::new()), otherwise propagate the error (preserving the existing
anyhow::anyhow! message), so the append/create flow can proceed when the shell
config doesn't yet exist.
In `@src/config.rs`:
- Around line 18-19: Replace the use of anyhow::Result for internal helpers with
a domain-specific thiserror enum (e.g., ConfigError) and change signatures like
preferred_config_dir_path() to return Result<PathBuf, ConfigError>; implement
variants for the failure cases used by these helpers (e.g., HomeNotSet,
InvalidPath, IoError) and convert underlying errors into those variants inside
functions such as get_current_home(), preferred_config_dir_path(), and the other
helpers referenced on lines 23, 28, 33, 47, 51; keep error context by mapping
underlying errors (from std::env, std::fs, Path operations) into the appropriate
ConfigError variants (using map_err or From impls) and reserve conversion to
anyhow::Error only at the CLI command boundary where these functions are called.
In `@src/task_discovery/turbo.rs`:
- Around line 53-57: The error branch currently attributes every parse error to
the root turbo.json (using turbo_json.display()), which is misleading because e
can originate from workspace-local or ancestor configs; update the Err(e)
handling in the block that pushes to discovered.errors and returns
TaskFileStatus::ParseError to avoid hardcoding turbo_json: use the error
message/source instead (e.g., include e.to_string() or extract any filename/path
from the error if available) so the log reads something like "Failed to parse
config: {error}" and return TaskFileStatus::ParseError(e.to_string()) still;
modify the code around discovered.errors.push(...) referencing turbo_json and
Err(e) to include the actual error text/source rather than turbo_json.display().
- Line 19: Replace the use of anyhow::Result in the internal helper
discover_turbo_tasks (and other internal helpers used by
collect_turbo_tasks_for_context) with a domain-specific error enum (e.g.,
TurboDiscoverError) implemented with thiserror::Error so the helper functions
return Result<..., TurboDiscoverError>; convert to anyhow only at the CLI
boundary. Update all error returns and propagation in functions like
discover_turbo_tasks and the helpers called by collect_turbo_tasks_for_context
to use the new TurboDiscoverError variants (parse_failure, io,
workspace_local_config, etc.). Also fix the error formatting where
collect_turbo_tasks_for_context errors are reported: when it returns Err,
surface/label that as a workspace-local configuration error (not a root
turbo.json parse failure) and include the original TurboDiscoverError
variant/message. Ensure all call sites that previously expected anyhow::Result
are updated to map/into anyhow at the outermost CLI-facing function.
🪄 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: 2f1c5523-0294-44e2-aa80-4a81ee458bc5
📒 Files selected for processing (14)
dev_docs/style.mdsrc/commands/allow_command.rssrc/commands/configure_shell.rssrc/commands/get_command.rssrc/commands/init.rssrc/commands/mcp.rssrc/commands/run_command.rssrc/config.rssrc/mcp/job_manager.rssrc/mcp/mod.rssrc/parsers/parse_justfile.rssrc/task_discovery/make.rssrc/task_discovery/taskfile.rssrc/task_discovery/turbo.rs
💤 Files with no reviewable changes (1)
- src/mcp/mod.rs
Comment on lines
+18
to
+19
| pub fn preferred_config_dir_path() -> anyhow::Result<PathBuf> { | ||
| let home = get_current_home().context("HOME environment variable not set")?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "dev_docs" -type d 2>/dev/null | head -5Repository: aleyan/dela
Length of output: 37
🏁 Script executed:
find . -name "style.md" -type f 2>/dev/nullRepository: aleyan/dela
Length of output: 75
🏁 Script executed:
head -100 dev_docs/style.mdRepository: aleyan/dela
Length of output: 1708
🏁 Script executed:
cat -n src/config.rsRepository: aleyan/dela
Length of output: 2354
🏁 Script executed:
rg "preferred_config_dir_path|legacy_dela_config_dir|legacy_allowlist_path|active_dela_config_dir|preferred_allowlist_path|active_allowlist_path" -t rs --glob '!src/config.rs' -A 2 -B 2Repository: aleyan/dela
Length of output: 82
🏁 Script executed:
rg "preferred_config_dir_path|legacy_dela_config_dir|legacy_allowlist_path|active_dela_config_dir|preferred_allowlist_path|active_allowlist_path" --type rust --glob '!src/config.rs' -A 2 -B 2Repository: aleyan/dela
Length of output: 9269
Define a typed config error enum for these internal helpers.
These functions are internal library logic used throughout the codebase, not CLI command boundaries. Per the style guide in dev_docs/style.md, define a domain-specific thiserror-based error type here instead of using anyhow::Result. Convert to anyhow at the CLI command layer where appropriate.
Also applies to: lines 23, 28, 33, 47, 51
🤖 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/config.rs` around lines 18 - 19, Replace the use of anyhow::Result for
internal helpers with a domain-specific thiserror enum (e.g., ConfigError) and
change signatures like preferred_config_dir_path() to return Result<PathBuf,
ConfigError>; implement variants for the failure cases used by these helpers
(e.g., HomeNotSet, InvalidPath, IoError) and convert underlying errors into
those variants inside functions such as get_current_home(),
preferred_config_dir_path(), and the other helpers referenced on lines 23, 28,
33, 47, 51; keep error context by mapping underlying errors (from std::env,
std::fs, Path operations) into the appropriate ConfigError variants (using
map_err or From impls) and reserve conversion to anyhow::Error only at the CLI
command boundary where these functions are called.
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
Chores
Bug Fixes / Reliability