P-015: carry reachability slices through the SARIF aggregator - #22
Conversation
Mirror the Own.NET reachability-evidence model in the audit lift-out so a
normalized finding can explain its path, not just its point:
- report/sarif.py: each result now emits relatedLocations from a finding's
optional `evidence` list and a codeFlows reachability slice from its optional
`flow` list (each item {path,line,label}). Purely additive and
forward-compatible -- findings without these keys produce byte-for-byte the
same SARIF, so existing fixtures are unaffected; when upstream normalization
starts attaching them (e.g. a DI captive's singleton -> transient -> scoped
retention path from own-check's OwnIR), they flow straight through to GitHub
code scanning / IDE hovers.
- src/OwnAudit.Core/Finding.cs: add an EvidenceSpan record and structured
Evidence (relatedLocations) + ordered Flow (codeFlows) members, default empty.
See Own.NET docs/proposals/P-015 for the full design and the gap analysis.
Reviewer orientationCompanion to PhysShell/Own.NET#118. This side just teaches the SARIF aggregator to pass a reachability slice through to GitHub code scanning / IDE hovers. Where to look: Safety claim to check: purely additive. Forward path: when own-check's OwnIR findings start carrying Generated by Claude Code |
![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: 03ef868764
ℹ️ 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".
| continue | ||
| if ln < 1: | ||
| continue | ||
| out.append((s.get("path") or "", ln, s.get("label") or "")) |
There was a problem hiding this comment.
When a producer sends an evidence or flow step that has a line but no path, this keeps the step and later emits artifactLocation.uri: "". That includes the new EvidenceSpan.File == "" same-file convention documented in Finding.cs; instead of resolving it to the primary finding path or dropping the malformed anchor, one optional evidence item can make the GitHub SARIF upload unprocessable because it contains an empty artifact location.
Useful? React with 👍 / 👎.
… URI Addresses Codex review (PR #22): an evidence/flow step with a line but no path was kept and serialized as artifactLocation.uri: "", which makes the whole SARIF upload unprocessable for GitHub code scanning. Now a step's empty path is first resolved to the parent finding's path (the EvidenceSpan.File == "" "same file as the finding" convention documented in Finding.cs); a step that still has no usable path is dropped, so one malformed optional step can no longer poison the export.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds structured evidence and flow data to findings, then exports those locations into SARIF as ChangesReachability evidence export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🤖 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 `@report/sarif.py`:
- Around line 104-113: The evidence step parser in `_evidence_steps()` only
reads `path`, so it drops spans coming from the new producer model that uses
`File` instead. Update the span extraction in `report/sarif.py` to accept `File`
as a fallback alongside `path` (using the existing `default_path` behavior), and
keep the rest of the validation in `s.get("line")` handling unchanged so
`relatedLocations` and `codeFlows` can emit correctly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34a2f97e-c02c-428c-b68e-7a589d2ed8d7
📒 Files selected for processing (2)
report/sarif.pysrc/OwnAudit.Core/Finding.cs
Addresses CodeRabbit review (PR #22, Major): _evidence_steps() read only the lowercase `path`/`line`/`label` keys, but the producer model in Finding.cs names them File/Line/Label. A span serialized straight from EvidenceSpan would have every step silently dropped, so relatedLocations/codeFlows would never emit. Read each field case-insensitively (path|file|File, line|Line, label|Label) so the aggregator tolerates either the normalized lowercase record or a raw EvidenceSpan dump, while the line/empty-path validation is unchanged (a step still needs a resolvable line and a non-empty path, the same-file convention still resolves to the finding's own path).
1 participant
What & why
Mirrors the Own.NET reachability-evidence model (P-015) in the audit lift-out so a normalized finding can explain its path, not just its point. Companion to physshell/own.net
claude/own-net-research-synthesis-8akz2e.Background: a review of the ReachHover (data-flow reachability UI) + Optional-Checker (modular contracts) research reframed the work from "Rust borrow checker compiling to C#" to reachability-oriented diagnostics + modular contract checking for C#/WPF resource & lifetime analysis. The first, highest-leverage step is structured, walkable evidence.
Changes
report/sarif.py— each result now emitsrelatedLocationsfrom a finding's optionalevidencelist and acodeFlowsreachability slice from its optionalflowlist (each item{path, line, label}). Purely additive & forward-compatible: findings without these keys produce byte-for-byte the same SARIF (existing fixtures unaffected); when upstream normalization starts attaching them (e.g. a DI captive'ssingleton → transient → scopedpath from own-check's OwnIR), they flow straight through to GitHub code scanning / IDE hovers.src/OwnAudit.Core/Finding.cs— add anEvidenceSpanrecord + structuredEvidence(relatedLocations) and orderedFlow(codeFlows) members, default empty.Testing
Not run in this environment (no local clone/build). Kept additive/backward-compatible so existing SARIF output is unchanged unless a producer attaches evidence/flow.
See Own.NET
docs/proposals/P-015-reachability-evidence.mdfor the full design and gap analysis.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes