feat(engine): capture complete Cargo analysis observations - #162
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe diagnostics parser now returns complete Cargo analyses with producer metadata, observations, raw and normalized spans, nested diagnostics, suggestions, completion states, and execution evidence. Existing diagnostic and status APIs remain available through compatibility wrappers. ChangesCargo analysis capture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Cargo
participant parse_cargo_analysis
participant CargoAnalysis
participant CompatibilityAPI
Cargo->>parse_cargo_analysis: compiler-message and build-finished JSON
parse_cargo_analysis->>CargoAnalysis: observations, metadata, spans, completion
CargoAnalysis->>CompatibilityAPI: normalized legacy diagnostics
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/lintdiff-engine/src/diagnostics/mod.rs`:
- Around line 827-830: Update the diagnostic test around parse_cargo_analysis to
use parse_cargo_analysis_with_repo_root with a repository root, then provide
sibling spans covering line_start 0 and a valid positive line. Assert the
zero-based span has normalized == None and the valid-line span has normalized ==
Some, so the line_start > 0 guard is exercised rather than masked by a missing
repository path.
- Around line 644-654: Make repository_path platform-independent by normalizing
both raw_file_name and repo_root separators to forward slashes, detecting
absolute paths using the normalized representation, and removing the normalized
repository root via string-prefix matching rather than Path::strip_prefix;
update crates/lintdiff-engine/src/diagnostics/mod.rs lines 644-654 accordingly.
In the test fixture at crates/lintdiff-engine/src/diagnostics/mod.rs lines
841-855, retain the Windows-style case and add a POSIX-style case, with both
asserting the normalized result src/lib.rs.
- Line 378: Update the documentation comment near the Cargo JSONL parser to say
it “derives” repository-relative paths instead of “earns” them, and rename the
related test to repository_root_derives_context_correct_absolute_paths.
- Around line 187-263: Add concise rustdoc comments to every public field in
ProducerUnit, CargoTarget, AnalysisScope, ObservationSpan, DiagnosticSuggestion,
DiagnosticChild, and DiagnosticObservation, describing each field’s purpose and
preserving the existing struct-level documentation.
- Around line 274-283: Make the completion field in UpstreamExecution store
AnalysisCompletion directly instead of Option, and add
AnalysisCompletion::default() returning IncompleteStream so
UpstreamExecution::default() remains valid. Update all construction sites,
including parse_cargo_analysis_with_repo_root and
CargoAnalysis::runtime_failure, to assign direct values, remove the unreachable
InvalidShape line: 0 branch from parse_cargo_messages_with_status, and adjust
affected test assertions.
- Around line 405-419: The build-finished handling in parse_cargo_messages
currently reads invocation scope fields that Cargo does not emit. Remove the
toolchain, target, and features extraction from the build-finished branch,
provide the invocation scope separately, and thread it through
parse_cargo_messages into parse_cargo_analysis so AnalysisScope is populated
from that supplied context; update the fixture to match Cargo’s build-finished
schema containing only reason and success.
- Around line 538-543: Update parse_producer so ProducerUnit does not expose the
full Cargo profile under the misleading profile field while retaining only
opt_level; rename the stored field and its consumers to opt_level, or introduce
a dedicated profile structure that captures opt_level, debuginfo,
debug_assertions, overflow_checks, and test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a3fd5daa-2923-4522-856d-63cea4d5505d
📒 Files selected for processing (2)
crates/lintdiff-engine/src/diagnostics/mod.rscrates/lintdiff-engine/src/lib.rs
| /// The Cargo producer identity attached to one compiler-message emission. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq)] | ||
| pub struct ProducerUnit { | ||
| pub package_id: Option<String>, | ||
| pub manifest_path: Option<String>, | ||
| pub target: Option<CargoTarget>, | ||
| pub profile: Option<String>, | ||
| } | ||
|
|
||
| /// The package target that produced a compiler message. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq)] | ||
| pub struct CargoTarget { | ||
| pub name: Option<String>, | ||
| pub kind: Vec<String>, | ||
| pub crate_types: Vec<String>, | ||
| pub src_path: Option<String>, | ||
| pub edition: Option<String>, | ||
| } | ||
|
|
||
| /// Hard and contextual inputs that determine whether two analyses are comparable. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq)] | ||
| pub struct AnalysisScope { | ||
| pub repository: Option<String>, | ||
| pub revision: Option<String>, | ||
| pub toolchain: Option<String>, | ||
| pub target: Option<String>, | ||
| pub features: Vec<String>, | ||
| pub package_selection: Vec<String>, | ||
| pub target_selection: Vec<String>, | ||
| pub lint_config_hash: Option<String>, | ||
| } | ||
|
|
||
| /// A source span retaining raw Cargo values alongside the current normalized span. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct ObservationSpan { | ||
| pub raw_file_name: Option<String>, | ||
| pub raw_line_start: Option<u32>, | ||
| pub raw_line_end: Option<u32>, | ||
| pub raw_column_start: Option<u32>, | ||
| pub raw_column_end: Option<u32>, | ||
| pub normalized: Option<Span>, | ||
| pub is_primary: bool, | ||
| } | ||
|
|
||
| /// A suggestion emitted as part of a compiler child diagnostic. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct DiagnosticSuggestion { | ||
| pub file_name: Option<String>, | ||
| pub line_start: Option<u32>, | ||
| pub line_end: Option<u32>, | ||
| pub replacement: Option<String>, | ||
| pub applicability: Option<String>, | ||
| } | ||
|
|
||
| /// A child note, help message, or nested diagnostic. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct DiagnosticChild { | ||
| pub raw_level: String, | ||
| pub level: DiagnosticLevel, | ||
| pub message: String, | ||
| pub rendered: Option<String>, | ||
| pub spans: Vec<ObservationSpan>, | ||
| pub suggestions: Vec<DiagnosticSuggestion>, | ||
| } | ||
|
|
||
| /// One Cargo compiler-message observation, before filtering or policy. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct DiagnosticObservation { | ||
| pub producer: ProducerUnit, | ||
| pub raw_level: String, | ||
| pub raw_code: Option<String>, | ||
| pub message: String, | ||
| pub rendered: Option<String>, | ||
| pub spans: Vec<ObservationSpan>, | ||
| pub children: Vec<DiagnosticChild>, | ||
| pub diagnostic: Diagnostic, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for missing_docs lint configuration in the crate and workspace.
fd -t f 'lib.rs' crates/lintdiff-engine/src --exec sed -n '1,20p' {}
fd -t f 'Cargo.toml' --exec rg -n 'missing_docs|\[lints' {} \;Repository: EffortlessMetrics/lintdiff
Length of output: 910
🏁 Script executed:
#!/bin/bash
set -u
echo "== Cargo lints / deny settings =="
for f in $(fd -t f 'Cargo.toml|^\.cargo/config.toml|Cargo.lock$' .); do
echo "--- $f"
rg -n 'missing_docs|deny\(.*docs|warn\(.*docs|forbid\(.*docs|\[lints\b|\[workspace\.lints' "$f" || true
done
echo
echo "== lintdiff-engine diagnostics relevant sections =="
sed -n '1,120p' crates/lintdiff-engine/src/diagnostics/mod.rs
sed -n '120,200p' crates/lintdiff-engine/src/diagnostics/mod.rs
sed -n '260,280p' crates/lintdiff-engine/src/diagnostics/mod.rsRepository: EffortlessMetrics/lintdiff
Length of output: 7637
Add field-level documentation to the new public types.
AnalysisScope, ObservationSpan, DiagnosticSuggestion, DiagnosticChild, DiagnosticObservation, ProducerUnit, and CargoTarget document the struct but leave public fields undoced. Cargo does not enable missing_docs, but field docs keep this module consistent with existing public API shapes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` around lines 187 - 263, Add
concise rustdoc comments to every public field in ProducerUnit, CargoTarget,
AnalysisScope, ObservationSpan, DiagnosticSuggestion, DiagnosticChild, and
DiagnosticObservation, describing each field’s purpose and preserving the
existing struct-level documentation.
| /// Process and Cargo completion evidence for one analysis. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq)] | ||
| pub struct UpstreamExecution { | ||
| pub command: Vec<String>, | ||
| pub exit_code: Option<i32>, | ||
| pub duration_ms: Option<u64>, | ||
| pub build_finished_seen: bool, | ||
| pub build_success: Option<bool>, | ||
| pub completion: Option<AnalysisCompletion>, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make completion non-optional in UpstreamExecution.
Every construction site sets completion to Some(..). parse_cargo_analysis_with_repo_root sets it at line 447. CargoAnalysis::runtime_failure sets it at line 302. The Option therefore models a state that the code never produces. It forces parse_cargo_messages_with_status (lines 464-469) to build an InvalidShape error with the meaningless line: 0 for an unreachable case.
Store AnalysisCompletion directly and delete the unreachable error branch.
♻️ Proposed change
pub struct UpstreamExecution {
pub command: Vec<String>,
pub exit_code: Option<i32>,
pub duration_ms: Option<u64>,
pub build_finished_seen: bool,
pub build_success: Option<bool>,
- pub completion: Option<AnalysisCompletion>,
+ pub completion: AnalysisCompletion,
}AnalysisCompletion then needs a Default implementation returning IncompleteStream so that UpstreamExecution::default() keeps working. Update the assertions in the tests accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` around lines 274 - 283, Make
the completion field in UpstreamExecution store AnalysisCompletion directly
instead of Option, and add AnalysisCompletion::default() returning
IncompleteStream so UpstreamExecution::default() remains valid. Update all
construction sites, including parse_cargo_analysis_with_repo_root and
CargoAnalysis::runtime_failure, to assign direct values, remove the unreachable
InvalidShape line: 0 branch from parse_cargo_messages_with_status, and adjust
affected test assertions.
| parse_cargo_analysis_with_repo_root(reader, None) | ||
| } | ||
|
|
||
| /// Parse Cargo JSONL and earn repository-relative paths from a known root. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the doc comment wording.
"earn repository-relative paths" is not meaningful. Use "derive".
📝 Proposed change
-/// Parse Cargo JSONL and earn repository-relative paths from a known root.
+/// Parse Cargo JSONL and derive repository-relative paths from a known root.The test name at line 842 uses the same word. Rename it to repository_root_derives_context_correct_absolute_paths.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Parse Cargo JSONL and earn repository-relative paths from a known root. | |
| /// Parse Cargo JSONL and derive repository-relative paths from a known root. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` at line 378, Update the
documentation comment near the Cargo JSONL parser to say it “derives”
repository-relative paths instead of “earns” them, and rename the related test
to repository_root_derives_context_correct_absolute_paths.
| let reason = value.get("reason").and_then(Value::as_str); | ||
| if reason == Some("build-finished") { | ||
| build_finished = true; | ||
| build_success = v.get("success").and_then(|x| x.as_bool()); | ||
| build_finished_seen = true; | ||
| build_success = value.get("success").and_then(Value::as_bool); | ||
| scope.toolchain = value | ||
| .get("toolchain") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string); | ||
| scope.target = value | ||
| .get("target") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string); | ||
| scope.features = string_array(value.get("features")); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
cargo JSON build-finished message fields reason success schema
💡 Result:
When running Cargo with --message-format=json, the build-finished message is emitted at the end of the build process to indicate that Cargo has finished producing JSON messages [1][2]. The JSON schema for this message is: { "reason": "build-finished", "success": true } Fields: reason (string): Always set to "build-finished" [1][3]. success (boolean): Indicates whether the build completed successfully (true) or failed (false) [1][4]. This message is useful for external tools to identify when Cargo will no longer produce additional JSON-formatted messages, although other output (such as stdout/stderr from a process executed by cargo run) may still follow [1][5]. It was introduced in Rust 1.44 [6].
Citations:
- 1: https://doc.rust-lang.org/stable/cargo/reference/external-tools.html
- 2: https://doc.rust-lang.org/beta/cargo/reference/external-tools.html
- 3: https://github.com/rust-lang/cargo/blob/refs/heads/master/src/cargo/util/machine_message.rs
- 4: https://doc.rust-lang.org/beta/nightly-rustc/cargo/util/machine_message/struct.BuildFinished.html
- 5: Add "build-finished" JSON message. rust-lang/cargo#8069
- 6: https://docs.rs/cargo_metadata/latest/cargo_metadata/enum.Message.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate diagnostics/mod.rs =="
git ls-files | rg 'crates/lintdiff-engine/src/diagnostics/mod\.rs$|Cargo\.lock$|Cargo\.toml$' | sed -n '1,80p'
echo
echo "== relevant source slice =="
sed -n '350,440p' crates/lintdiff-engine/src/diagnostics/mod.rs
echo
echo "== tests around build-finished =="
sed -n '760,830p' crates/lintdiff-engine/src/diagnostics/mod.rs
echo
echo "== struct/function signatures in diagnostics/mod.rs =="
ast-grep outline crates/lintdiff-engine/src/diagnostics/mod.rs --view expanded | sed -n '1,220p' 2>/dev/null || true
echo
echo "== search for parse build function usages =="
rg -n "parse .*build|BuildScope|build-scope|toolchain|target|features" crates/lintdiff-engine/src crates/lintdiff -S | sed -n '1,160p'Repository: EffortlessMetrics/lintdiff
Length of output: 17213
Do not read toolchain, target, and features from build-finished.
Cargo JSON build-finished only contains reason and success, so these lines leave AnalysisScope fields empty for real Cargo output. Provide the invocation scope information separately and pass it into parse_cargo_messages/parse_cargo_analysis instead, then use a Cargo-schema-aligned fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` around lines 405 - 419, The
build-finished handling in parse_cargo_messages currently reads invocation scope
fields that Cargo does not emit. Remove the toolchain, target, and features
extraction from the build-finished branch, provide the invocation scope
separately, and thread it through parse_cargo_messages into parse_cargo_analysis
so AnalysisScope is populated from that supplied context; update the fixture to
match Cargo’s build-finished schema containing only reason and success.
| profile: value | ||
| .get("profile") | ||
| .and_then(Value::as_object) | ||
| .and_then(|profile| profile.get("opt_level")) | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
cargo JSON compiler-artifact profile object fields opt_level debuginfo
💡 Result:
In Cargo's JSON machine-readable output (specifically the compiler-artifact message), the profile object provides metadata about the compiler settings used for a target [1][2]. The profile object contains the following fields: opt_level: A string representing the optimization level (e.g., "0", "1", "2", "3", "s", or "z") [1][3]. debuginfo: Specifies the level of debug information included [1][2]. This field can be an integer (0, 1, or 2) or a string (such as "line-directives-only" or "line-tables-only") [1][4]. If null, it defaults to 0 [1][2]. debug_assertions: A boolean indicating whether debug assertions (like those triggered by debug_assert!) are enabled [1][3]. overflow_checks: A boolean indicating whether integer overflow checks are enabled [1][3]. test: A boolean indicating whether the target was compiled with test support (e.g., if the --test flag was used) [1][3]. The JSON structure is used by external tools to understand the build environment [1][5]. While some third-party metadata crates historically represented debuginfo as a simple integer, modern Cargo may emit string-based debug information levels, which requires handling both types in consuming applications [4][6].
Citations:
- 1: https://doc.rust-lang.org/stable/cargo/reference/external-tools.html
- 2: https://doc.rust-lang.org/cargo/reference/external-tools.html
- 3: https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.ArtifactProfile.html
- 4: Non numerical values for debuginfo cannot be parsed from cargo messages oli-obk/cargo_metadata#240
- 5: --message-format=json should indicate which produced files are executables rust-lang/cargo#5426
- 6: https://github.com/oli-obk/cargo_metadata/blob/main/src/messages.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'crates/lintdiff-engine/src/diagnostics/mod.rs' 'crates/*Cargo.toml' 'Cargo.toml' | sed -n '1,80p'
echo
echo "== diagnostics outline =="
ast-grep outline crates/lintdiff-engine/src/diagnostics/mod.rs --view expanded 2>/dev/null | sed -n '1,160p' || true
echo
echo "== relevant diagnostics lines =="
cat -n crates/lintdiff-engine/src/diagnostics/mod.rs | sed -n '480,560p'
echo
echo "== DiagnosticObservation and related types =="
rg -n "struct DiagnosticObservation|profile:|opt_level|Compiler|compiler-" crates/lintdiff-engine/src/diagnostics/mod.rs crates/lintdiff-engine/src -g '*.rs' | sed -n '1,220p'
echo
echo "== search for cargo message crate usage =="
rg -n "cargo_metadata|cargo_metadata::|compiler-artifact|compiler-message|JsonMessage|Message" . -g '*.rs' -g 'Cargo.toml' | sed -n '1,240p'Repository: EffortlessMetrics/lintdiff
Length of output: 45739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'crates/lintdiff-engine/src/diagnostics/mod.rs' 'crates/*Cargo.toml' 'Cargo.toml' | sed -n '1,80p'
echo
echo "== diagnostics outline =="
ast-grep outline crates/lintdiff-engine/src/diagnostics/mod.rs --view expanded 2>/dev/null | sed -n '1,160p' || true
echo
echo "== relevant diagnostics lines =="
cat -n crates/lintdiff-engine/src/diagnostics/mod.rs | sed -n '480,560p'
echo
echo "== DiagnosticObservation and related types =="
rg -n "struct DiagnosticObservation|profile:|opt_level|Compiler|compiler-" crates/lintdiff-engine/src/diagnostics/mod.rs crates/lintdiff-engine/src -g '*.rs' | sed -n '1,220p'
echo
echo "== search for cargo message crate usage =="
rg -n "cargo_metadata|cargo_metadata::|compiler-artifact|compiler-message|JsonMessage|Message" . -g '*.rs' -g '*.toml' | sed -n '1,240p'Repository: EffortlessMetrics/lintdiff
Length of output: 50382
🌐 Web query:
docs.rs cargo_metadata ArtifactProfile fields rustdoc
💡 Result:
In the cargo_metadata crate, the ArtifactProfile struct represents the profile settings used to determine compiler flags for a target [1]. This struct is marked as #[non_exhaustive], meaning its definition may change in future versions [1]. As of the latest documentation, ArtifactProfile contains the following fields [1][2]: - opt_level (String): The optimization level; common values include 0-3, s, or z [1][2]. - debuginfo (ArtifactDebuginfo): The type of debug information included in the artifact [1][2]. - debug_assertions (bool): The state of the cfg(debug_assertions) directive, which controls macros like debug_assert! [1][2]. - overflow_checks (bool): The state of overflow checks [1][2]. - test (bool): Indicates whether the profile is for a test [1][2]. Because the struct is marked as non-exhaustive, it cannot be constructed using traditional struct syntax and cannot be matched against without a wildcard (..), and struct update syntax is not supported [1]. A builder pattern is available if the builder feature is enabled in the crate [3].
Citations:
- 1: https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.ArtifactProfile.html
- 2: https://docs.rs/cargo_metadata/latest/src/cargo_metadata/messages.rs.html
- 3: https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.ArtifactProfileBuilder.html
Preserve the full Cargo profile in ProducerUnit.
parse_producer stores only profile.opt_level in producer.profile, but Cargo compiler-artifact profile objects also include debuginfo, debug_assertions, overflow_checks, and test. Rename this field to opt_level, or replace it with a dedicated profile struct that preserves all fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` around lines 538 - 543, Update
parse_producer so ProducerUnit does not expose the full Cargo profile under the
misleading profile field while retaining only opt_level; rename the stored field
and its consumers to opt_level, or introduce a dedicated profile structure that
captures opt_level, debuginfo, debug_assertions, overflow_checks, and test.
| fn repository_path(raw_file_name: Option<&str>, repo_root: Option<&Path>) -> Option<NormPath> { | ||
| let raw_file_name = raw_file_name?; | ||
| let repo_root = repo_root?; | ||
| let raw = Path::new(raw_file_name); | ||
| if raw.is_absolute() { | ||
| let relative = raw.strip_prefix(repo_root).ok()?; | ||
| relative.to_str().map(NormPath::from_repo_path) | ||
| } else { | ||
| Some(NormPath::from_repo_path(raw_file_name)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Path normalization relies on host-OS std::path semantics. repository_path uses Path::is_absolute and Path::strip_prefix, which interpret drive letters and backslashes only on Windows. The same Cargo stream therefore normalizes differently per platform, and the accompanying test encodes the Windows result as the expected value.
crates/lintdiff-engine/src/diagnostics/mod.rs#L644-L654: convert separators to forward slashes first, then strip the normalized repository root by string prefix instead ofPath::strip_prefix.crates/lintdiff-engine/src/diagnostics/mod.rs#L841-L855: keep the Windows fixture and add a POSIX fixture, so both cases assertsrc/lib.rson every platform.
As per coding guidelines: "Maintain deterministic outputs: same inputs must produce byte-identical outputs with stable ordering and reproducible truncation".
📍 Affects 1 file
crates/lintdiff-engine/src/diagnostics/mod.rs#L644-L654(this comment)crates/lintdiff-engine/src/diagnostics/mod.rs#L841-L855
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` around lines 644 - 654, Make
repository_path platform-independent by normalizing both raw_file_name and
repo_root separators to forward slashes, detecting absolute paths using the
normalized representation, and removing the normalized repository root via
string-prefix matching rather than Path::strip_prefix; update
crates/lintdiff-engine/src/diagnostics/mod.rs lines 644-654 accordingly. In the
test fixture at crates/lintdiff-engine/src/diagnostics/mod.rs lines 841-855,
retain the Windows-style case and add a POSIX-style case, with both asserting
the normalized result src/lib.rs.
Source: Coding guidelines
| let span = observation.spans.first().ok_or("missing span")?; | ||
| assert_eq!(span.raw_line_start, Some(0)); | ||
| assert!(span.normalized.is_none()); | ||
| assert_eq!(observation.diagnostic.spans[0].line_start, 1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This assertion does not verify the line_start > 0 guard.
parse_cargo_analysis passes repo_root = None. repository_path then returns None for every input, so normalized is None regardless of raw_line_start. The assertion at line 829 passes for any span value.
Use parse_cargo_analysis_with_repo_root with a root, and assert that a span with line_start of 0 yields normalized == None while a sibling span with a valid line yields Some.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lintdiff-engine/src/diagnostics/mod.rs` around lines 827 - 830, Update
the diagnostic test around parse_cargo_analysis to use
parse_cargo_analysis_with_repo_root with a repository root, then provide sibling
spans covering line_start 0 and a valid positive line. Assert the zero-based
span has normalized == None and the valid-line span has normalized == Some, so
the line_start > 0 guard is exercised rather than masked by a missing repository
path.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab6a565bd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let raw_file_name = raw_file_name?; | ||
| let repo_root = repo_root?; | ||
| let raw = Path::new(raw_file_name); | ||
| if raw.is_absolute() { |
There was a problem hiding this comment.
When Windows-generated JSON is parsed on Ubuntu or macOS, std::path::Path does not recognize C:\repo\... as absolute, so this condition falls through and preserves the full C:/repo/src/lib.rs path instead of stripping the supplied root. The newly added Windows-path unit test therefore fails in the repository's Ubuntu cargo test --all-features job, and stored cross-platform analyses cannot normalize correctly; detect drive-letter and UNC paths using host-independent syntax before relativizing them.
Useful? React with 👍 / 👎.
| .get("target") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string); | ||
| scope.features = string_array(value.get("features")); |
There was a problem hiding this comment.
For a normal Cargo 1.95 JSON stream, build-finished carries only reason and success; enabled features and profile data are emitted on compiler-artifact, which this parser discards. Consequently scope.features remains an indistinguishable empty vector and producer.profile remains None even for analyses run with different features or profiles, preventing the hard-scope comparability checks this model is intended to support. Collect and correlate artifact evidence, while distinguishing unavailable features from a known-empty feature set.
Useful? React with 👍 / 👎.
| pub raw_file_name: Option<String>, | ||
| pub raw_line_start: Option<u32>, | ||
| pub raw_line_end: Option<u32>, | ||
| pub raw_column_start: Option<u32>, | ||
| pub raw_column_end: Option<u32>, |
There was a problem hiding this comment.
For diagnostics containing macro expansions, labels, or precise source evidence, this supposedly raw observation retains only file and line/column values and irreversibly drops Cargo's byte offsets, source text/highlights, label, expansion chain, and top-level suggestion fields. Because the original JSON is not retained elsewhere, downstream inventory and source-correspondence logic cannot distinguish macro call sites or reconstruct the evidence needed by the macro_generated_path case; extend the typed span model or retain the complete raw span payload.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
Closes #157.
Proof
Claim boundary
This PR provides the evidence spine needed by #158 and #102. It does not serialize inventory.v1, change current report construction, implement source correspondence or pairing, or authorize release/tag/publication.