P-015: structured reachability-slice evidence for diagnostics#118
Conversation
Add the "explain the path, not just the point" layer the ReachHover/ Optional-Checker research synthesis identifies as the highest-leverage, lowest-footprint P0 for Own.NET: - ownlang/evidence.py: a pure, dependency-free transform turning an ordered chain of (file, line, label) program points into SARIF relatedLocations (unordered secondary anchors) and codeFlows (the ordered reachability slice, e.g. a DI captive's singleton -> transient -> scoped retention path). One vocabulary every producer (OwnIR DI checker, ownership checker, future XAML->.g.cs join) emits and every consumer reads. - ownlang/diagnostics.py: Diagnostic gains a structured `evidence` slice (the typed successor to the textual `[consumed by ... at file:line]` riders), rendered as `note:` lines in both the plain and rustc-style surfaces. Additive and backward compatible (empty -> identical single-line output). - docs/proposals/P-015-reachability-evidence.md: the research distillation, the precise gap (codeFlows absent though relatedLocations already present), and the staged plan incl. the one-import ownir.build_sarif wiring follow-up.
Reviewer orientationThe one idea: a finding should answer "why is this held, and through what?" — not just "something is wrong here." That's a reachability slice. The DI checker already computes the retention path ( Where to look first: Safety claim to check: the Known follow-up (not in this PR): the ~10-line Companion PR: PhysShell/OwnAudit#22 (same slice model in the audit aggregator). Generated by Claude Code |
- import Iterable from collections.abc, not typing (ruff UP035). - parametrize every dict/list return as dict[str, Any] (mypy --strict's --disallow-any-generics rejects bare generics), matching ownir.py's idiom.
|
Warning Review limit reached
More reviews will be available in 15 minutes and 14 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds the P-015 reachability-evidence proposal, a new ChangesReachability Evidence
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ownlang/diagnostics.py (1)
159-190: 📐 Maintainability & Code Quality | 🟡 MinorAdd regression coverage for Diagnostic rendering
- Add a test for
evidence == ()that keeps bothrender()andrender_pretty()unchanged.- Add one ordered
note:assertion to lock the evidence slice output.🤖 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 `@ownlang/diagnostics.py` around lines 159 - 190, Add regression tests for Diagnostic rendering in diagnostics.py, covering the _evidence_lines(), render(), and render_pretty() paths. Verify that when evidence is empty (evidence == ()), both render() and render_pretty() remain unchanged from the existing plain output, and add a single ordered note: assertion to pin the evidence slice output order for a non-empty evidence case. Use the Diagnostic class and its render/render_pretty behavior as the main anchors when locating the code.
🤖 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.
Outside diff comments:
In `@ownlang/diagnostics.py`:
- Around line 159-190: Add regression tests for Diagnostic rendering in
diagnostics.py, covering the _evidence_lines(), render(), and render_pretty()
paths. Verify that when evidence is empty (evidence == ()), both render() and
render_pretty() remain unchanged from the existing plain output, and add a
single ordered note: assertion to pin the evidence slice output order for a
non-empty evidence case. Use the Diagnostic class and its render/render_pretty
behavior as the main anchors when locating the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c2615ee8-de02-44fb-8e38-7da27230fccc
📒 Files selected for processing (3)
docs/proposals/P-015-reachability-evidence.mdownlang/diagnostics.pyownlang/evidence.py
Preempt the same issue Codex flagged on the OwnAudit side: a step with a line but an empty file would serialize as artifactLocation.uri: "", which makes a SARIF log unprocessable for GitHub code scanning. The builders now require both a resolvable line AND a non-empty file (the caller is responsible for resolving the diagnostics.Evidence "same file as anchor" convention to a concrete path before building, exactly as ownir.Finding already carries concrete paths).
Addresses CodeRabbit's coverage request on PR #118: - the empty-evidence invariant: render() and render_pretty() are byte-for-byte the prior plain output when evidence == () (the common case); - a populated slice appends one ordered `note:` line per step, in order, after the caret block; the same-file convention (Evidence.file is None) renders the diagnostic's own filename; - plus a smoke check of the ownlang.evidence SARIF builders (lineless/empty-file steps dropped; di_path_steps labels captor and captured). Standalone and zero-dependency, matching the tests/ convention (run() -> int, runnable as `python tests/test_diagnostics.py`). Folding it into the tests/run_tests.py suite aggregator is a one-line follow-up (an import + an `or diag_rc` in the final return), left for a local edit since run_tests.py is large and edited best where the suite can be run.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_diagnostics.py`:
- Around line 50-55: The empty-evidence check in render_pretty is too weak and
should assert the full output exactly, not just the absence of note lines or
that it ends with a caret. Update the test around d.render_pretty("m.own",
_SOURCE) to compare the entire rendered string against the expected
byte-for-byte output for the empty-evidence case, so regressions in the header,
gutter, or caret spacing are caught.
- Around line 113-118: The current diagnostics test only checks the literal
empty-input case for evidence.code_flow(), so add a case where all provided
steps are filtered out and verify the result is still an empty list rather than
a non-empty flow with no locations. Update the test around evidence.code_flow
and the existing locs/flow assertions to cover this “all steps filtered out”
scenario explicitly, using the same code_flow helper and steps fixture pattern
already in the 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b473147-7f72-4dc5-bfab-76b388f33c0d
📒 Files selected for processing (1)
tests/test_diagnostics.py
- empty-evidence invariant: pin the FULL render_pretty() output byte-for-byte (header + gutter + caret), not just "no note: lines / ends with caret", so a header/gutter/caret-spacing regression is caught. (NB: the caret indent is 14 spaces, not 10 as the review's literal suggested — `b` is at column 9 and the gutter " 3 | " is 6 chars, so 6 + 9 - 1 = 14.) - code_flow(): add the "all steps filtered out" case (every step dropped for empty file / no line) returns [], beyond the literal empty-input case; add the symmetric related_locations() case too.
1 participant
What & why
Distillation of the ReachHover / Optional-Checker research exchange into the one concrete, highest-leverage P0 it actually implies for Own.NET: explain the path, not just the point.
The key realisation: the "fact-based frontends → one core → explainable diagnostics" architecture the review recommends already exists here (
ownir.pyfacts, one core, several analyses). So the recommendation collapses to a single gap —relatedLocationsare already emitted, but there are nocodeFlows: the ordered retention path a finding already computes (e.g. a DI captive'ssingleton → transient → scoped) is dropped into the message string instead of a structured slice a SARIF/IDE consumer can walk. That ordered slice is the ReachHover idea.Changes
ownlang/evidence.py(new) — pure, dependency-free transform from ordered(file, line, label)steps to SARIFrelatedLocations+codeFlows;di_path_steps()turns a DI dependency path into a registration-site-anchored slice. One vocabulary for every producer.ownlang/diagnostics.py—Diagnosticgains a structuredevidenceslice (Evidence(line, label, file, role)), the typed successor to the textual[consumed by … at file:line]riders, rendered asnote:lines in both surfaces. Additive & backward compatible: empty slice → byte-for-byte identical output.docs/proposals/P-015-reachability-evidence.md— the research distillation, the precise gap, and the staged P0–P4 plan.Deliberately out of scope (next patch)
Wiring the slice through
ownir.build_sarif(flowfield onownir.Finding+_di_findingsbuildingloc_by_nameand callingevidence.di_path_steps). It's ~10 lines butownir.pyis 1700 lines with golden-file SARIF tests, so it should be edited where it can be run against them, not rewritten wholesale over the API. Exact steps are in P-015.Testing
Not run in this environment (no local clone/build). Changes were kept additive and backward-compatible precisely for that reason: no existing rendering or SARIF output changes unless a producer attaches evidence.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
note:lines, and can be projected into SARIF as related locations and ordered code flow when available.Documentation
Tests
note:placement and ordering) and SARIF projection behavior.