Emit bounded AST lowering metrics (7.3.2) (#308) - #310
Conversation
Record one latency sample and one categorized outcome for every lowering attempt, with parser recovery tracked orthogonally.
Keep adapter metrics scenarios separate from syntax-lowering tests so both modules remain below the repository file-size ceiling.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughChangesAdd bounded AST lowering metrics for duration, categorised outcomes, and parser recovery. Instrument both parser-enabled and parser-disabled paths, wire shared dependencies, and add scenario-based validation and design documentation. AST lowering metrics
Sequence Diagram(s)sequenceDiagram
participant lower_span
participant lower_span_inner
participant record_lower_span_metrics
lower_span->>lower_span_inner: Lower the byte span
lower_span_inner-->>lower_span: Return result and recovery flag
lower_span->>record_lower_span_metrics: Submit duration, result, and recovery
record_lower_span_metrics-->>lower_span: Record bounded metrics
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (17 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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: 02aaf844dc
ℹ️ 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".
| } | ||
|
|
||
| fn describe_metrics() { | ||
| DESCRIBE_METRICS.call_once(|| { |
There was a problem hiding this comment.
The process-global Once permanently sends these descriptions only to whichever recorder handles the first lower_span call. If that call occurs before the production recorder is installed, or inside with_local_recorder as in these tests, later measurements reach the production recorder without their unit and purpose metadata. Register the descriptions after recorder initialization or avoid a process-wide guard so each active recorder receives them.
AGENTS.md reference: AGENTS.md:L299-L300
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/whitaker_clones_core/src/ast/lowering.rs (1)
72-135: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCollapse the three early-return matches back into a
?-chain.
lower_span_innernow carries three duplicatedmatch { Ok(x) => x, Err(error) => return (Err(error), recovered) }blocks purely to smugglerecoveredout alongside the result. Capturerecoveredby mutation from an inner closure and let?do the propagation — same behaviour, far fewer branches to hold in your head at once.♻️ Proposed refactor
fn lower_span_inner(file_text: &str, span: ByteSpan) -> (AstResult<NormalizedTree>, bool) { - let span = match ByteSpan::new(file_text, span.start(), span.end()).map_err(|error| { - trace_ast_error( - error, - "AST span lies outside the supplied source text", - "AST span validation failed", - ) - }) { - Ok(span) => span, - Err(error) => return (Err(error), false), - }; - let parse = SourceFile::parse(file_text, Edition::CURRENT); - let parse_errors = parse.errors(); - let recovered = !parse_errors.is_empty(); - if recovered { - warn!( - start = span.start(), - end = span.end(), - errors = parse_errors.len(), - "lowered AST span from source with parser recovery errors" - ); - } - let root = parse.tree().syntax().clone(); - let target_range = text_range(span); - let selected = match select_covering_node(&root, span).map_err(|error| { - trace_ast_error( - error, - "no AST syntax node covers the requested span", - "AST covering-node selection failed", - ) - }) { - Ok(selected) => selected, - Err(error) => return (Err(error), recovered), - }; - debug!( - kind = ?selected.kind(), - span_width = u32::from(selected.text_range().len()), - "selected AST covering node" - ); - let lowered = match LoweringLimits::new(span) - .lower(&selected, 0) - .map_err(|error| { - if matches!(error, AstError::UnparsableSpan { .. }) { - error!( - start = span.start(), - end = span.end(), - "selected AST span contains parser error elements" - ); - } - error - }) { - Ok(lowered) => lowered, - Err(error) => return (Err(error), recovered), - }; - debug_assert!(selected.text_range().contains_range(target_range)); - (Ok(NormalizedTree::new(lowered, span)), recovered) + let mut recovered = false; + let result = (|| -> AstResult<NormalizedTree> { + let span = ByteSpan::new(file_text, span.start(), span.end()).map_err(|error| { + trace_ast_error( + error, + "AST span lies outside the supplied source text", + "AST span validation failed", + ) + })?; + let parse = SourceFile::parse(file_text, Edition::CURRENT); + let parse_errors = parse.errors(); + recovered = !parse_errors.is_empty(); + if recovered { + warn!( + start = span.start(), + end = span.end(), + errors = parse_errors.len(), + "lowered AST span from source with parser recovery errors" + ); + } + let root = parse.tree().syntax().clone(); + let target_range = text_range(span); + let selected = select_covering_node(&root, span).map_err(|error| { + trace_ast_error( + error, + "no AST syntax node covers the requested span", + "AST covering-node selection failed", + ) + })?; + debug!( + kind = ?selected.kind(), + span_width = u32::from(selected.text_range().len()), + "selected AST covering node" + ); + let lowered = LoweringLimits::new(span).lower(&selected, 0).map_err(|error| { + if matches!(error, AstError::UnparsableSpan { .. }) { + error!( + start = span.start(), + end = span.end(), + "selected AST span contains parser error elements" + ); + } + error + })?; + debug_assert!(selected.text_range().contains_range(target_range)); + Ok(NormalizedTree::new(lowered, span)) + })(); + (result, recovered) }As per path instructions, "Seek to keep the cognitive complexity of functions no more than 9."
🤖 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/whitaker_clones_core/src/ast/lowering.rs` around lines 72 - 135, Refactor lower_span_inner to eliminate the duplicated early-return match blocks by capturing recovered through a mutable outer variable from an inner closure, then use ? for ByteSpan validation, covering-node selection, and lowering propagation. Preserve parser-recovery logging, error-specific logging, and the existing (Err(error), recovered) return behavior.Source: Path instructions
🤖 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 `@docs/whitaker-clone-detector-design.md`:
- Around line 284-289: Update the documentation sentence to use an Oxford comma
in “individual nodes, spans, paths, or raw source text” and insert a comma
before “so” in “SARIF Run 1 consumer, so a vector is counted.”
---
Outside diff comments:
In `@crates/whitaker_clones_core/src/ast/lowering.rs`:
- Around line 72-135: Refactor lower_span_inner to eliminate the duplicated
early-return match blocks by capturing recovered through a mutable outer
variable from an inner closure, then use ? for ByteSpan validation,
covering-node selection, and lowering propagation. Preserve parser-recovery
logging, error-specific logging, and the existing (Err(error), recovered) return
behavior.
🪄 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: 77a98852-4a3f-4464-bed0-481819dc3dc0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/whitaker_clones_core/Cargo.tomlcrates/whitaker_clones_core/src/ast/lowering.rscrates/whitaker_clones_core/src/ast/lowering_metrics_tests.rscrates/whitaker_clones_core/src/ast/metrics.rscrates/whitaker_clones_core/src/ast/mod.rsdocs/whitaker-clone-detector-design.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)
| The outcome label is limited to `success`, `invalid_span`, `unparsable_span`, | ||
| `node_budget_exhausted`, `depth_budget_exhausted` and `parser_unavailable`. | ||
| Metrics must not label individual nodes, spans, paths or raw source text. | ||
| Feature-vector emission remains owned by the 7.3.2 scoring and SARIF Run 1 | ||
| consumer so a vector is counted only when that boundary consumes it. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add the Oxford comma and a comma before "so".
Triage: [type:grammar]
Line 286: "individual nodes, spans, paths or raw source text" needs the Oxford comma before "or". Line 288: "SARIF Run 1 consumer so a vector is counted" needs a comma before "so" since it joins two independent clauses.
✏️ Proposed fix
-Metrics must not label individual nodes, spans, paths or raw source text.
-Feature-vector emission remains owned by the 7.3.2 scoring and SARIF Run 1
-consumer so a vector is counted only when that boundary consumes it.
+Metrics must not label individual nodes, spans, paths, or raw source text.
+Feature-vector emission remains owned by the 7.3.2 scoring and SARIF Run 1
+consumer, so a vector is counted only when that boundary consumes it.As per coding guidelines, "Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension."
📝 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.
| The outcome label is limited to `success`, `invalid_span`, `unparsable_span`, | |
| `node_budget_exhausted`, `depth_budget_exhausted` and `parser_unavailable`. | |
| Metrics must not label individual nodes, spans, paths or raw source text. | |
| Feature-vector emission remains owned by the 7.3.2 scoring and SARIF Run 1 | |
| consumer so a vector is counted only when that boundary consumes it. | |
| The outcome label is limited to `success`, `invalid_span`, `unparsable_span`, | |
| `node_budget_exhausted`, `depth_budget_exhausted` and `parser_unavailable`. | |
| Metrics must not label individual nodes, spans, paths, or raw source text. | |
| Feature-vector emission remains owned by the 7.3.2 scoring and SARIF Run 1 | |
| consumer, so a vector is counted only when that boundary consumes it. |
🧰 Tools
🪛 LanguageTool
[style] ~286-~286: The serial comma (Oxford comma, Harvard comma) is missing.
Context: ...must not label individual nodes, spans, paths or raw source text. Feature-vector emissio...
(SERIAL_COMMA_ON)
[uncategorized] ~288-~288: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e 7.3.2 scoring and SARIF Run 1 consumer so a vector is counted only when that boun...
(COMMA_COMPOUND_SENTENCE_2)
🤖 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 `@docs/whitaker-clone-detector-design.md` around lines 284 - 289, Update the
documentation sentence to use an Oxford comma in “individual nodes, spans,
paths, or raw source text” and insert a comma before “so” in “SARIF Run 1
consumer, so a vector is counted.”
Sources: Coding guidelines, Linters/SAST tools
Summary
This branch adds bounded operational metrics for AST span lowering so roadmap
task 7.3.2 can observe latency and categorized results without exposing source,
span, path or per-node data. It centralizes the stable metric vocabulary,
instruments every parser and no-parser exit path, and proves the emitted values
with a local debugging recorder.
Closes #308.
Roadmap task: (7.3.2)
Review walkthrough
AstErrormapping and recorder helpers.Validation
make check-fmt: passed.make lint: passed, including Rustdoc and Clippy with warnings denied.make test: passed, 1,592 tests with 3 skipped by the configured filter.make markdownlint: passed.make nixie: passed.mbake validate Makefile: passed.cargo test -p whitaker_clones_core --no-default-features --lib parser_unavailable_stub_records_outcome_and_latency: passed.coderabbit review --agent: passed with zero findings after the implementation and test-module split milestones.git diff --check: passed.Notes
Feature-vector emission remains reserved for the 7.3.2 scoring and SARIF Run 1
consumer. This branch makes lowering latency and categorized outcomes observable
now, while ensuring a future vector metric counts vectors only when that
consumer actually uses them.
References