Conversation
Reviewer's GuideEnhance the manifest parser’s YAML error handling by mapping serde_yml errors through a custom formatter that reports line/column information and context-aware hints, and verify these diagnostics with dedicated unit tests. Sequence diagram for YAML parse error reporting with hintssequenceDiagram
participant ManifestParser
participant YamlErrorFormatter
participant User
User->>ManifestParser: Provide YAML manifest string
ManifestParser->>YamlErrorFormatter: On parse error, call format_yaml_error(err, src)
YamlErrorFormatter-->>ManifestParser: Return formatted error with line/column and hint
ManifestParser-->>User: Return error with diagnostics and suggestions
Class diagram for improved YAML error handling in manifest parserclassDiagram
class ManifestParser {
+from_str(yaml: &str) Result<NetsukeManifest>
}
class YamlErrorFormatter {
+format_yaml_error(err: YamlError, src: &str) -> anyhow::Error
}
ManifestParser --> YamlErrorFormatter: uses
class YamlError {
+location() -> Option<Location>
+to_string() -> String
}
YamlErrorFormatter --> YamlError: formats
class NetsukeManifest
ManifestParser --> NetsukeManifest: parses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 18 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughIntroduce a dedicated YAML parse error formatter and wire it into manifest::from_str to surface detailed messages with location and hints. Remove the old generic YAML parse error constant. Add unit tests asserting line/column and hint content for tab indentation and missing colon cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Manifest as manifest::from_str
participant Serde as serde_yml::from_str
participant Formatter as format_yaml_error
Caller->>Manifest: from_str(yaml)
Manifest->>Serde: parse YAML
Serde-->>Manifest: Ok(Value) or Err(YamlError)
alt YAML error
Manifest->>Formatter: format_yaml_error(error, yaml)
Formatter-->>Manifest: Detailed error message
Manifest-->>Caller: Err(detailed message)
else Success
Manifest-->>Caller: Ok(Manifest)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Instead of constructing a fresh anyhow! error in format_yaml_error, consider using with_context or error.source() to attach the original YamlError as the cause so you don’t lose the original error chain.
- You might extract the hint-detection logic (matching substrings in err_str) into a dedicated lookup table or helper to make it easier to extend and maintain as you add more YAML error cases.
- For even clearer diagnostics, consider including the actual source line (and a caret at the column) in the error message so users can visually spot the problem in context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of constructing a fresh anyhow! error in format_yaml_error, consider using with_context or error.source() to attach the original YamlError as the cause so you don’t lose the original error chain.
- You might extract the hint-detection logic (matching substrings in err_str) into a dedicated lookup table or helper to make it easier to extend and maintain as you add more YAML error cases.
- For even clearer diagnostics, consider including the actual source line (and a caret at the column) in the error message so users can visually spot the problem in context.
## Individual Comments
### Comment 1
<location> `tests/yaml_error_tests.rs:3` </location>
<code_context>
+use netsuke::manifest;
+
+#[test]
+fn reports_line_and_column_with_tab_hint() {
+ let yaml = "targets:\n\t- name: test\n";
</code_context>
<issue_to_address>
Consider adding a test for YAML parse errors without location information.
Existing tests only cover errors with location details. Please add a test for errors lacking this information to verify correct error formatting.
Suggested implementation:
```rust
#[test]
```
```rust
#[test]
fn reports_error_without_location() {
// This YAML is invalid in a way that does not produce location info (e.g., empty input)
let yaml = "";
let err = manifest::from_str(yaml).expect_err("parse should fail");
let msg = err.to_string();
// Should not mention line/column
assert!(
!msg.contains("line"),
"unexpected location info in error: {msg}"
);
assert!(
!msg.contains("column"),
"unexpected location info in error: {msg}"
);
// Should still be a useful error message
assert!(
!msg.is_empty(),
"error message should not be empty"
);
}
```
</issue_to_address>
### Comment 2
<location> `tests/yaml_error_tests.rs:16` </location>
<code_context>
+}
+
+#[test]
+fn suggests_colon_when_missing() {
+ let yaml = "targets:\n - name: hi\n command echo\n";
+ let err = manifest::from_str(yaml).expect_err("parse should fail");
+ let msg = err.to_string();
+ assert!(msg.contains("line 3"), "missing line info: {msg}");
+ assert!(msg.contains("expected ':'"), "missing error detail: {msg}");
+ assert!(
+ msg.contains("Ensure each key is followed by ':'"),
+ "missing suggestion: {msg}"
+ );
+}
</code_context>
<issue_to_address>
Add a test for the 'did not find expected -' error hint.
Please include a test with a YAML input that triggers the 'did not find expected -' error to confirm the hint appears as intended.
</issue_to_address>
### Comment 3
<location> `src/manifest.rs:16` </location>
<code_context>
-const ERR_INITIAL_YAML_PARSE: &str = "initial YAML parse error";
const ERR_MANIFEST_PARSE: &str = "manifest parse error";
+fn format_yaml_error(err: &YamlError, src: &str) -> anyhow::Error {
+ let location = err.location().map(|l| (l.line(), l.column()));
+ let err_str = err.to_string();
</code_context>
<issue_to_address>
Consider replacing the nested if/else chain with a lookup table for YAML error hints to improve maintainability.
Here’s one way to collapse that nested‐`if`/`else` chain into a small lookup table. You still get the same hints, but it’s far easier to extend or change later:
```rust
// near the top of the file
// (all patterns must be lowercase)
const YAML_HINTS: &[(&str, &str)] = &[
("did not find expected '-'", "Start list items with '-' and ensure proper indentation."),
("expected ':'", "Ensure each key is followed by ':' separating key and value."),
];
fn format_yaml_error(err: &YamlError, src: &str) -> anyhow::Error {
let location = err.location().map(|l| (l.line(), l.column()));
let err_str = err.to_string();
let mut msg = match location {
Some((line, col)) => format!("YAML parse error at line {line}, column {col}: {err_str}"),
None => format!("YAML parse error: {err_str}"),
};
// build hints via a small table + a special tab‐case
let lower = err_str.to_lowercase();
let hint = if src.contains('\t') {
Some("Use spaces for indentation; tabs are invalid in YAML.")
} else {
YAML_HINTS
.iter()
.find_map(|(pat, hint)| lower.contains(pat).then(|| *hint))
};
if let Some(h) = hint {
use std::fmt::Write;
write!(&mut msg, " Hint: {h}").unwrap();
}
anyhow!(msg)
}
```
Advantages:
- All your patterns live in one small `YAML_HINTS` array.
- The `find_map` replaces the manual chain of `if/else if`.
- It’s trivial to add/remove hints or special‐case rules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,26 @@ | |||
| use netsuke::manifest; | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for YAML parse errors without location information.
Existing tests only cover errors with location details. Please add a test for errors lacking this information to verify correct error formatting.
Suggested implementation:
#[test]#[test]
fn reports_error_without_location() {
// This YAML is invalid in a way that does not produce location info (e.g., empty input)
let yaml = "";
let err = manifest::from_str(yaml).expect_err("parse should fail");
let msg = err.to_string();
// Should not mention line/column
assert!(
!msg.contains("line"),
"unexpected location info in error: {msg}"
);
assert!(
!msg.contains("column"),
"unexpected location info in error: {msg}"
);
// Should still be a useful error message
assert!(
!msg.is_empty(),
"error message should not be empty"
);
}| assert!( | ||
| msg.contains("Use spaces for indentation"), | ||
| "missing hint: {msg}" | ||
| ); |
There was a problem hiding this comment.
suggestion (testing): Add a test for the 'did not find expected -' error hint.
Please include a test with a YAML input that triggers the 'did not find expected -' error to confirm the hint appears as intended.
| const ERR_INITIAL_YAML_PARSE: &str = "initial YAML parse error"; | ||
| const ERR_MANIFEST_PARSE: &str = "manifest parse error"; | ||
|
|
||
| fn format_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { |
There was a problem hiding this comment.
issue (complexity): Consider replacing the nested if/else chain with a lookup table for YAML error hints to improve maintainability.
Here’s one way to collapse that nested‐if/else chain into a small lookup table. You still get the same hints, but it’s far easier to extend or change later:
// near the top of the file
// (all patterns must be lowercase)
const YAML_HINTS: &[(&str, &str)] = &[
("did not find expected '-'", "Start list items with '-' and ensure proper indentation."),
("expected ':'", "Ensure each key is followed by ':' separating key and value."),
];
fn format_yaml_error(err: &YamlError, src: &str) -> anyhow::Error {
let location = err.location().map(|l| (l.line(), l.column()));
let err_str = err.to_string();
let mut msg = match location {
Some((line, col)) => format!("YAML parse error at line {line}, column {col}: {err_str}"),
None => format!("YAML parse error: {err_str}"),
};
// build hints via a small table + a special tab‐case
let lower = err_str.to_lowercase();
let hint = if src.contains('\t') {
Some("Use spaces for indentation; tabs are invalid in YAML.")
} else {
YAML_HINTS
.iter()
.find_map(|(pat, hint)| lower.contains(pat).then(|| *hint))
};
if let Some(h) = hint {
use std::fmt::Write;
write!(&mut msg, " Hint: {h}").unwrap();
}
anyhow!(msg)
}Advantages:
- All your patterns live in one small
YAML_HINTSarray. - The
find_mapreplaces the manual chain ofif/else if. - It’s trivial to add/remove hints or special‐case rules.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/manifest.rs(2 hunks)tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
tests/yaml_error_tests.rssrc/manifest.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/yaml_error_tests.rssrc/manifest.rs
tests/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection (e.g., Env/Clock traits) using mockable crate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/yaml_error_tests.rs
src/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
src/**/*.rs: Every module must begin with a //! module-level comment explaining its purpose and utility
Never export eyre::Report from library code; convert to domain error enums at API boundaries
Files:
src/manifest.rs
🧬 Code Graph Analysis (1)
tests/yaml_error_tests.rs (1)
src/manifest.rs (1)
from_str(50-71)
🔍 Remote MCP Context7, Deepwiki
Summary of Additional Context for PR #127 Review:
-
Original YAML parsing in
manifest::from_strusedserde_yml::from_strwith generic context:- Errors were wrapped with
.context("YAML parse error")viaanyhowfor generic messaging.
- Errors were wrapped with
-
No existing
format_yaml_errorfunction found in pre-PR code or documentation:- Searches in codebase and wiki yielded no direct references to
format_yaml_errorbefore PR changes. Error handling relied onanyhowcontexts andthiserrorfor friendly messages, as described indocs/netsuke-design.mdandsrc/manifest.rs.
- Searches in codebase and wiki yielded no direct references to
-
Design philosophy for YAML error handling:
- The five-stage pipeline’s “YAML Parse” stage aims to surface line/column and actionable hints (e.g., spaces vs tabs), implemented using
anyhow::Context.
- The five-stage pipeline’s “YAML Parse” stage aims to surface line/column and actionable hints (e.g., spaces vs tabs), implemented using
No direct code diff for format_yaml_error could be retrieved, indicating its addition is new and isolated to this PR. Review should focus on verifying that the new function correctly captures error location, context hints, and integrates with existing error-handling patterns.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (2)
src/manifest.rs (1)
10-11: Alias the YAML error type locally — LGTMKeep the alias; it improves readability at call sites and in the formatter.
tests/yaml_error_tests.rs (1)
3-13: Assert location and hint — LGTM; keep this as a regression guardThe test captures both the precise location and the indentation hint. This guards the intended UX.
| fn format_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { | ||
| let location = err.location().map(|l| (l.line(), l.column())); | ||
| let err_str = err.to_string(); | ||
| let mut msg = match location { | ||
| Some((line, column)) => { | ||
| format!("YAML parse error at line {line}, column {column}: {err_str}") | ||
| } | ||
| None => format!("YAML parse error: {err_str}"), | ||
| }; | ||
| let lower = err_str.to_lowercase(); | ||
| let hint = if src.contains('\t') { | ||
| Some("Use spaces for indentation; tabs are invalid in YAML.") | ||
| } else if lower.contains("did not find expected '-'") { | ||
| Some("Start list items with '-' and ensure proper indentation.") | ||
| } else if lower.contains("expected ':'") { | ||
| Some("Ensure each key is followed by ':' separating key and value.") | ||
| } else { | ||
| None | ||
| }; | ||
| if let Some(h) = hint { | ||
| use std::fmt::Write; | ||
| write!(&mut msg, " Hint: {h}").expect("string write"); | ||
| } | ||
| anyhow!(msg) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine tab detection to indentation-only and avoid panicking on write
Prevent false-positive “tabs” hints when a tab appears in quoted scalars or non-indentation positions. Also avoid the unnecessary write! + expect by appending directly to the String. While here, cover the common “mapping values are not allowed in this context” variant for a missing colon.
Apply this diff within the shown range:
@@
- let lower = err_str.to_lowercase();
- let hint = if src.contains('\t') {
+ let lower = err_str.to_lowercase();
+ let hint = if has_tab_in_indentation(src) {
Some("Use spaces for indentation; tabs are invalid in YAML.")
} else if lower.contains("did not find expected '-'") {
Some("Start list items with '-' and ensure proper indentation.")
- } else if lower.contains("expected ':'") {
+ } else if lower.contains("expected ':'") || lower.contains("mapping values are not allowed in this context") {
Some("Ensure each key is followed by ':' separating key and value.")
} else {
None
};
if let Some(h) = hint {
- use std::fmt::Write;
- write!(&mut msg, " Hint: {h}").expect("string write");
+ msg.push_str(" Hint: ");
+ msg.push_str(h);
}Add this helper anywhere in the module (e.g., just above format_yaml_error) to scope the hint to indentation:
fn has_tab_in_indentation(src: &str) -> bool {
src.lines()
.any(|line| line.chars().take_while(|c| c.is_whitespace()).any(|c| c == '\t'))
}🤖 Prompt for AI Agents
In src/manifest.rs around lines 16 to 40, the YAML error formatting currently
flags any tab anywhere in the source and uses write! with expect to append the
hint; change this so tab detection only considers indentation by adding the
helper fn has_tab_in_indentation(src: &str) -> bool { src.lines().any(|line|
line.chars().take_while(|c| c.is_whitespace()).any(|c| c == '\t')) } (place it
in the module above format_yaml_error), replace the src.contains('\t') check
with has_tab_in_indentation(src), replace the write!(&mut msg, ...) + expect
with direct msg.push_str(...) to append the hint, and also add a hint branch for
the error message variant containing "mapping values are not allowed in this
context" to suggest a missing colon.
| /// Returns an error if YAML parsing or Jinja evaluation fails. | ||
| pub fn from_str(yaml: &str) -> Result<NetsukeManifest> { | ||
| let mut doc: YamlValue = serde_yml::from_str(yaml).context(ERR_INITIAL_YAML_PARSE)?; | ||
| let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| format_yaml_error(&e, yaml))?; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Retain the original error as the source for better error chains
Mapping to a freshly formatted anyhow::Error discards the original serde_yml::Error as the source. Preserve the cause chain so callers can inspect/log the underlying parser error with full details. Introduce a small error wrapper that implements std::error::Error with the source set to the original error, or switch to a typed error enum exposing both message and source.
I can sketch a minimal thiserror-based ManifestYamlError { msg, #[source] source: serde_yml::Error } and wire it here without touching call sites. Want me to draft it?
| #[test] | ||
| fn suggests_colon_when_missing() { | ||
| let yaml = "targets:\n - name: hi\n command echo\n"; | ||
| let err = manifest::from_str(yaml).expect_err("parse should fail"); | ||
| let msg = err.to_string(); | ||
| assert!(msg.contains("line 3"), "missing line info: {msg}"); | ||
| assert!(msg.contains("expected ':'"), "missing error detail: {msg}"); | ||
| assert!( | ||
| msg.contains("Ensure each key is followed by ':'"), | ||
| "missing suggestion: {msg}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Broaden coverage: add tests for missing list dash and avoid false-positive tab hints
Extend the suite to cover the “expected '-'” path and ensure tab hints are not emitted when tabs appear in quoted scalars (non-indentation). This protects against regressions in the heuristic.
Add the following tests below in this file:
#[test]
fn suggests_dash_when_missing_for_sequence_items() {
// Missing '-' before 'one' under deps should produce a list-dash hint.
let yaml = "targets:\n - name: hi\n deps:\n one\n";
let err = manifest::from_str(yaml).expect_err("parse should fail");
let msg = err.to_string();
assert!(msg.to_lowercase().contains("expected '-'") || msg.to_lowercase().contains("did not find expected '-'"),
"missing parser detail about expected '-' : {msg}");
assert!(msg.contains("Start list items with '-'"), "missing list dash hint: {msg}");
}
#[test]
fn does_not_hint_tabs_for_tabs_inside_quotes() {
// Tabs inside quoted scalars are valid; if another error occurs, do not emit the “tabs” hint.
// Force an unrelated error (missing ':') while including a quoted tab.
let yaml = "targets:\n - name: \"tab\\tinside\"\n command echo\n";
let err = manifest::from_str(yaml).expect_err("parse should fail");
let msg = err.to_string();
assert!(!msg.contains("tabs are invalid in YAML"), "tab hint should not trigger for quoted tabs: {msg}");
assert!(msg.contains("expected ':'"), "missing colon diagnostic: {msg}");
}🤖 Prompt for AI Agents
In tests/yaml_error_tests.rs around lines 15–26, add two new unit tests below
the existing test: one that verifies the parser emits an "expected '-'" (or "did
not find expected '-'") message and the "Start list items with '-'" suggestion
when a sequence dash is missing, and another that ensures when a quoted scalar
contains a tab the diagnostics do not include the "tabs are invalid in YAML"
hint while still reporting the missing ':' error; insert the exact assertions
shown in the review comment, using to_lowercase() for the dash message check and
negating the tab hint check so the suite covers both the missing-list-dash path
and avoids false-positive tab hints for quoted tabs.
|
Superseded by #129 |
Summary
closes #49
Testing
make check-fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68a2c9e1467483229721c7ed29af854b
Summary by Sourcery
Provide detailed YAML parse errors by including line/column information and context-sensitive hints for common mistakes, replace the generic initial parse error handling, and add tests to verify the new diagnostics.
New Features:
Enhancements:
Tests: