P-015: reachability codeFlows for flow-local + DI-sourced subscription findings#125
Conversation
…n findings Extends the codeFlows reachability slice (shipped for DI captives in #119) to the other detectors that carry a path, so a SARIF/IDE consumer can answer "where did this come from, and where does it go wrong?" beyond just DI: - flow-local (POOL / local IDisposable): OWN002/003/009/025 now emit a 2-step slice — the Rent/acquire site (origin) -> the violation site (use-after-return, double-return, over-read). A plain OWN001 leak stays a single point (the acquire is its only locus) and gets no flow. New helper `_flow_local_steps`. - DI-sourced subscription escape (OWN014): a cross-file slice — the subscribe site -> where the longer-lived source service was registered (its lifetime is *why* the subscriber escapes), resolved from the services graph. Purely additive: relatedLocations and every existing field/message are unchanged; codeFlows only appears on findings that now carry a path. The static-source `capture` case stays single-point (no second line to point at — honest). Tests: flow-local OWN002 slice on the handoff fixture (acquired -> used-after), and a cross-file DI-source OWN014 escape slice. ownir 166/166; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml 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)
📝 WalkthroughWalkthroughOwnIR now attaches structured reachability-flow evidence to flow-local ownership findings and to OWN014 DI-sourced escape findings. The test suite adds assertions for the new ChangesOwnership finding flow evidence
Sequence Diagram(s)sequenceDiagram
participant check_facts
participant _flow_local_steps
participant Finding
participant svc_loc
check_facts->>_flow_local_steps: build flow slice for OWN001/002/003/025
_flow_local_steps-->>check_facts: origin to manifestation steps
check_facts->>svc_loc: map DI services to registration sites
check_facts->>Finding: attach flow to OWN025 and flow-local findings
check_facts->>Finding: attach esc_flow to OWN014
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.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: d43f89f643
ℹ️ 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".
| component=component, event=name, handler="", message=msg, | ||
| kind="pooled buffer" if pool else "disposable")) | ||
| kind="pooled buffer" if pool else "disposable", | ||
| flow=_flow_local_steps(sub, d.code, d.line, bool(pool)))) |
There was a problem hiding this comment.
When the same local has multiple violations with the same acquire site, code, and message, this newly added flow is the only field that distinguishes them, but the later de-duplication key does not include flow. For example, acquire s; release s; use s on line 3; use s on line 4 produces two core OWN002 diagnostics, yet check_facts keeps only the first flow and silently drops the second violation site, so SARIF points to only one bad use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — the premise doesn't reproduce. Two checks:
1. The core emits one OWN002 per local, not two. The loan/permission model reports use-after-release once (at the first offending use), so there is no second finding to drop. Verified on exactly your scenario:
acquire s @3; release s @4; use s @5; use s @6
→ check_facts returns 1 finding:
OWN002 anchor 3 | "IDisposable local 's' is used after it is disposed"
flow: (F.cs:3 "acquired 's' here", F.cs:5 "used here after it was released/returned")
The second use @6 was already not separately reported before this PR — that's pre-existing precision-first behaviour, unrelated to flow.
2. There is no flow-blind de-duplication. Neither check_facts nor build_sarif dedupes findings — every diagnostic becomes its own Finding and every Finding its own SARIF result. So even if two identical-except-flow findings did arise, both would be emitted; nothing is silently collapsed.
So no behaviour change is needed here. (Reporting all use-after-release sites for one local would be a deliberate change to the core analysis's one-finding-per-variable policy — out of scope for this slice, and a separate precision/noise tradeoff.)
Generated by Claude Code
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ownlang/ownir.py (1)
1667-1668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOWN025 flow slice is untested. The general flow-local path is exercised by the OWN002 handoff assertion, but this pooled-buffer branch (
pool=Truehardcoded, primary anchor atd.linewhile the flow's first hop is the Rent sitesub["line"]) has noflowassertion — theosptest only checks(code, line, kind). A regression in the Rent→view slice would slip through.Want me to add a
flowassertion to theosptest (expecting(("Framer.cs", 10, "rented 'buf' here"), ("Framer.cs", 12, ...)))?🤖 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/ownir.py` around lines 1667 - 1668, The pooled-buffer branch in the osp test is missing coverage for the flow slice, so a Rent→view regression could go unnoticed. Update the osp assertion to also verify the flow returned by _flow_local_steps for this pooled-buffer path, using the relevant sub["line"] and d.line anchors from ownir.py so the test checks both the primary tuple fields and the expected flow chain.
🤖 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.
Nitpick comments:
In `@ownlang/ownir.py`:
- Around line 1667-1668: The pooled-buffer branch in the osp test is missing
coverage for the flow slice, so a Rent→view regression could go unnoticed.
Update the osp assertion to also verify the flow returned by _flow_local_steps
for this pooled-buffer path, using the relevant sub["line"] and d.line anchors
from ownir.py so the test checks both the primary tuple fields and the expected
flow chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5846f5d8-eb61-40d2-a5b0-9b18d819b208
📒 Files selected for processing (2)
ownlang/ownir.pytests/test_ownir.py
The flow-local OWN002 path was asserted via the handoff fixture, but the OWN025 pooled-buffer branch (pool=True, primary anchor at the view site while the flow begins at the Rent site) had no flow assertion. Pin its Rent->view slice so a regression in that distinct branch can't slip through. ownir 167/167; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
2 participants
What & why
Step "A" of the P-015 follow-up: point the codeFlows engine (shipped for DI captives in #119) at the other detectors that carry a path, so a finding answers "where did this come from, and where does it go wrong?" beyond just DI.
Changes (
ownlang/ownir.py)IDisposable) —OWN002/003/009/025now emit a 2-step slice: the Rent/acquire site (origin) → the violation site (use-after-return, double-return, over-read). A plainOWN001leak stays a single point (the acquire is its only locus) and gets no flow. New helper_flow_local_steps.OWN014) — a cross-file slice: the subscribe site → where the longer-lived source service was registered (its lifetime is why the subscriber escapes), resolved from theservicesgraph.capturecase stays single-point (no second line to point at — honest).Example slices (verified end-to-end)
Testing
python tests/run_tests.py→ green (ownir: 166/166,diagnostics: PASS)ruff check .clean,mypycleanPurely additive —
relatedLocationsand every existing field/message are unchanged;codeFlowsonly appears on findings that now carry a path.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit