Enforce span validation at runtime across all builds

[leynos]

Summary

• Replaces debug-only span validation with unconditional runtime validation across all builds.
• ParsedSpans::build() now returns a Result<ParsedSpans, SpanListValidationError> instead of panicking; callers can inspect per-list issues via the new error type.
• Introduced SpanListIssue and SpanListValidationError to report per-list issues and aggregate errors.
• Added internal helper enforce_valid_span_lists(...) used by the build path.
• Documentation updated to reflect that spans are sorted and non-overlapping across all builds.

Changes

Core Functionality

• In src/parser/cst_builder/spans.rs, replace the previous debug-only validation with unconditional runtime validation:
  • ParsedSpansBuilder::build(self) now validates and returns Ok(ParsedSpans) or Err(SpanListValidationError).
  • Added SpanListIssue and SpanListValidationError to report per-list issues and aggregate errors.
  • Introduced internal helper enforce_valid_span_lists(...) used by the build path.
  • ParsedSpans construction now relies on validated inputs.
• Updated tests to reflect the always-on validation and error reporting path.

Documentation

• Update module and struct doc comments to state that spans are sorted and non-overlapping in all builds (not just debug builds).

Tests

• Added src/parser/cst_builder/spans/tests.rs with tests for runtime validation behavior:
  • Valid spans pass through validation.
  • Invalid spans produce a SpanListValidationError with per-list details.
  • Validation reports the correct list name when failures occur.
• Updated existing tests to reflect the new error-path semantics.

Public API

• ParsedSpans::builder().<list>(...)... .build() now returns Result<ParsedSpans, SpanListValidationError> instead of panicking via debug assertions.
• New error types: SpanListIssue and SpanListValidationError expose per-list details.

Test plan

• Build and run tests in both debug and release configurations:
  • cargo test
  • cargo test --release
• Validate that a scenario with overlapping or unsorted spans results in a SpanListValidationError containing per-list details in all builds.
• Ensure valid inputs continue to succeed.

🌿 Generated by Terry

---

ℹ️ Tag @terragon-labs to ask questions and address PR feedback

📎 Task: https://www.terragonlabs.com/task/6f6dec64-e1fc-4e31-95ca-1e3497b2e7d5

Summary by Sourcery

Enforce span list validation at runtime so invalid spans surface as errors in all build modes.

Enhancements:

• Replace debug-only span list assertions with unconditional runtime validation that reports per-list issues.
• Clarify documentation to state that parsed span lists are always sorted and non-overlapping across all builds.

[coderabbitai]

Summary by CodeRabbit

• New Features

  • Builder now returns explicit validation errors for span ordering (recoverable Result) instead of panicking.

• Improvements

  • Parsing validates named span lists before construction and reports multiple, labelled issues; error paths now surface validation messages and fall back gracefully.

• Tests

  • Added unit and integration tests covering valid/invalid span ordering and error messages, including public API behaviour.

• Chores

  • Test utilities extended to assert on panic messages and re-exported for tests.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

Summarise the new explicit span-list validation: introduce SpanListIssue and SpanListValidationError; make ParsedSpansBuilder::build() return Result<ParsedSpans, SpanListValidationError>; enforce validation via enforce_valid_span_lists() before construction; add unit tests and a test helper to capture panic messages.

Changes

Cohort / File(s)	Summary
Span validation types & builder src/parser/cst_builder/spans.rs	Add SpanListIssue (list, prev, next) with accessors and Display. Add SpanListValidationError (message, issues) with new and issues(). Change ParsedSpansBuilder::build() to return Result<ParsedSpans, SpanListValidationError> and enforce validation via enforce_valid_span_lists() (remove debug-only assert path). Update top-level docs accordingly.
Test utilities src/test_util/assertions.rs, src/test_util/mod.rs	Add pub fn assert_panic_with_message<F>(f: F) -> String to run code that must panic and capture its message; re-export it from src/test_util/mod.rs.
Span validation tests src/parser/cst_builder/spans/tests.rs, tests/span_validation.rs	Add and extend unit tests for span validation and public API: check sorted/overlap/unsorted cases; assert build() returns Err with descriptive messages for invalid lists; assert multi-issue reporting; use assert_panic_with_message where relevant.
Builder usage in tests / scanner src/parser/cst_builder/tree.rs, src/parser/span_scanner.rs	Update tests and span scanner to handle ParsedSpans::builder().build() returning Result: match on Ok/Err, push validation error into scanner errors and fall back to defaults when build fails.

Sequence Diagram(s)

(Skipped — changes do not introduce a multi-component sequential control flow warranting a diagram.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related issues

• Consider enforcing span validation in release builds #80: Enforce span-list validation in non-debug builds by returning an explicit SpanListValidationError from the builder — matches this PR's change to make validation a fallible public build path.

Poem

Where debug checks once hid from sight,
Raise errors clear and name each plight.
Collect the issues, list each span,
Build returns truth — reveal the plan,
Sorted lines keep parsing right.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: enforcing span validation at runtime across all builds, replacing debug-only validation with unconditional validation.
Description check	✅ Passed	The description comprehensively covers the changeset, explaining the validation mechanism shift, new error types, affected files, and test coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch terragon/enforce-span-validation-release-zd0r7f

---

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b5d55c and a1d85af.

📒 Files selected for processing (7)

• src/parser/cst_builder/spans.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/cst_builder/tree.rs
• src/parser/span_scanner.rs
• src/test_util/assertions.rs
• src/test_util/mod.rs
• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• src/test_util/mod.rs
• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs
• src/parser/span_scanner.rs
• src/test_util/assertions.rs
• src/parser/cst_builder/tree.rs
• src/parser/cst_builder/spans.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• src/test_util/mod.rs
• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs
• src/parser/span_scanner.rs
• src/test_util/assertions.rs
• src/parser/cst_builder/tree.rs
• src/parser/cst_builder/spans.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• src/test_util/mod.rs
• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs
• src/parser/span_scanner.rs
• src/test_util/assertions.rs
• src/parser/cst_builder/tree.rs
• src/parser/cst_builder/spans.rs

🧬 Code graph analysis (5)

src/test_util/mod.rs (1)

src/test_util/assertions.rs (3)

• assert_no_parse_errors (53-55)
• assert_panic_with_message (24-38)
• assert_parse_error (75-94)

src/parser/cst_builder/spans/tests.rs (1)

src/parser/cst_builder/spans.rs (10)

• validate_spans_sorted (299-310)
• new (20-26)
• new (68-75)
• imports (125-128)
• imports (234-236)
• validate_span_lists_sorted (318-331)
• list (30-32)
• builder (226-228)
• typedefs (132-135)
• typedefs (240-242)

tests/span_validation.rs (3)

src/parser/cst_builder/mod.rs (2)

• errors (51-53)
• root (45-47)

src/parser/cst_builder/spans.rs (3)

• imports (125-128)
• imports (234-236)
• builder (226-228)

src/test_util/assertions.rs (2)

• err (32-32)
• err (34-34)

src/parser/span_scanner.rs (1)

src/parser/cst_builder/spans.rs (1)

• builder (226-228)

src/parser/cst_builder/tree.rs (1)

src/parser/cst_builder/spans.rs (1)

• builder (226-228)

🔍 Remote MCP Deepwiki

Relevant facts discovered for reviewing this PR

• ParsedSpans are consumed by CST construction (build_green_tree) and SpanCursors; the CST builder and downstream AST wrappers assume span lists are sorted and non‑overlapping — invalid spans can change CST structure or cause panics during CST construction or later AST access. (Core Parsing Pipeline; Parser Implementation Details)

• The builder path (ParsedSpans::builder().build()) is the canonical place where span lists are validated before CST construction; previously validation ran only under debug assertions. Making build() fail/return an error therefore changes where failures surface in the main parse→CST pipeline. (Core Parsing Pipeline; Parser Implementation Details)

• The parser pipeline collects spans in multiple named lists (imports, typedefs, relations, indexes, functions, transformers, rules, expressions) and uses those lists in CST construction; error aggregation from span collection is part of parse_tokens → Parsed result. Any change in validation/reporting at the ParsedSpans builder affects the overall error flow. (Core Parsing Pipeline; Parser Implementation Details)

• Tests and CI run both debug and release builds; the repository’s testing strategy emphasizes running cargo test and cargo test --release, and the Makefile/CI treats clippy warnings as errors. Thus behavior that panics unconditionally in release builds may break CI/tests or change test expectations. (Testing Strategy; Development Environment)

• The project’s error‑recovery philosophy favors collecting parse errors and continuing rather than panicking; changing the public ParsedSpans builder to panic (or to return an error type) is a behavioral/API contract change that impacts callers (e.g., span_scanner, build_green_tree) and the recovery model. (Parser Implementation Details; Testing Strategy)

Sources consulted: Deepwiki repository documentation pages (Core Parsing Pipeline; Parser Implementation Details; Testing Strategy; Development Environment).

⏰ 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 (18)

src/parser/cst_builder/spans.rs (5)

11-55: LGTM! Well-designed error detail type.

The SpanListIssue struct properly encapsulates validation failure details with clean accessors and a helpful Display implementation. The private constructor ensures controlled creation via the validation path.

---

57-82: LGTM! Proper error type implementation.

SpanListValidationError correctly uses thiserror::Error for semantic error handling and provides public access to both the formatted message and structured issue list. The constructor properly aggregates multiple issues into a single error.

---

179-183: LGTM! Correct Result-based validation.

The build() method correctly returns Result<ParsedSpans, SpanListValidationError> as per the PR objectives, replacing the previous debug-only validation with runtime checks in all builds. The use of the ? operator is idiomatic and clear.

---

185-220: LGTM! Clean helper methods.

The private helpers span_lists() and into_parsed_spans() provide good encapsulation. The array of named lists centralises validation input, and into_parsed_spans() ensures ParsedSpans is only constructed after successful validation.

---

312-331: LGTM! Validation logic correctly aggregates errors.

The enforce_valid_span_lists() wrapper maintains explicit intent at call sites, and the comment explains the extensibility reasoning (addressing the previous review comment). The validate_span_lists_sorted() function properly collects all issues before returning, enabling callers to see all validation failures at once.

src/parser/span_scanner.rs (1)

54-73: LGTM! Correct error-recovery implementation.

The error handling properly converts span validation failures into parse errors and continues with default spans, aligning with the project's error-recovery philosophy. The use of a 0..0 span for this meta-error is reasonable, as span validation failures aren't tied to a specific token position.

The fallback to ParsedSpans::default() is safe—downstream code receives empty span lists rather than invalid data, preventing potential CST construction issues.

Based on learnings, ensure you run cargo fmt before committing to avoid CI formatting failures.

src/test_util/mod.rs (1)

12-12: LGTM! Clean test utility export.

The addition of assert_panic_with_message to the public test utilities properly exposes the new helper for validating panic payloads in tests.

tests/span_validation.rs (2)

5-10: LGTM! Good happy-path coverage.

The test correctly verifies that valid input produces no errors and properly captures span information.

---

12-23: LGTM! Proper error-path testing.

The test correctly uses .expect_err() as per coding guidelines and validates that the public API returns meaningful errors for invalid span lists. The #[expect] annotation is appropriately scoped with a clear reason.

src/test_util/assertions.rs (1)

8-38: LGTM! Robust panic assertion helper.

The assert_panic_with_message implementation correctly captures panic payloads and handles both String and &str types. The explicit panic for unknown payload types (lines 35-37) properly addresses the previous review concern about silent failures with .unwrap_or_default().

The documentation includes examples, panic conditions are documented, and #[track_caller] ensures useful error locations.

src/parser/cst_builder/spans/tests.rs (7)

1-4: LGTM!

Module-level documentation present and imports are appropriate. The use of super::* provides access to the parent module's items for testing internal validation functions.

---

6-38: LGTM!

Comprehensive coverage of validate_spans_sorted edge cases: overlapping spans, unsorted spans, empty list, single element, and properly sorted spans. The #[expect] attribute on the single-span test is correctly scoped with a clear reason.

---

40-51: LGTM!

Uses .expect_err() and .expect() as per coding guidelines. Validates that error reporting correctly identifies the list name, which is essential for debugging validation failures in production.

---

53-65: LGTM!

Good positive test case verifying the builder accepts sorted spans and stores them correctly. Uses .expect() appropriately with the required lint expectation.

---

67-76: LGTM!

Good extraction of a reusable test helper that centralises the build error assertion. This reduces duplication in the parameterised tests and keeps the lint expectation tightly scoped.

---

78-84: LGTM!

Good use of rstest parameterisation to cover both unsorted and overlapping span scenarios without code duplication. The helper function handles the lint expectation appropriately.

---

86-102: LGTM!

Important test verifying that validation errors are aggregated across multiple span lists rather than short-circuiting on the first failure. This aligns with the project's error-recovery philosophy of collecting errors rather than failing early.

src/parser/cst_builder/tree.rs (1)

195-198: Use .expect() instead of manual match with panic.

The manual match with panic! is verbose. Use .expect() for consistency with the coding guidelines and other tests in this module.

🔎 Proposed fix

-        let spans = match ParsedSpans::builder().build() {
-            Ok(spans) => spans,
-            Err(err) => panic!("expected valid spans, got: {err}"),
-        };
+        let spans = ParsedSpans::builder()
+            .build()
+            .expect("expected valid spans");

Likely an incorrect or invalid review comment.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Enforces span list validation at runtime for all build modes by turning a debug-only assertion into a panic-on-error check and aligning the span documentation with this guarantee.

Sequence diagram for runtime span validation in ParsedSpansBuilder

sequenceDiagram
    participant ParsedSpansBuilder
    participant validate_span_lists_sorted
    participant ParsedSpans

    ParsedSpansBuilder->>ParsedSpansBuilder: build
    ParsedSpansBuilder->>validate_span_lists_sorted: validate_span_lists_sorted(span_lists)
    alt spans valid
        validate_span_lists_sorted-->>ParsedSpansBuilder: Ok
        ParsedSpansBuilder->>ParsedSpansBuilder: construct ParsedSpans fields
        ParsedSpansBuilder-->>ParsedSpans: create_instance
        ParsedSpans-->>ParsedSpansBuilder: ParsedSpans
    else spans invalid
        validate_span_lists_sorted-->>ParsedSpansBuilder: Err(message)
        ParsedSpansBuilder->>ParsedSpansBuilder: panic(message)
    end

Loading

Class diagram for ParsedSpans and ParsedSpansBuilder span validation

classDiagram
    class ParsedSpans {
        +Vec~Span~ imports
        +Vec~Span~ typedefs
        +Vec~Span~ relations
        +Vec~Span~ sources
        +Vec~Span~ transformers
        +Vec~Span~ rules
        +Vec~Span~ expressions
    }

    class ParsedSpansBuilder {
        +Vec~Span~ imports
        +Vec~Span~ typedefs
        +Vec~Span~ relations
        +Vec~Span~ sources
        +Vec~Span~ transformers
        +Vec~Span~ rules
        +Vec~Span~ expressions
        +build() ParsedSpans
    }

    class validate_span_lists_sorted {
        +validate_span_lists_sorted(span_lists) Result
    }

    ParsedSpansBuilder --> ParsedSpans : builds
    ParsedSpansBuilder ..> validate_span_lists_sorted : calls for runtime validation
    ParsedSpans ..> Span : uses
    ParsedSpansBuilder ..> Span : uses

Loading

File-Level Changes

Change	Details	Files
Enforce span list validation at runtime instead of only in debug builds.	Replace the previous debug_assert-based validation of span lists with a runtime check that panics when validate_span_lists_sorted(...) returns an error message. Ensure that invalid, overlapping, or unsorted span lists will cause the process to crash with a clear panic message in all build configurations, including release builds.	src/parser/cst_builder/spans.rs
Align documentation with the new span invariants across all build modes.	Update module-level documentation comments to state that span lists are always sorted and free from overlaps, without scoping this guarantee to debug builds. Update ParsedSpans struct documentation to reflect that the builder enforces sorted, non-overlapping span lists in all builds.	src/parser/cst_builder/spans.rs

Possibly linked issues

• #Consider enforcing span validation in release builds: PR replaces debug-only span validation with runtime panics in all builds, exactly fulfilling the issue request.
• Refactor CST construction into module #79: The PR implements the issue’s request by enforcing validate_span_lists_sorted with a panic in release builds.
• #(no explicit number provided): PR converts debug-only span checks into runtime panics and updates docs, directly addressing the span validation issue.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Since this changes invalid spans from a debug-only failure to an unconditional panic, consider whether ParsedSpansBuilder::build (or equivalent) should return a Result instead so callers can handle invalid spans without bringing down the process in release builds.
• If you do want to keep a hard failure, you might retain the debug_assert! in addition to the panic or wrap the validation in a clearly named helper (e.g. enforce_valid_spans(...)) so the intent and failure mode are more explicit at the call site.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Since this changes invalid spans from a debug-only failure to an unconditional panic, consider whether `ParsedSpansBuilder::build` (or equivalent) should return a `Result` instead so callers can handle invalid spans without bringing down the process in release builds.
- If you do want to keep a hard failure, you might retain the `debug_assert!` in addition to the panic or wrap the validation in a clearly named helper (e.g. `enforce_valid_spans(...)`) so the intent and failure mode are more explicit at the call site.

## Individual Comments

### Comment 1
<location> `src/parser/cst_builder/spans.rs:120` </location>
<code_context>
         } = self;

-        let result = validate_span_lists_sorted(&[
+        if let Err(message) = validate_span_lists_sorted(&[
             ("imports", &imports),
             ("typedefs", &typedefs),
</code_context>

<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests that cover the now-always-on span validation (panic on error), since this is a behavior change that must be backed by tests.

Treat this as a behavior change: previously the span list validation only fired in debug builds via `debug_assert!`, while now it panics unconditionally and can affect release behavior. Add tests that:

- Exercise `ParsedSpans::builder` with valid, sorted, non-overlapping spans and assert the builder succeeds.
- Exercise cases with unsorted and overlapping spans and assert that the builder panics (or returns an error, depending on how you want to expose this contract) in both debug and release configurations.

Ensure you add both higher-level behavioural tests (covering the parsing/building flow that relies on `ParsedSpans`) and lower-level unit tests specifically targeting `validate_span_lists_sorted`/`ParsedSpansBuilder::build`. This satisfies the requirement that all feature/behavior changes and bug fixes be demonstrated by tests.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*`

**Instructions:**
- For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
- Bug fixes *must* be demonstrated by a test.

</details>
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[leynos]

@coderabbitai Have the following now been resolved?

Please address the comments from this code review:

## Overall Comments
- Since this changes invalid spans from a debug-only failure to an unconditional panic, consider whether `ParsedSpansBuilder::build` (or equivalent) should return a `Result` instead so callers can handle invalid spans without bringing down the process in release builds.
- If you do want to keep a hard failure, you might retain the `debug_assert!` in addition to the panic or wrap the validation in a clearly named helper (e.g. `enforce_valid_spans(...)`) so the intent and failure mode are more explicit at the call site.

## Individual Comments

### Comment 1
<location> `src/parser/cst_builder/spans.rs:120` </location>
<code_context>
         } = self;

-        let result = validate_span_lists_sorted(&[
+        if let Err(message) = validate_span_lists_sorted(&[
             ("imports", &imports),
             ("typedefs", &typedefs),
</code_context>

<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests that cover the now-always-on span validation (panic on error), since this is a behavior change that must be backed by tests.

Treat this as a behavior change: previously the span list validation only fired in debug builds via `debug_assert!`, while now it panics unconditionally and can affect release behavior. Add tests that:

- Exercise `ParsedSpans::builder` with valid, sorted, non-overlapping spans and assert the builder succeeds.
- Exercise cases with unsorted and overlapping spans and assert that the builder panics (or returns an error, depending on how you want to expose this contract) in both debug and release configurations.

Ensure you add both higher-level behavioural tests (covering the parsing/building flow that relies on `ParsedSpans`) and lower-level unit tests specifically targeting `validate_span_lists_sorted`/`ParsedSpansBuilder::build`. This satisfies the requirement that all feature/behavior changes and bug fixes be demonstrated by tests.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*`

**Instructions:**
- For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
- Bug fixes *must* be demonstrated by a test.

</details>
</issue_to_address>

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the current implementation of spans.rs
cat src/parser/cst_builder/spans.rs

Length of output: 8235

---

🏁 Script executed:

#!/bin/bash
# Search for test files and test functions related to span validation
rg -n "test.*span" --type rust -C 3

Length of output: 2467

---

🏁 Script executed:

#!/bin/bash
# Look for tests that might be testing ParsedSpansBuilder or validate_span_lists_sorted
rg -n "ParsedSpansBuilder|validate_span_lists_sorted" --type rust -C 3

Length of output: 2296

---

🏁 Script executed:

#!/bin/bash
# Search for panic test attributes that might be testing the validation behavior
rg -n "#\[should_panic\]" --type rust -A 5

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check how ParsedSpans is actually used in the parser to understand the full flow
rg -n "ParsedSpans::builder" --type rust -C 5

Length of output: 2792

---

🏁 Script executed:

#!/bin/bash
# Look for integration/behavioral tests that might test the parser flow
fd -e rs test | head -20

Length of output: 208

---

🏁 Script executed:

#!/bin/bash
# Check the cst_builder module to understand the flow
cat src/parser/cst_builder/mod.rs

Length of output: 1079

---

🏁 Script executed:

#!/bin/bash
# Check span_scanners tests to see what they cover
cat src/parser/span_scanners/tests.rs | head -100

Length of output: 3043

---

🏁 Script executed:

#!/bin/bash
# Check if there are any tests that parse full programs (behavioral tests)
rg -n "parse.*\(" --type rust tests/ -C 2 | head -50

Length of output: 3125

---

🏁 Script executed:

#!/bin/bash
# Look for tests that would exercise the full parsing pipeline including span validation
rg -n "fn test_|fn.*_test\(" src/parser/tests/ --type rust -A 10 | head -100

Length of output: 0

---

@leynos: Let me assess the resolution status of those review comments:

Test Coverage ✅ RESOLVED

The test requirements have been fully addressed:

Unit tests (in src/parser/cst_builder/spans.rs):

• validate_spans_sorted_err_on_overlap: Tests overlapping spans detection
• validate_spans_sorted_err_on_unsorted: Tests unsorted spans detection
• validate_spans_sorted_ok_on_empty: Tests empty span list
• validate_spans_sorted_ok_on_single: Tests single span
• validate_spans_sorted_ok_on_sorted: Tests valid sorted spans
• builder_panics_on_unsorted: Tests builder panic with invalid imports
• builder_reports_all_errors: Tests reporting of multiple validation errors

Behavioural tests: Existing integration tests in tests/rule_behaviour.rs (e.g., parse_and_extract_rule()) exercise the full parsing pipeline, which calls through span_scanner.rs → ParsedSpans::builder().build(), thereby testing the validation in a real-world context.

The tests use std::panic::catch_unwind rather than #[should_panic], which is a superior approach for verifying panic messages.

API Design Concerns ⚠️ NOT ADDRESSED

The original review raised two API design suggestions that remain unaddressed:

1. Return Result instead of panic: The build() method still panics unconditionally rather than returning Result<ParsedSpans, String> to allow callers to handle validation failures gracefully.

2. Explicit naming: No wrapper function like enforce_valid_spans(...) was added to make the failure mode more explicit at the call site.

The current implementation directly calls panic!("{message}") on validation failure.

---

Summary: Test coverage is complete and satisfies the requirements. However, if you want to address the API design feedback, you would need to either:

• Change build() to return Result<ParsedSpans, String>, or
• Add explicit documentation/naming to clarify that invalid spans cause a panic (if you've decided that panicking is the correct behavior for this invariant)

Would you like guidance on implementing either approach, or are you satisfied with the current panic-on-invalid-input design?

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/parser/cst_builder/spans.rs (2)

338-460: File exceeds the 400-line limit—extract tests to a separate file.

At 460 lines, this file breaches the 400-line guideline. Move the #[cfg(test)] mod tests block (lines 338-460) to a dedicated file such as src/parser/cst_builder/spans/tests.rs or keep it as an external integration test under tests/. This also removes the duplication of assert_panic_with_message if extracted to a shared test utility.

As per coding guidelines, files must not exceed 400 lines; large test modules should be decomposed.

---

342-354: Extract assert_panic_with_message to a shared test utility.

This helper is duplicated in tests/span_validation.rs. Move it to a common location (e.g., src/test_util/mod.rs gated behind a test-support feature, or tests/common/mod.rs) to adhere to DRY.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06f9376 and 0fbaa1f.

📒 Files selected for processing (2)

• src/parser/cst_builder/spans.rs
• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• tests/span_validation.rs
• src/parser/cst_builder/spans.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• tests/span_validation.rs
• src/parser/cst_builder/spans.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• tests/span_validation.rs
• src/parser/cst_builder/spans.rs

🧬 Code graph analysis (2)

tests/span_validation.rs (1)

src/parser/cst_builder/spans.rs (3)

• imports (123-126)
• imports (241-243)
• builder (233-235)

src/parser/cst_builder/spans.rs (3)

src/parser/cst_builder/tree.rs (2)

• new (19-24)
• new (51-64)

src/parser/cst_builder/mod.rs (1)

• new (25-35)

tests/span_validation.rs (3)

• err (11-11)
• err (13-13)
• assert_panic_with_message (3-15)

🪛 GitHub Actions: CI

tests/span_validation.rs

[error] 31-31: rustfmt formatting check failed. Detected diff at line 31. Run 'cargo fmt' to auto-format.

🪛 GitHub Check: build-test

tests/span_validation.rs

[warning] 31-31:
Diff in /home/runner/work/ddlint/ddlint/tests/span_validation.rs

🔍 Remote MCP Deepwiki

Summary of Additional Context

Based on my search of the ddlint repository, here is the relevant context for reviewing this pull request:

Critical Purpose of Span Validation

Span validation ensures that collected spans for different statement categories are sorted and do not overlap, which is essential because these spans are later used by build_green_tree to accurately construct the Concrete Syntax Tree (CST), where incorrect span ordering or overlaps would lead to a corrupted or malformed tree.

Production Risk of Current Debug-Only Validation

The PR addresses a significant production vulnerability. If invalid spans were to exist in production code due to the absence of validation, it could lead to corrupted CST, misleading error diagnostics (where error messages point to incorrect locations), and incorrect AST construction.

Currently, validation is enforced only in debug builds using debug_assert!, meaning the validation checks are compiled out and not performed in release builds.

Integration Context

The parse_tokens function in src/parser/span_scanner.rs collects spans through individual collector functions for each DDlog construct type and then uses ParsedSpansBuilder to create a ParsedSpans object that is passed to build_green_tree.

The broader parsing pipeline already employs multiple error handling patterns including span-level error recovery and token-level error wrapping, suggesting a mature error recovery strategy that the new validation mechanism integrates with.

Design Review Consideration

The CodeRabbit feedback correctly identifies a tension: the PR enforces validation unconditionally (via panic in build() and explicit try_build() error return), which prevents silent corruption in production but requires all callers to either handle the error path or accept panic behavior. Given that span construction is an internal invariant managed by the parsing pipeline rather than user input, the panic-on-failure approach aligns with the architectural assumption that invalid spans represent a development error, not a user input error.

⏰ 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: Sourcery review

🔇 Additional comments (14)

tests/span_validation.rs (3)

17-22: LGTM!

This test exercises the happy path through the public parse API, confirming valid spans produce no errors and the expected import count.

---

24-30: LGTM!

Good coverage of the panic path for invalid spans via the public builder API, with assertion on the error message content.

---

32-41: LGTM!

This test validates the non-panicking try_build() path and confirms the error message contains the expected substring.

src/parser/cst_builder/spans.rs (11)

1-7: LGTM!

The module-level doc comment clearly explains the module's purpose and the invariants enforced by the builder.

---

11-55: LGTM!

SpanListIssue is well-structured with private fields, public accessors marked #[must_use], and a clear Display implementation. The design allows callers to inspect validation failures programmatically.

---

57-80: LGTM!

SpanListValidationError correctly uses thiserror for the Error trait derivation. The new constructor aggregates issues into a human-readable message, and issues() exposes structured access. Good use of semantic error enums per the coding guidelines.

---

177-190: LGTM!

The dual API (try_build returning Result, build panicking) is clean. The unwrap_or_else with a panic on the error message makes the intent explicit and satisfies the PR objective of surfacing mistakes in all builds.

---

192-227: LGTM!

span_lists() centralises the list of span categories for validation, and into_parsed_spans() cleanly destructures and rebuilds the struct. Both helpers reduce duplication and improve maintainability.

---

319-336: enforce_valid_span_lists is a trivial wrapper—keep or inline depending on call-site clarity.

The helper is a single-line delegation to validate_span_lists_sorted. If the naming aids intent at call sites, retain it; otherwise, inline to reduce indirection. Either way, this is a minor readability choice.

---

386-396: LGTM!

Good test verifying that validate_span_lists_sorted correctly reports the list name in the returned issue.

---

398-403: LGTM!

This test confirms the builder succeeds on valid, sorted spans and returns the expected data.

---

405-421: LGTM!

Tests for panic on unsorted and overlapping spans are appropriate and verify the expected error message substring.

---

423-435: LGTM!

This test confirms that multiple validation errors (across different span lists) are all reported in a single panic message.

---

437-459: LGTM!

Good coverage of try_build: error path with structured issue inspection and success path returning valid ParsedSpans.

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

Fix all issues with AI Agents 🤖

In @tests/span_validation.rs:
- Around line 24-30: The test unnecessarily binds the builder result to `_`
before invoking the panic; remove the unused binding so the panic occurs
directly. In the test function builder_panics_on_invalid_spans_in_public_api,
call ParsedSpans::builder().imports(vec![2..4, 1..2]).build() directly inside
assert_panic_with_message (without assigning to `_`), leaving the rest of the
assertion (assert!(text.contains("imports not sorted"))) unchanged.
- Around line 32-39: The test should use .expect() instead of the manual else {
panic!(...) } pattern and assert on the structured validation error: call
ParsedSpans::builder().imports(...).try_build().expect_err("expected validation
error") to get the error value, downcast or match it to the
SpanListValidationError and then inspect its .issues() (or the appropriate
accessor) to assert the per-list issue contains the "imports not sorted"
condition rather than only checking the formatted string; update the test around
ParsedSpans::builder() and try_build() to perform these checks.

♻️ Duplicate comments (2)

tests/span_validation.rs (2)

1-1: Add the module-level doc comment.

The module-level doc comment required by the coding guidelines is still missing.

As per coding guidelines, every Rust module must begin with a module-level (//!) comment.

---

3-15: Extract the duplicated helper to shared test utilities.

This helper is still duplicated from src/parser/cst_builder/spans.rs.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c7a2008 and 8bda774.

📒 Files selected for processing (1)

• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• tests/span_validation.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• tests/span_validation.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• tests/span_validation.rs

🧬 Code graph analysis (1)

tests/span_validation.rs (1)

src/parser/cst_builder/spans.rs (6)

• assert_panic_with_message (342-354)
• err (350-350)
• err (352-352)
• imports (123-126)
• imports (241-243)
• builder (233-235)

🔍 Remote MCP Deepwiki

Summary of additional facts relevant to review

• Where validation matters: CST construction (build_green_tree) consumes the eight ParsedSpans lists (imports, typedefs, relations, indexes, functions, transformers, rules) via SpanCursors; incorrect ordering/overlaps in these lists can corrupt the green tree construction and AST/diagnostics. Review should treat span-list validation as a runtime correctness requirement, not just a nicety.,

• New public API surface and behavior:

  • ParsedSpansBuilder::try_build() returns Result<ParsedSpans, SpanListValidationError> (non-panicking).
  • ParsedSpansBuilder::build() now calls the same validation and will panic on validation failure (unconditional runtime validation).
  • New SpanListIssue / SpanListValidationError aggregate per-list issues for richer error reporting. These are now exposed to callers via try_build.,

• Tests and verification:

  • Unit and integration tests added to assert build() panics on invalid spans and try_build() returns aggregated errors; author ran cargo test and cargo test --release. Tests exercise parse → CST → AST pipeline that reaches ParsedSpans::builder().build().,

• Key review action items (concrete):

  1. Search codebase for all call sites that currently invoke ParsedSpansBuilder::build() (including build_green_tree and parse_tokens pipeline) and confirm callers can accept a panic in release; if any caller must continue or handle invalid spans, prefer switching those sites to try_build() and handle Err.,
  2. Confirm that build_green_tree and downstream consumers (AST wrappers, rule runner) do not rely on encountering unsorted/overlapping spans at runtime (i.e., they assume valid spans). If any code silently relied on debug-only checks, update it to handle validation errors or document the hard failure.
  3. Verify the new SpanListValidationError formatting/content is adequate for tooling/CI (message, issues()) and that tests cover multi-list aggregation cases mentioned in PR.

Sources

• Core Parsing Pipeline (parsed-spans usage, build_green_tree, SpanCursors):
• Parser Implementation Details (ParsedSpansBuilder, validation behavior, span collection):
• Testing Strategy (tests exercising parse→CST→AST and release/debug verification):

⏰ 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). (2)

• GitHub Check: Sourcery review
• GitHub Check: build-test

🔇 Additional comments (1)

tests/span_validation.rs (1)

17-22: LGTM!

The test correctly validates that valid spans parse without errors and produce the expected import count.

[coderabbitai]

Actionable comments posted: 2

Fix all issues with AI Agents 🤖

In @src/parser/cst_builder/spans.rs:
- Around line 319-321: The small wrapper function enforce_valid_span_lists
merely delegates to validate_span_lists_sorted and should be removed or
documented: either inline the call to validate_span_lists_sorted at both call
sites that invoke enforce_valid_span_lists, or keep the function but add a brief
comment above enforce_valid_span_lists explaining why the indirection exists
(e.g., reserved for future additional validations), so the intent is clear to
readers.

In @src/test_util/assertions.rs:
- Around line 32-35: The current extraction of the panic payload uses
err.downcast_ref::<String>()...unwrap_or_default(), which silently returns an
empty string for unknown payload types; replace the final unwrap_or_default with
an explicit panic that reports the unexpected payload type so tests fail loudly.
Locate the chain using err.downcast_ref::<String>() and
err.downcast_ref::<&str>() and change the trailing .unwrap_or_default() to
something like .unwrap_or_else(|| panic!("unexpected panic payload type: not
String or &str")); ensure the descriptive panic runs when neither downcast
matches.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/parser/cst_builder/spans.rs (2)

338-447: Extract tests to reduce file length.

The file is 447 lines, exceeding the 400-line limit. Move the tests to a separate tests.rs submodule or to an integration test file to comply with the coding guidelines.

As per coding guidelines, files must not exceed 400 lines in length.

---

392-408: Parameterise these duplicate test cases with #[rstest].

builder_panics_on_unsorted and builder_panics_on_overlap are nearly identical, differing only in input spans. Use #[rstest] to combine them into a single parameterised test.

As per coding guidelines, replace duplicated tests with #[rstest(...)] parameterised cases.

Proposed fix

+    use rstest::rstest;
+
-    #[test]
-    fn builder_panics_on_unsorted() {
-        let unsorted = vec![1..2, 0..1];
-        let text = assert_panic_with_message(|| {
-            let _ = ParsedSpans::builder().imports(unsorted).build();
-        });
-        assert!(text.contains("imports not sorted"));
-    }
-
-    #[test]
-    fn builder_panics_on_overlap() {
-        let overlapping = vec![0..3, 2..4];
-        let text = assert_panic_with_message(|| {
-            let _ = ParsedSpans::builder().imports(overlapping).build();
-        });
-        assert!(text.contains("imports not sorted"));
-    }
+    #[rstest]
+    #[case::unsorted(vec![1..2, 0..1])]
+    #[case::overlap(vec![0..3, 2..4])]
+    fn builder_panics_on_invalid_spans(#[case] spans: Vec<Span>) {
+        let text = assert_panic_with_message(|| {
+            let _ = ParsedSpans::builder().imports(spans).build();
+        });
+        assert!(text.contains("imports not sorted"));
+    }

♻️ Duplicate comments (2)

tests/span_validation.rs (2)

14-16: Remove the unnecessary let _ = binding.

The result is never used since the function panics. Invoke the builder directly without binding.

Proposed fix

     let text = assert_panic_with_message(|| {
-        let _ = ParsedSpans::builder().imports(vec![2..4, 1..2]).build();
+        ParsedSpans::builder().imports(vec![2..4, 1..2]).build();
     });

---

20-27: Use .expect_err() and validate the structured error.

Replace the let Err(err) = err else { panic!(...) } pattern with .expect_err() as recommended by the coding guidelines. Additionally, validate the structured error via .issues() to ensure SpanListValidationError properly exposes per-list issues.

Proposed fix

 #[test]
 fn builder_try_build_exposes_validation_error() {
-    let err = ParsedSpans::builder().imports(vec![2..4, 1..2]).try_build();
-    let Err(err) = err else {
-        panic!("expected validation error");
-    };
+    let err = ParsedSpans::builder()
+        .imports(vec![2..4, 1..2])
+        .try_build()
+        .expect_err("expected validation error");
     assert!(err.to_string().contains("imports not sorted"));
+    assert!(!err.issues().is_empty(), "expected at least one issue");
 }

As per coding guidelines, prefer .expect() over manual error unwrapping in tests.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8bda774 and 20facff.

📒 Files selected for processing (4)

• src/parser/cst_builder/spans.rs
• src/test_util/assertions.rs
• src/test_util/mod.rs
• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• src/test_util/mod.rs
• src/test_util/assertions.rs
• tests/span_validation.rs
• src/parser/cst_builder/spans.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• src/test_util/mod.rs
• src/test_util/assertions.rs
• tests/span_validation.rs
• src/parser/cst_builder/spans.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• src/test_util/mod.rs
• src/test_util/assertions.rs
• tests/span_validation.rs
• src/parser/cst_builder/spans.rs

🧬 Code graph analysis (3)

src/test_util/mod.rs (1)

src/test_util/assertions.rs (3)

• assert_no_parse_errors (51-53)
• assert_panic_with_message (24-36)
• assert_parse_error (73-92)

tests/span_validation.rs (3)

src/test_util/assertions.rs (3)

• assert_panic_with_message (24-36)
• err (32-32)
• err (34-34)

src/parser/cst_builder/mod.rs (2)

• errors (51-53)
• root (45-47)

src/parser/cst_builder/spans.rs (3)

• imports (123-126)
• imports (241-243)
• builder (233-235)

src/parser/cst_builder/spans.rs (1)

src/test_util/assertions.rs (3)

• assert_panic_with_message (24-36)
• err (32-32)
• err (34-34)

⏰ 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). (2)

• GitHub Check: Sourcery review
• GitHub Check: build-test

🔇 Additional comments (7)

src/parser/cst_builder/spans.rs (4)

11-55: LGTM!

The SpanListIssue struct is well-designed with clear accessors and a descriptive Display implementation. The private constructor appropriately restricts instantiation to internal validation logic.

---

57-80: LGTM!

Good use of thiserror for error derivation. The constructor pre-computes the display message from issues, and the issues() accessor allows callers to inspect individual validation failures programmatically.

---

177-181: LGTM!

The try_build() method provides a non-panicking alternative that returns a Result, allowing callers to handle validation errors gracefully. This aligns with the coding guidelines' preference for Result over panicking.

---

183-190: LGTM!

The build() method intentionally panics on validation failure as documented. The unwrap_or_else pattern is appropriate here since the panic is the intended API behaviour for invalid spans, and the alternative try_build() is available for callers who need error handling.

src/test_util/mod.rs (1)

11-14: LGTM!

The new assert_panic_with_message export is correctly grouped with related assertion helpers, making it available for integration tests.

tests/span_validation.rs (2)

1-3: LGTM!

The module-level doc comment properly explains the purpose of this test module, and the imports are clean.

---

5-10: LGTM!

Simple and clear test verifying the happy path for span parsing.

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 8

Fix all issues with AI Agents 🤖

In @src/parser/cst_builder/spans.rs:
- Around line 183-190: In the ParsedSpans builder method `build`, replace the
call `enforce_valid_span_lists(&self.span_lists()).unwrap_or_else(|error| {
panic!("{error}"); });` with a single `expect(...)` call so the panic pattern is
simplified; call `enforce_valid_span_lists(&self.span_lists()).expect("span list
validation failed");` (or include a short contextual message) and then return
`self.into_parsed_spans()` as before, referencing the `build`,
`enforce_valid_span_lists`, `span_lists`, and `into_parsed_spans` symbols to
locate the change.

In @src/parser/cst_builder/spans/tests.rs:
- Around line 25-29: The test function validate_spans_sorted_ok_on_single
constructs a single-element Vec<Span> using std::iter::once(0..3).collect(),
which is verbose; change it to use the clearer vec![0..3] literal instead so the
spans variable is created as let spans: Vec<Span> = vec![0..3]; keeping the rest
of the test and the call to validate_spans_sorted(&spans) unchanged.
- Around line 56-65: The test builder_panics_on_invalid_spans contains an
unnecessary drop(parsed) call; remove the drop(parsed) line and simply invoke
ParsedSpans::builder().imports(spans).build() inside the closure (keeping the
assert_panic_with_message wrapper) so the temporary parsed value is allowed to
fall out of scope naturally and avoid the redundant drop(parsed) reference.
- Around line 67-80: The test builder_reports_all_errors contains an unnecessary
explicit drop call; remove the drop(parsed) statement inside the closure so the
ParsedSpans instance created by ParsedSpans::builder()...build() is allowed to
fall out of scope and be dropped automatically; update the test to simply
construct parsed (let parsed = ParsedSpans::builder()...build();) and not call
drop(parsed).
- Around line 96-104: In the test try_build_ok_on_sorted_spans replace the
manual match that panics with a call to .expect(...) on
ParsedSpans::builder().imports(spans.clone()).try_build() so the code becomes a
single expression that unwraps or fails with a clear message; keep the same
expectation text (e.g., "expected valid spans") to preserve test intent and then
assert parsed.imports() as before.
- Around line 82-94: The test try_build_errs_on_unsorted_spans uses manual
let-else panic patterns; change it to use expect/expect_err for clarity and
consistency: call try_build and use .expect_err("expected validation error") to
get the error, then replace the let-else on err.issues().first() with
.expect("expected at least one issue") to obtain the first issue, keeping the
existing assertions (assert_eq! on issue.list() and issues length) intact;
reference ParsedSpans::builder(), try_build(), and the test function
try_build_errs_on_unsorted_spans when making the edits.
- Around line 37-47: The test validate_span_lists_sorted_reports_list_name uses
let-else panic patterns; replace them with .expect() calls: call
validate_span_lists_sorted(&[("imports", &imports)]) and unwrap the Err with
.expect("expected validation error"), then call
err.issues().first().expect("expected at least one issue") to get the issue;
keep the final assert_eq!(issue.list(), "imports") unchanged. This removes
manual panic patterns and follows the test guideline to prefer .expect().

In @tests/span_validation.rs:
- Around line 20-36: In the test builder_try_build_exposes_validation_error
replace the manual let-else panic for extracting the first issue with a direct
.expect() call: change the block using issues.first() with let Some(issue) = ...
else { panic!(...) } to let issue = issues.first().expect("expected at least one
issue"); so the test uses .expect() consistently when extracting the first item
from issues.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20facff and e4e4e94.

📒 Files selected for processing (4)

• src/parser/cst_builder/spans.rs
• src/parser/cst_builder/spans/tests.rs
• src/test_util/assertions.rs
• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs
• src/test_util/assertions.rs
• src/parser/cst_builder/spans.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs
• src/test_util/assertions.rs
• src/parser/cst_builder/spans.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs
• src/test_util/assertions.rs
• src/parser/cst_builder/spans.rs

⏰ 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: Sourcery review

🔇 Additional comments (15)

src/parser/cst_builder/spans/tests.rs (1)

1-6: LGTM: Module structure is correct.

The module doc comment and imports are appropriate for a test module.

src/parser/cst_builder/spans.rs (10)

1-9: LGTM: Module documentation is clear and accurate.

The module doc correctly states that validation is enforced across all builds.

---

11-55: LGTM: SpanListIssue is well-designed.

The type provides good encapsulation with private fields and public accessors. The Display implementation is clear and helpful.

---

57-80: LGTM: SpanListValidationError follows error handling best practices.

The type correctly uses thiserror::Error for a semantic error enum, as recommended by the coding guidelines. The message field caches the formatted output for the panic case; whilst this duplicates data, the overhead is negligible for validation errors.

---

82-105: LGTM: Documentation correctly reflects the validation guarantee.

The doc comment at line 85 accurately states that instances ensure sorted and non-overlapping spans.

---

177-181: LGTM: try_build provides a non-panicking validation path.

The method correctly validates spans and returns a Result, allowing callers to handle validation failures without process termination.

---

192-227: LGTM: Helper methods are appropriately scoped.

The span_lists() and into_parsed_spans() helpers cleanly separate validation setup from the final construction.

---

288-317: LGTM: Internal validation logic is correct.

The validate_spans_sorted function correctly detects overlapping and unsorted spans by checking that each span ends before the next begins.

---

319-323: LGTM: Wrapper is appropriately documented.

The comment at lines 319-320 clearly explains the purpose of the indirection, addressing the concern from the previous review.

---

325-338: LGTM: Validation correctly accumulates all issues.

The function validates all span lists and collects all issues before returning, providing comprehensive error reporting rather than failing fast.

---

340-341: LGTM: Test module declaration is standard.

src/test_util/assertions.rs (1)

8-38: LGTM: Panic assertion helper is correctly implemented.

The function properly uses catch_unwind, validates that a panic occurred, and downcasts the payload to String or &str. The explicit panic for unknown payload types (lines 35-37) ensures test failures are loud and clear, addressing the concern from the previous review.

tests/span_validation.rs (3)

1-4: LGTM: Module structure is correct.

The module doc clearly describes the purpose of these behavioural tests.

---

5-10: LGTM: Smoke test covers the valid parsing path.

---

12-18: LGTM: Panic test correctly verifies builder behaviour.

The test properly asserts that invalid spans trigger a panic with the expected message. The explicit drop() at line 15 makes it clear the result is intentionally unused.

[coderabbitai]

Actionable comments posted: 4

Fix all issues with AI Agents 🤖

In @src/parser/cst_builder/spans.rs:
- Around line 66-73: The constructor SpanListValidationError::new is private
which prevents external tests or aggregators from creating instances; either
make it public by changing the signature to pub fn new(issues:
Vec<SpanListIssue>) -> Self so callers can construct errors, or if intentional,
add a doc comment above fn new explaining why external construction is forbidden
and how callers should obtain/represent validation errors instead; update any
module tests or uses accordingly to use the new public API or the documented
alternative.
- Around line 177-180: The try_build() method is redundant because it just
delegates to build() while both now return Result<ParsedSpans,
SpanListValidationError>; remove the try_build() function entirely to avoid API
confusion, and update any call sites referencing try_build() to call build()
instead (or, if you intended two behaviors, change build() to panic on error via
.expect() and keep try_build() returning Result). Ensure references to
try_build() in tests and other modules are updated accordingly.

In @src/parser/cst_builder/spans/tests.rs:
- Around line 67-123: The tests duplicate coverage for ParsedSpans::build() and
ParsedSpans::try_build() — decide which API to keep and update tests
accordingly: if you remove try_build(), delete the tests
try_build_errs_on_unsorted_spans and try_build_ok_on_sorted_spans; if you keep
both but make build() panic on error, change builder_errs_on_invalid_spans and
builder_succeeds_on_sorted_spans to assert panic behavior using
std::panic::catch_unwind (verifying a panic for invalid spans) and keep the
try_build_* tests to assert Result-based errors/success; update references in
the test file to use the correct method names (ParsedSpans::builder().build() vs
.try_build()) so each method has unique, non-duplicated coverage.

In @tests/span_validation.rs:
- Around line 12-39: Tests builder_errs_on_invalid_spans_in_public_api and
builder_try_build_exposes_validation_error duplicate coverage for
ParsedSpans::builder() with unsorted imports since build() and try_build() now
behave identically; either remove the redundant test (remove
builder_try_build_exposes_validation_error) if try_build() is deleted, or make
the APIs semantically different and update the tests accordingly (e.g., have
build() panic and assert via std::panic::catch_unwind in
builder_errs_on_invalid_spans_in_public_api while keeping
builder_try_build_exposes_validation_error to call try_build() and assert the
returned Result error), referencing ParsedSpans::builder(), build(),
try_build(), and the test function names to locate changes.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4e4e94 and e095dbb.

📒 Files selected for processing (5)

• src/parser/cst_builder/spans.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/cst_builder/tree.rs
• src/parser/span_scanner.rs
• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• tests/span_validation.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/span_scanner.rs
• src/parser/cst_builder/tree.rs
• src/parser/cst_builder/spans.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• tests/span_validation.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/span_scanner.rs
• src/parser/cst_builder/tree.rs
• src/parser/cst_builder/spans.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• tests/span_validation.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/span_scanner.rs
• src/parser/cst_builder/tree.rs
• src/parser/cst_builder/spans.rs

🧬 Code graph analysis (4)

tests/span_validation.rs (4)

src/parser/cst_builder/mod.rs (2)

• errors (51-53)
• root (45-47)

src/parser/ast/parse_utils/errors.rs (1)

• is_empty (144-146)

src/parser/cst_builder/spans.rs (6)

• imports (123-126)
• imports (237-239)
• builder (229-231)
• issues (67-70)
• issues (77-79)
• list (30-32)

src/test_util/assertions.rs (2)

• err (32-32)
• err (34-34)

src/parser/cst_builder/spans/tests.rs (2)

src/parser/cst_builder/spans.rs (12)

• validate_spans_sorted (302-313)
• new (20-26)
• new (66-73)
• imports (123-126)
• imports (237-239)
• validate_span_lists_sorted (321-334)
• list (30-32)
• builder (229-231)
• typedefs (130-133)
• typedefs (243-245)
• issues (67-70)
• issues (77-79)

src/test_util/assertions.rs (2)

• err (32-32)
• err (34-34)

src/parser/span_scanner.rs (1)

src/parser/cst_builder/spans.rs (1)

• builder (229-231)

src/parser/cst_builder/tree.rs (1)

src/parser/cst_builder/spans.rs (1)

• builder (229-231)

🔍 Remote MCP Deepwiki

Summary of Additional Context for PR Review

Based on the repository documentation, here is the relevant context for reviewing this span validation enforcement PR:

Purpose and Significance of Changes

**** Span validation ensures the Concrete Syntax Tree (CST) maintains data integrity by verifying all collected spans are sorted and non-overlapping. Spans represent byte ranges in source text that are critical for the CST builder's build_green_tree function to correctly determine where syntax nodes should start and finish. Invalid spans can lead to corrupted syntax trees and misleading error diagnostics.

Critical change: The PR transitions validation from debug-only (compiled out in release builds) to unconditional runtime validation in all builds. This is significant because:

1. Production reliability: Previously, unsorted or overlapping spans could silently fail in release builds, potentially corrupting the CST and affecting downstream processing.
2. Accurate error reporting: chumsky relies on correct spans to generate precise error messages pointing to syntax error locations. Invalid spans would produce misleading diagnostics.
3. Earlier bug detection: Enforcing validation at runtime shifts potential silent failures to explicit panics during development and testing.

API and Integration Context

**** The ParsedSpansBuilder is part of the Span Analysis phase of the parsing pipeline. It collects span lists (imports, typedefs, relations, indexes, functions, transformers, rules, expressions) and constructs a validated ParsedSpans object passed to build_green_tree. The builder previously called validate_span_lists_sorted only as a debug assertion; the PR makes this validation unconditional and exposes errors through a public SpanListValidationError API.

Error Handling Design Context

**** The parser follows a recovery-oriented design using chumsky that distinguishes recoverable errors (delimiter mismatches, missing delimiters) from fatal failures. The codebase uses N_ERROR nodes and resynchronization to continue parsing after errors. This context is relevant because the PR's introduction of SpanListValidationError with detailed issue reporting aligns with the broader error handling philosophy of providing structured, actionable error information rather than silent failures.

⏰ 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). (2)

• GitHub Check: Sourcery review
• GitHub Check: build-test

🔇 Additional comments (3)

src/parser/cst_builder/spans.rs (1)

315-319: LGTM—wrapper purpose is documented.

The comment at lines 315-316 clearly explains why enforce_valid_span_lists exists as a wrapper, addressing the prior review feedback about potential inlining.

src/parser/cst_builder/tree.rs (1)

187-190: LGTM—correctly adapts test to Result-based API.

The test now matches on the Result returned by build(), panicking with a clear message on unexpected validation errors. This preserves the test's intent (verifying that OOB token spans trigger a panic in build_green_tree, not in span validation) while adapting to the new API.

src/parser/span_scanner.rs (1)

54-73: Remove the error handling comment—the implementation is safe.

ParsedSpans::default() safely produces empty collections via derived Default on Vec<Span> fields. The error is properly captured in all_errors, so validation failures are not hidden. The 0..0 sentinel span correctly indicates the error originated from validation rather than a specific source location.

[coderabbitai]

Actionable comments posted: 1

Fix all issues with AI Agents 🤖

In @src/parser/cst_builder/spans/tests.rs:
- Around line 67-75: The test builder_errs_on_invalid_spans uses a manual
let-else with panic; replace that pattern by calling .expect_err("expected
validation error") on ParsedSpans::builder().imports(spans).build() to assert an
error is returned and remove the let-else block so the assertion is consistent
with other tests.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e095dbb and e24cce6.

📒 Files selected for processing (3)

• src/parser/cst_builder/spans.rs
• src/parser/cst_builder/spans/tests.rs
• tests/span_validation.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Clippy warnings must be disallowed in Rust code.
Fix any warnings emitted during tests in the code itself rather than silencing them in Rust.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider using Arc to reduce the amount of data returned.
Write unit and scenario tests for new Rust functionality; run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///), so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings in Rust.
Handle errors with the Result type instead of panicking in Rust where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced in Rust except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated tests with #[rstest(…)] parameterized cases in Rust.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of functions in Rust where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) } instead of multi-line variants).
Use NewTypes in Rust to model domain values and eliminate "i...

Files:

• tests/span_validation.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/cst_builder/spans.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 / -yse / -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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• 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.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• tests/span_validation.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/cst_builder/spans.rs

🧠 Learnings (1)

📚 Learning: 2025-12-26T13:23:35.435Z

Learnt from: leynos
Repo: leynos/ddlint PR: 197
File: src/test_util/literals.rs:135-139
Timestamp: 2025-12-26T13:23:35.435Z
Learning: In Rust projects, rustfmt may reflow long simple constructor functions (e.g., pub fn lit_bool(b: bool) -> Expr { Expr::Literal(Literal::Bool(b)) }) to a multi-line form. Ensure you run and respect rustfmt formatting so CI formatting checks pass. Do not manually enforce non-default line breaks; format with rustfmt. Consider adding a rustfmt --check (or cargo fmt) step in CI and running cargo fmt before commits to avoid formatting-related failures.

Applied to files:

• tests/span_validation.rs
• src/parser/cst_builder/spans/tests.rs
• src/parser/cst_builder/spans.rs

🧬 Code graph analysis (2)

tests/span_validation.rs (4)

src/parser/cst_builder/mod.rs (2)

• errors (51-53)
• root (45-47)

src/parser/ast/parse_utils/errors.rs (1)

• is_empty (144-146)

src/parser/cst_builder/spans.rs (3)

• imports (125-128)
• imports (234-236)
• builder (226-228)

src/test_util/assertions.rs (2)

• err (32-32)
• err (34-34)

src/parser/cst_builder/spans/tests.rs (1)

src/parser/cst_builder/spans.rs (10)

• validate_spans_sorted (299-310)
• new (20-26)
• new (68-75)
• imports (125-128)
• imports (234-236)
• validate_span_lists_sorted (318-331)
• list (30-32)
• builder (226-228)
• typedefs (132-135)
• typedefs (240-242)

🔍 Remote MCP Deepwiki

Summary of additional repository context relevant to this PR

• Parsing pipeline: parse() → tokenize() → parse_tokens() (span collection) → build_green_tree() (CST); ParsedSpans produced by span_scanner feed cst_builder/tree.rs for CST construction. Enforced span correctness affects CST construction and downstream AST creation.

• ParsedSpans / builder role: ParsedSpansBuilder collects named span lists (imports, typedefs, relations, indexes, functions, transformers, rules, expressions) and previously performed validation only via debug assertions; build_green_tree relies on these spans and SpanCursors to start/finish nodes. Making validation unconditional changes the contract consumed by CST builder.

• Error handling and recovery philosophy: the project emphasizes recovery-oriented parsing (chumsky) and collecting parse errors (Vec<Simple>) rather than panics; introducing a public SpanListValidationError shifts some failures from silent/recoverable to explicit/fatal and interacts with this design.

• Tests & CI expectations: extensive test strategy uses rstest fixtures and enforces tests and clippy warnings as errors (make targets run cargo test / clippy with -D warnings). New tests added for span validation must pass both debug and release test runs as per repo conventions.

• Developer workflow/quality gates: repo Makefile and CI expect formatting, clippy, and tests to pass; panics in build() (if retained) will fail tests and CI—preferred alternatives in prior reviewer comments were to make build() return Result or provide an explicit enforce_* wrapper.

Sources used

• Deepwiki_read_wiki_contents (repository overview, parser pipeline, span handling, testing strategy, build/CI expectations).

⏰ 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). (2)

• GitHub Check: Sourcery review
• GitHub Check: build-test

🔇 Additional comments (9)

src/parser/cst_builder/spans.rs (3)

11-82: LGTM! Public validation API well-designed.

The SpanListIssue and SpanListValidationError types provide clear, structured error reporting. Making SpanListValidationError::new() public (line 68) addresses the previous review feedback and enables external test construction.

The use of thiserror::Error aligns with the coding guidelines' preference for semantic error enums.

---

179-183: LGTM! build() returning Result addresses API design concerns.

Changing build() to return Result<ParsedSpans, SpanListValidationError> instead of panicking aligns with the project's recovery-oriented parsing philosophy and addresses the API design feedback mentioned in the PR objectives. Callers can now handle validation failures explicitly.

---

312-316: LGTM! Wrapper comment clarifies intent.

The comment explaining why enforce_valid_span_lists exists addresses previous feedback. The indirection provides clear semantics at call sites and reserves space for additional validation types.

tests/span_validation.rs (2)

1-10: LGTM! Integration test covers valid span parsing.

The module-level doc comment and the happy-path test parse_builds_with_valid_spans provide clear coverage for the expected behaviour with valid input.

---

12-23: LGTM! Error path test uses .expect_err() correctly.

The test builder_errs_on_invalid_spans_in_public_api properly uses .expect_err() per coding guidelines and validates the error message content.

src/parser/cst_builder/spans/tests.rs (4)

1-38: LGTM! Comprehensive coverage of validate_spans_sorted.

The unit tests cover overlap detection, unsorted detection, and successful validation for empty, single, and sorted span lists. The clippy::single_range_in_vec_init expectation at line 26 is properly documented.

---

40-51: LGTM! Test uses .expect() patterns correctly.

The test validate_span_lists_sorted_reports_list_name follows the coding guidelines by using .expect_err() and .expect() instead of manual panic patterns.

---

53-65: LGTM! Builder success path validated correctly.

The test builder_succeeds_on_sorted_spans uses .expect() per guidelines and confirms the builder constructs ParsedSpans correctly when given valid input.

---

77-93: LGTM! Multi-error reporting validated correctly.

The test builder_reports_all_errors uses .expect_err() per guidelines and confirms that validation errors for multiple span lists are aggregated and reported together.

[coderabbitai]

The review comment has been resolved. The problematic unwrap_or_else pattern no longer exists - build() now returns Result directly instead of panicking.