Improve YAML error reporting with miette#129
Conversation
Reviewer's GuideThis PR integrates miette-based diagnostics in YAML parsing to emit precise spans and actionable hints, standardizes error wrapping with miette across parsing and rendering workflows, updates documentation to reflect the new diagnostics strategy, and adds the necessary dependencies for runtime and testing. Sequence diagram for YAML parsing with miette diagnosticssequenceDiagram
participant User
participant manifest
participant serde_yml
participant miette
User->>manifest: from_str(yaml)
manifest->>serde_yml: from_str(yaml)
alt Parse error
serde_yml-->>manifest: YamlError
manifest->>miette: map_yaml_error(err, yaml, name)
miette-->>manifest: Report<YamlDiagnostic>
manifest-->>User: Error with span and hint
else Success
serde_yml-->>manifest: YamlValue
manifest-->>User: NetsukeManifest
end
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 4 minutes and 13 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 selected for processing (3)
Summary by CodeRabbit
WalkthroughIntroduce miette-based structured diagnostics and YAML-specific hinting into manifest parsing, update tests to assert rendered diagnostics (stripping ANSI in CI), add docs referencing miette and diag-json, and add two dependencies (miette and strip-ansi-escapes). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Manifest as manifest::from_str_named
participant Serde as serde_yaml::from_str
participant Mapper as map_yaml_error
participant Reporter as miette::Report
Caller->>Manifest: from_str(yaml) / from_str_named(yaml,name)
Manifest->>Serde: parse(yaml)
alt Parse success
Serde-->>Manifest: Ok(deserialised)
Manifest-->>Caller: Ok(manifest)
else Parse failure
Serde-->>Manifest: Err(YamlError{ loc, msg })
Manifest->>Mapper: map_yaml_error(err, source, name)
Mapper->>Reporter: build Report with NamedSource, SourceSpan, hints
Mapper-->>Manifest: Err(Report)
Manifest-->>Caller: Err(Report) (renderable by GraphicalReportHandler)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## 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 a specific hint.
Adding a test for generic YAML parse errors will help verify that error reporting remains informative even when no custom hint is triggered.
</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>
Consider testing for other actionable hints, such as missing dash for list items.
Add a test case for a list item missing a dash to confirm the hint is triggered as expected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
tests/yaml_error_tests.rs (1)
1-27: Consolidate into parameterised rstest cases and harden assertions.Replace near-duplicate tests with a
#[rstest]table and assert on stable substrings (location, primary cause, and hint). This reduces duplication and aligns with test guidelines.Apply a refactor similar to:
use netsuke::manifest; use rstest::rstest; #[rstest] #[case("targets:\n\t- name: test\n", &["line 2, column 1", "Use spaces for indentation"])] #[case("targets:\n - name: hi\n command echo\n", &["line 3", "expected ':'", "Ensure each key is followed by ':'"])] fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) { let err = manifest::from_str(yaml).expect_err("parse should fail"); let msg = err.to_string(); for needle in needles { assert!(msg.contains(needle), "missing: {needle}\nmessage: {msg}"); } }
📜 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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)docs/netsuke-design.md(3 hunks)src/manifest.rs(2 hunks)tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges and keep dependencies up to date
Mandate SemVer caret requirements for all dependencies (e.g., "1.2.3")
Prohibit wildcard (*) and open-ended (>=) version specifiers; use ~ only with a specific, documented reason
Files:
Cargo.toml
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Treat docs/ markdown as the reference source of truth for requirements and decisions; keep updated with changes
Use en-GB-oxendict spelling/grammar in documentation (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Wrap Markdown paragraphs and bullet points at 80 columns; do not wrap tables or headings
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Validate Mermaid diagrams in Markdown using make nixie
Files:
docs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
🧬 Code Graph Analysis (1)
tests/yaml_error_tests.rs (1)
src/manifest.rs (1)
from_str(92-113)
🔍 Remote MCP
Here’s the key additional context to assist your review:
-
Issue #49 (“Add friendlier YAML syntax error messages”) calls for replacing opaque parse failures with messages that name
– What happened
– Where (line & column)
– Why (error type)
– How to fix (actionable hints)
[::functions.ask_question::] -
In src/manifest.rs, YAML parse errors are now converted via map_yaml_error into a
YamlDiagnostic(miette Diagnostic) that:
– Attaches aSourceSpanfromserde_spanned::Spanned
– Includes a hint viahint_forfor common YAML issues:
• Tab indentation (suggest “use spaces”)
• Missing list dash (“ensure each list item starts with ‘-’”)
• Missing colon (“ensure each key is followed by ‘:’”)
– Wraps intoanyhow::Errorwith codenetsuke.yaml.parse -
tests/yaml_error_tests.rs validate that error messages include both:
– The precise “line X, column Y” location
– The corresponding hint text (e.g. “Use spaces for indentation” or “Ensure each key is followed by ‘:’”) -
docs/netsuke-design.md and docs/netsuke-design.md’s technology-summary table were updated to document miette + serde_spanned usage and a new
diag-jsonoption for machine-friendly error output.
This confirms the PR addresses each objective from #49 by adding span-aware diagnostics, descriptive messages, and actionable hints.
⏰ 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 (5)
src/manifest.rs (3)
9-17: Adopt miette/serde_spanned imports cleanly; keep error context uniform.The new imports and constants align with the diagnostics strategy. Keep using
anyhow::Contextfor higher-level wrapping to maintain uniform error chains.
20-31: Compute a minimal, robust span; LGTM.The span calculation guards against out-of-bounds and avoids highlighting newlines. This is a sensible default for pinpointing parse locations.
93-93: Maintain structured parse errors; LGTM.Mapping
serde_yml::from_strerrors throughmap_yaml_errormeets the friendly-error requirements. Keep this path the same when introducing the filename threading suggested above.docs/netsuke-design.md (2)
1570-1579: Document the error‑reporting stack succinctly; LGTM.The technology summary reflects
miette + serde_spannedcorrectly. Keep this table unwrapped as required by the docs policy.
1272-1279: Wrap the “7.2” paragraph at 80 columns and maintain en-GB spellingWrap the new paragraph under “7.2 Crate Selection and Strategy” so that no line exceeds 80 characters, using en-GB spelling.
Leave the heading and any tables unwrapped.
Manually run your project’s markdownlint command to confirm there are no lint errors.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/manifest.rs (5)
304-308: Propagate the actual file name from from_path.Do not lose the filename. Add a small helper and use the path’s display name when mapping YAML errors.
Outside this hunk, add:
// New helper pub fn from_str_named(yaml: &str, name: &str) -> Result<NetsukeManifest> { let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml, name))?; let mut env = Environment::new(); env.set_undefined_behavior(UndefinedBehavior::Strict); if let Some(vars) = doc.get("vars").and_then(|v| v.as_mapping()).cloned() { for (k, v) in vars { let key = k .as_str() .ok_or_else(|| anyhow!("non-string key in 'vars' mapping: {k:?}"))? .to_string(); env.add_global(key, Value::from_serialize(v)); } } expand_foreach(&mut doc, &env)?; let manifest: NetsukeManifest = serde_yml::from_value(doc).context(ERR_MANIFEST_PARSE)?; render_manifest(manifest, &env) }Then update this function:
pub fn from_path(path: impl AsRef<Path>) -> Result<NetsukeManifest> { let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref) .with_context(|| format!("Failed to read {}", path_ref.display()))?; - from_str(&data) + from_str_named(&data, &path_ref.display().to_string()) }
31-46: Fix misuse of #[diagnostic] and drop dynamic label field.Attach
#[diagnostic(...)]to the struct, not to a field, and remove the dynamiclabel: String. miette expects a literal in#[label("…")]. Also remove#[doc(hidden)]from a public type and document it per guidelines.Apply:
#[derive(Debug, Error, Diagnostic)] -#[error("{message}")] -#[doc(hidden)] -pub struct YamlDiagnostic { +/// Diagnostic for YAML parse errors in Netsuke manifests. +/// +/// Includes a precise span, a readable message, and optional actionable help. +#[error("{message}")] +#[diagnostic(code(netsuke.yaml.parse))] +pub struct YamlDiagnostic { #[source_code] src: NamedSource, - #[label("{label}")] + #[label("parse error here")] span: Option<SourceSpan>, #[help] help: Option<String>, #[source] source: Option<anyhow::Error>, - #[diagnostic(code(netsuke.yaml.parse))] message: String, - label: String, }
48-59: Scope the “tabs are invalid” hint to indentation near the fault.Avoid emitting the tabs hint for any tab anywhere in the file. Limit the check to leading indentation on the fault line. Thread the optional location into
hint_for.Apply:
-fn hint_for(err_str: &str, src: &str) -> Option<String> { +fn hint_for(err_str: &str, src: &str, loc: Option<Location>) -> Option<String> { let lower = err_str.to_lowercase(); - if src.contains('\t') { - Some("Use spaces for indentation; tabs are invalid in YAML.".into()) - } else if lower.contains("did not find expected '-'") { + if let Some(loc) = loc { + // Only inspect leading indentation on the fault line. + let idx = loc.index(); + let line_start = src[..idx].rfind('\n').map(|i| i + 1).unwrap_or(0); + let line_end = src[idx..].find('\n').map(|i| idx + i).unwrap_or(src.len()); + let line = &src[line_start..line_end]; + let mut saw_tab_in_indent = false; + for ch in line.chars() { + if ch == ' ' { + continue; + } else if ch == '\t' { + saw_tab_in_indent = true; + break; + } else { + break; // first non-whitespace + } + } + if saw_tab_in_indent { + return Some("Use spaces for indentation; tabs are invalid in YAML.".into()); + } + } + if lower.contains("did not find expected '-'") { Some("Start list items with '-' and ensure proper indentation.".into()) } else if lower.contains("expected ':'") { Some("Ensure each key is followed by ':' separating key and value.".into()) } else { None } }
61-79: Propagate filename and thread location into hints; tighten diagnostic construction.Name the
NamedSourcewith the actual manifest path for better UX in IDE/CI, and pass the location intohint_for. Also drop the now-removedlabelassignment.Apply:
-fn map_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { +fn map_yaml_error(err: &YamlError, src: &str, name: &str) -> anyhow::Error { let (line, col, span) = err.location().map_or((1, 1, None), |l| { (l.line(), l.column(), Some(to_span(src, l))) }); let err_str = err.to_string(); - let hint = hint_for(&err_str, src); + let hint = hint_for(&err_str, src, err.location()); let message = format!("YAML parse error at line {line}, column {col}: {err_str}"); let diag = YamlDiagnostic { - src: NamedSource::new("manifest.yml", src.to_string()), + src: NamedSource::new(name, src.to_string()), span, help: hint, source: Some(anyhow::Error::msg(err_str.clone())), message, - label: "parse error here".into(), }; anyhow::Error::new(diag) }
90-90: Thread a sensible name for in-memory manifests.Pass a default name so diagnostics aren’t stuck with a placeholder later.
Apply:
- let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml))?; + let mut doc: YamlValue = + serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml, "Netsukefile"))?;tests/yaml_error_tests.rs (1)
1-40: Add missing cases from prior suggestions.Add tests for:
- A list item missing a dash to verify the “missing '-'” hint.
- A generic parse error without a specific hint to ensure the baseline message remains informative.
I can draft these tests if you want them in this PR.
📜 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:
src/manifest.rstests/yaml_error_tests.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:
src/manifest.rstests/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
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
🧬 Code Graph Analysis (2)
src/manifest.rs (1)
tests/yaml_error_tests.rs (2)
err(8-9)err(26-27)
tests/yaml_error_tests.rs (1)
src/manifest.rs (1)
from_str(89-110)
🔍 Remote MCP Context7, Deepwiki
Relevant findings to aid PR review:
-
CI runs on PRs to main and executes (in order) make check-fmt, make lint, make test, and coverage steps; snapshot tests run with INSTA_UPDATE=no so snapshot mismatches fail CI.,
-
Make targets / commands used locally and in CI: make fmt / make check-fmt (cargo fmt), make lint (cargo clippy with -D warnings), make test (cargo test --all-targets --all-features), and cargo tarpaulin for coverage. Reviewer should run these locally before approving.
-
YAML-parsing–related tests live and run in:
- tests/ast_tests.rs (unit tests for NetsukeManifest deserialization)
- tests/features/manifest.feature (BDD scenarios via cucumber-rs)
- New tests in this PR: tests/yaml_error_tests.rs (validates line/column and hint text)
Failures in these files will cause the Test step to fail.
-
CI will surface any YAML-parsing failures with details in the Test job logs (line/column, message); snapshot mismatches are treated as failures in CI.,
Sources:,,
⏰ 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 (1)
tests/yaml_error_tests.rs (1)
1-3: Avoid coupling tests to the concrete diagnostic type.Relying on
YamlDiagnosticbeingpubties the external test crate to an internal type. Expose a small test-only helper that returnsmiette::Reportor re-export a trait object (pub type ManifestDiag = dyn Diagnostic;) to decouple, or keepYamlDiagnosticdocumented and stable.Do you want me to open a follow-up to add a
pub fn render_yaml_error(err: &anyhow::Error) -> Stringhelper behindcfg(test)so tests don’t need to downcast?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/yaml_error_tests.rs (2)
6-13: Add cases for “missing dash” and a generic parser error to cover the remaining hint paths.Extend the parameterised matrix to exercise:
- Missing dash for list items under a sequence key (e.g. targets).
- A generic YAML parse error without a tailored hint.
This aligns with the PR objectives and prior review feedback.
Example additions (adjust needles to your exact hint text):
#[rstest] #[case( "targets:\n\t- name: test\n", &["line 2, column 1", "Use spaces for indentation"], )] #[case( "targets:\n - name: hi\n command echo\n", &["line 3", "expected ':'", "Ensure each key is followed by ':'"], )] +#[case( + // Missing dash for list items: mapping where a sequence is expected + "targets:\n name: hi\n command: echo\n", + &[ + "targets", // context key + "list", // error context mentions list/sequence + "-", // hint mentions a dash + ], +)] +#[case( + // Generic malformed YAML to exercise the fallback diagnostic + "targets: [\n", + &[ + "line 1", // location + "parse error", // generic wording from parser path + ], +)] fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) {If you want, I can wire in exact substrings from the implemented hints to remove guesswork.
15-18: Remove the downcast; expose a parsing API that returns a miette report.Downcasting to a concrete diagnostic couples the test to internals and was previously flagged. Expose a helper that returns a
miette::Reportand render that directly.Apply in test:
- let err = manifest::from_str(yaml).expect_err("parse should fail"); - let diag = err - .downcast_ref::<manifest::YamlDiagnostic>() - .expect("diagnostic type"); + let report = manifest::from_str_report(yaml).unwrap_err();And update rendering (see next comment) to pass
&reportwhere&dyn Diagnosticis expected.Outside this file, add a test-only-friendly API in
src/manifest.rs:// New API preserving the existing public surface pub fn from_str_report(yaml: &str) -> miette::Result<NetsukeManifest> { from_str(yaml).map_err(|e| { // Preserve structured YAML diagnostic if present; otherwise wrap. if let Some(diag) = e.downcast_ref::<YamlDiagnostic>() { // Clone or re-create the diagnostic as needed miette::Report::new(diag.clone()) } else { miette::Report::msg(e.to_string()) } }) }If cloning the diagnostic isn’t available, return
miette::Report::new(map_yaml_error(...))by re-mapping from the originalserde_yml::Erroryou have at the parsing site.
📜 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 (1)
tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.rs
🧬 Code Graph Analysis (1)
tests/yaml_error_tests.rs (1)
src/manifest.rs (1)
from_str(89-110)
🔍 Remote MCP Context7
Findings relevant to reviewing dependency additions:
-
miette resolved to Context7 library ID /zkat/miette (description: "Fancy extension for std::error::Error with pretty, detailed diagnostic printing"; trust score 9.9). Recommended: dependency miette = { version = "5", features = ["fancy"] } matches a high-quality crate.,
-
serde_spanned did not produce a clear one-to-one Context7 library match in the search results (search returned general Serde/Serde-related entries and Miette again). Recommend verifying the exact crate name and source (crates.io repository or GitHub) for serde_spanned used in Cargo.toml to ensure it maps to the intended crate.,
Tools used: Context7 resolve-library-id (via parallel call).
🔇 Additional comments (1)
tests/yaml_error_tests.rs (1)
23-25: LGTM: concise substring assertions and helpful failure context.The loop-based asserts keep the test compact and the failure message is informative. Keep this pattern as additional cases are added.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/manifest.rs (5)
9-15: Imports are tight and follow crate guidelines.Keep
miette::{Diagnostic, NamedSource, SourceSpan}scoped;serde_spannedwas removed as advised previously.
30-44: Diagnostic struct is well-formed and concise.Struct-level
#[diagnostic(code(...))], static#[label("…")], and#[doc(hidden)]are all correctly applied. This enables precise spans and stable rendering.
46-76: Hint generation is scoped to the indentation run.Limit the “tabs” hint to the leading whitespace of the faulting line; this avoids false positives in other parts of the file. Good.
78-96: Propagate filename and precise location in the user-facing message.The message embeds line/column and the NamedSource uses the actual
name. This improves IDE/CI annotations.
333-338: Include the actual path in diagnostics.Passing
path_ref.display().to_string()improves UX in reports and aligns with the testing strategy.tests/yaml_error_tests.rs (1)
1-3: Add a module-level inner doc comment per test guidelines.Document the test module’s purpose at the top.
+//! Tests for YAML diagnostics: validates spans, labels, and actionable hints. use miette::GraphicalReportHandler; use netsuke::manifest; use rstest::rstest;
📜 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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)docs/netsuke-design.md(3 hunks)src/manifest.rs(4 hunks)tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges and keep dependencies up to date
Mandate SemVer caret requirements for all dependencies (e.g., "1.2.3")
Prohibit wildcard (*) and open-ended (>=) version specifiers; use ~ only with a specific, documented reason
Files:
Cargo.toml
**/*.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
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Treat docs/ markdown as the reference source of truth for requirements and decisions; keep updated with changes
Use en-GB-oxendict spelling/grammar in documentation (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Wrap Markdown paragraphs and bullet points at 80 columns; do not wrap tables or headings
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Validate Mermaid diagrams in Markdown using make nixie
Files:
docs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
🧬 Code Graph Analysis (2)
tests/yaml_error_tests.rs (1)
src/manifest.rs (1)
from_str(138-140)
src/manifest.rs (1)
tests/yaml_error_tests.rs (1)
err(30-31)
🔍 Remote MCP Context7
Findings (concise)
-
miette: resolves to Context7 library ID /zkat/miette — "Fancy extension for std::error::Error with pretty, detailed diagnostic printing", high trust score (9.9) and many code snippets (28). This aligns with the PR's added dependency (miette) and supports that it's a well-known crate for pretty diagnostics.,
-
serde_spanned: resolve returned no clear one-to-one match for a crate named serde_spanned; results instead surfaced general Serde entries (e.g., /serde-rs/serde, /websites/rs-crate-serde_yml). Recommendation: verify the exact crate name and source (crates.io/GitHub) and ensure the Cargo.toml entry matches the intended published crate.,
Tools used: Context7 resolve-library-id (via parallel call).,
⏰ 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 (4)
docs/netsuke-design.md (2)
1272-1282: Good addition: clarify the role of miette in the error stack.The section accurately frames miette as the user-facing diagnostics layer and stays consistent with the module changes.
1570-1577: Table update aligns with the new diagnostics stack.The “Error Handling” row now reflects anyhow + thiserror + miette, matching the implementation.
src/manifest.rs (2)
106-128: Thread diagnostics through parsing without sacrificing API stability.
from_str_namedcleanly maps serde_yml errors to typed diagnostics and leaves the public signature unchanged.
130-141: Default source name is sensible.Using "Netsukefile" as the default display name yields readable diagnostics for in-memory parsing.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
6-24: Remove serde_spanned references
No usage ofserde_spannedfound and it is not declared in Cargo.toml. Remove all mentions of serde_spanned integration from the PR description or accompanying documentation.
- Delete serde_spanned references in the PR description or docs
♻️ Duplicate comments (2)
src/manifest.rs (1)
17-27: Fix SourceSpan length type conversion to avoid API breakagePass the length as the expected offset type. This was previously flagged; re-apply the fix to keep compatibility across miette versions.
Apply:
- let len = end.saturating_sub(at); - SourceSpan::new(at.into(), len) + let len = end.saturating_sub(at); + SourceSpan::new(at.into(), len.into())tests/yaml_error_tests.rs (1)
15-37: Parameterise YAML error cases (resolved as per prior review).The rstest cases remove duplication and cover tabs, missing colons, missing dashes, and a generic parse error. This addresses earlier feedback to add a no-hint failure case.
📜 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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(2 hunks)src/manifest.rs(11 hunks)src/runner.rs(2 hunks)tests/ast_tests.rs(1 hunks)tests/runner_tests.rs(1 hunks)tests/steps/ir_steps.rs(1 hunks)tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/runner_tests.rstests/ast_tests.rstests/steps/ir_steps.rstests/yaml_error_tests.rssrc/runner.rssrc/manifest.rs
🔍 Remote MCP Context7
Summary of additional relevant facts for reviewing the PR
-
miette is a known crate; Context7 match: /zkat/miette (description: "Fancy extension for std::error::Error with pretty, detailed diagnostic printing"; trust score 9.9). This aligns with the PR adding miette for rich diagnostics.
-
serde_spanned did not resolve to a clear, single Context7 library match (search returned general Serde-related results but no definitive "serde_spanned" entry). Recommend verifying the exact crate name/source (crates.io or GitHub) and that Cargo.toml uses the intended published crate.
-
strip-ansi-escapes did not return a clear Context7 match in the parallel search output. Confirm the dev-dependency name/version and source (crates.io) to ensure test normalization uses the intended crate.
Tools used
- Context7 resolve-library-id (parallel calls for "miette", "serde_spanned", "strip-ansi-escapes") — functions.resolve-library-id invoked via multi_tool_use.parallel.
🔇 Additional comments (9)
src/manifest.rs (3)
45-75: LGTM: Scope the “tabs” hint to indentation and add targeted hintsThe heuristics correctly probe indentation on the error line for tabs and add specific fix-ups for missing dashes and colons. Keep it.
77-95: LGTM: Map YAML errors to rich miette diagnosticsThe diagnostic includes a NamedSource, narrow span, human message, and hint. This aligns with the PR objective and integrates cleanly with miette reporters.
124-126: LGTM: Preserve “manifest parse error” context on deserialisationWrapping the deserialisation step with a stable context string keeps tests predictable while retaining diagnostic detail underneath.
tests/ast_tests.rs (1)
3-3: LGTM: Shift tests to miette::ResultAlign the helper with the library’s diagnostic type to propagate structured errors through tests.
Also applies to: 9-11
tests/yaml_error_tests.rs (3)
1-5: Document the test module succinctly (LGTM).Keep the inner doc comment. It satisfies the module-doc requirement and explains scope clearly.
11-13: Normalise ANSI escapes efficiently (LGTM).Use of strip-ansi-escapes is appropriate and keeps assertions stable across environments.
38-44: Ignore the Diagnostic type-mismatch suggestionThe test’s
erris amiette::Report(the error type alias frommiette::Result) soerr.as_ref()yields&Report, which implementsDiagnostic. The existing call torender_report(&mut msg, err.as_ref())compiles and works correctly. No change required.Likely an incorrect or invalid review comment.
Cargo.toml (2)
14-14: Enable rich diagnostics (LGTM).miette with the fancy feature is appropriate for graphical diagnostics and aligns with the tests.
65-76: Stabilise test output (LGTM).strip-ansi-escapes as a dev-dependency is a sound choice for normalising rendered diagnostics in tests.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/manifest.rs (1)
271-296: DRY up repeated render-and-wrap patternsExtract a tiny helper for the “render string with context + into_diagnostic + wrap_err” sequence to reduce repetition across rule/target/vars/string-or-list rendering.
Example helper (outside selected range):
fn render_str_with<E: minijinja::value::StructObject + Clone>( env: &Environment, tpl: &str, ctx: &E, what: &str, ) -> miette::Result<String> { env.render_str(tpl, ctx) .into_diagnostic() .wrap_err(what) }Then call with descriptive labels like "render rule command", "render target script", etc.
Also applies to: 297-319, 334-353
♻️ Duplicate comments (2)
src/manifest.rs (1)
17-35: Fix potential type mismatch inSourceSpan::newlength parameterConvert the computed length to the expected type to avoid API drift across miette versions and ensure spans render reliably.
let len = end.saturating_sub(start); - SourceSpan::new(start.into(), len) + SourceSpan::new(start.into(), len.into())tests/runner_tests.rs (1)
70-76: Assert against the error chain instead of only the immediate sourceAvoid coupling to the immediate
source()layer; assert over the full chain to remain resilient to future layering changes.- let err = run(&cli).expect_err("should have error"); - assert!(err.to_string().contains("loading manifest at")); - let source = err.source().expect("source error").to_string(); - assert!( - source.contains("manifest parse error"), - "expected parse error in source, got: {source}" - ); + let err = run(&cli).expect_err("should have error"); + assert!(err.to_string().contains("loading manifest at")); + let chain: Vec<String> = err.chain().map(|e| e.to_string()).collect(); + assert!( + chain.iter().any(|s| s.contains("manifest parse error")), + "expected error chain to include 'manifest parse error', got: {chain:?}" + );
📜 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 (4)
src/manifest.rs(11 hunks)src/runner.rs(5 hunks)tests/runner_tests.rs(1 hunks)tests/steps/ir_steps.rs(2 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/steps/ir_steps.rstests/runner_tests.rssrc/manifest.rssrc/runner.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/steps/ir_steps.rstests/runner_tests.rssrc/manifest.rssrc/runner.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/steps/ir_steps.rstests/runner_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.rssrc/runner.rs
🧬 Code Graph Analysis (3)
tests/steps/ir_steps.rs (1)
src/ir.rs (1)
from_manifest(117-128)
tests/runner_tests.rs (1)
src/runner.rs (1)
run(116-137)
src/runner.rs (1)
src/ir.rs (1)
from_manifest(117-128)
🔍 Remote MCP Context7
Summary of additional facts useful for review
-
I attempted to resolve the added dependencies (miette, serde_spanned, strip-ansi-escapes) against the Context7 library index to confirm canonical crate IDs and metadata. The parallel lookup completed but did not return clear, authoritative entries for those three crates (search results were broad/unrelated). Recommend verifying the exact crate names/sources (crates.io / GitHub) and versions used in Cargo.toml before merging.
-
Practical notes for reviewers (from PR context + test changes):
- Tests rely on strip-ansi-escapes to normalise GraphicalReportHandler output — ensure the dev-dependency name/version matches crates.io and compiles on CI.
- serde_spanned is referenced in docs/design; confirm the crate exists and is the intended crate (some similarly named crates exist; verify API compatibility).
- miette is a known diagnostics crate and is appropriate for the goal, but verify the Cargo.toml entry includes any required features and that code imports match the published API.
Tools used
- Context7 resolve-library-id (parallel attempt to resolve miette, serde_spanned, strip-ansi-escapes) —
🔇 Additional comments (7)
src/manifest.rs (4)
37-50: Good: Internal diagnostic type with precise span and stable codeKeep
YamlDiagnosticcrate-visible and annotated with a stable diagnostic code and a static label. This aligns with the goal of structured, inspectable diagnostics.
52-82: Good: Narrow hints to indentation context; guard against noisy suggestionsThe indentation probe constrained to leading whitespace mitigates false positives. Keep the hint strings short and actionable.
84-102: Good: Name the source and report line/column for actionable UXPropagate the filename via
NamedSource, compute a narrowSourceSpan, and include line/column in the message. This will render well withGraphicalReportHandler.
363-366: Good: Preserve IO context and thread source name into diagnosticsWrap the read error with file path context and delegate to
from_str_namedwith the actual filename to improve traceability.src/runner.rs (3)
9-9: Good: Use miette’sResult,IntoDiagnostic, andContextconsistentlyAdopt miette end-to-end so caller-facing errors carry rich context and are renderable with handlers.
177-179: Good: Convert subprocess errors into diagnostics with contextConvert
io::Resultto a diagnostic and attach a clear “run ninja” context. This maintains the causal chain while aligning with the new diagnostics strategy.
257-264: Good: Preserve manifest parse diagnostics and add high-level contextUse
with_contextfor the “loading manifest at …” banner and keep serialisation/IR generation errors wrapped with concise labels.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider extracting the repeated
.into_diagnostic().wrap_err*error‐wrapping pattern into a small helper or macro to cut down on boilerplate and ensure consistency. - The
hint_forfunction’s branching logic is growing—consider refactoring it into smaller methods or a pattern‐matching table of error substrings to keep it maintainable as more hints are added. - Add a test for the scenario where a
YamlErrorhas noLocationto verify that the default span and message fallback (line 1, column 1) behaves correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the repeated `.into_diagnostic().wrap_err*` error‐wrapping pattern into a small helper or macro to cut down on boilerplate and ensure consistency.
- The `hint_for` function’s branching logic is growing—consider refactoring it into smaller methods or a pattern‐matching table of error substrings to keep it maintainable as more hints are added.
- Add a test for the scenario where a `YamlError` has no `Location` to verify that the default span and message fallback (line 1, column 1) behaves correctly.
## Individual Comments
### Comment 1
<location> `tests/yaml_error_tests.rs:38` </location>
<code_context>
+ "targets:\n - name: 'unterminated\n",
+ &["YAML parse error", "line 2"],
+)]
+fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) {
+ let err = manifest::from_str(yaml).expect_err("parse should fail");
+ let mut msg = String::new();
+ GraphicalReportHandler::new()
+ .render_report(&mut msg, err.as_ref())
+ .expect("render yaml error");
+ let msg = normalise_report(&msg);
+ for needle in needles {
+ assert!(msg.contains(needle), "missing: {needle}\nmessage: {msg}");
+ }
</code_context>
<issue_to_address>
Consider adding tests for empty YAML input and completely invalid YAML.
Tests currently focus on specific syntax errors but lack coverage for empty or malformed YAML inputs. Including these cases will improve error handling robustness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[case(
"targets:\n - name: 'unterminated\n",
&["YAML parse error", "line 2"],
)]
=======
#[case(
"targets:\n - name: 'unterminated\n",
&["YAML parse error", "line 2"],
)]
#[case(
"",
&["YAML parse error", "empty", "no content"],
)]
#[case(
"not: yaml: at all: %$#@!",
&["YAML parse error", "invalid", "could not parse"],
)]
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/manifest.rs:133` </location>
<code_context>
- let manifest: NetsukeManifest = serde_yml::from_value(doc).context(ERR_MANIFEST_PARSE)?;
+ let manifest: NetsukeManifest = serde_yml::from_value(doc)
+ .into_diagnostic()
+ .wrap_err(ERR_MANIFEST_PARSE)?;
</code_context>
<issue_to_address>
Refactor repeated error conversion and context wrapping to avoid duplication.
There are multiple instances of .into_diagnostic().wrap_err() and similar error conversion patterns. Consider refactoring these into a helper or macro to keep the code DRY and maintain readability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlintcloses #49
https://chatgpt.com/codex/tasks/task_e_68a2c9e1467483229721c7ed29af854b
Summary by Sourcery
Introduce miette-powered diagnostics for YAML parsing and migrate existing error handling to miette across the codebase
New Features:
Enhancements:
Build:
Documentation:
Tests: