P-015: emit DI captive codeFlows + fold diagnostics tests into the suite#119
Conversation
The reachability-slice follow-up to PR #118, now that the evidence builders (ownlang/evidence.py) are on main: wire them through the OwnIR SARIF path and run the diagnostics tests in CI. - ownlang/ownir.py: Finding gains an ordered `flow` field; _sarif_result emits it as a SARIF `codeFlows` (the step-through retention trace relatedLocations cannot express); _di_findings builds each DI001-005 flow from the captive's dependency `path` and a name->registration-site map via evidence.di_path_steps. A DI captive now ships both its consuming-ctor relatedLocation AND the ordered singleton -> [transient ->]* scoped retention slice. - tests/test_ownir.py: assert the EmailSender direct (2-hop) and ReportService transitive (3-hop, transient pass-through) codeFlows, on top of the existing relatedLocations checks (now 151 bridge checks). - tests/run_tests.py: fold tests/test_diagnostics.py (added in #118 but not yet aggregated) into the suite so the empty-evidence invariant and slice ordering run in CI. Local: full suite green (ownir 151/151, diagnostics PASS), ruff + mypy --strict clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
Reviewer orientationFollow-up to #118 — turns the now-merged evidence builders into live SARIF output. The payload: a DI captive's SARIF result now carries Where to look: Verified locally (against a checkout, which is why this could land vs. #118's deferral): Generated by Claude Code |
|
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)
📝 WalkthroughWalkthroughThe PR threads ordered DI reachability traces through findings, emits SARIF ChangesDI reachability flow plumbing
Sequence Diagram(s)sequenceDiagram
participant _di_findings
participant Finding
participant code_flow
participant SARIF_result
_di_findings->>Finding: set flow steps
Finding->>code_flow: flow tuple
code_flow->>SARIF_result: codeFlows
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_ownir.py (1)
824-833: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider adding flow/codeFlows coverage for DI002–DI005.
The PR wires
flow=di_path_steps(...)into DI002, DI003, DI004, and DI005 findings, but the new regression assertions only pin DI001 (em/rs). The other four families—particularly DI004/DI005, where the finding anchors at the call/store site while the flow begins at the registration site—now emitcodeFlowswith no test guarding the ordering or the per-familyend_label("weakly captures scoped service", "leaks transient IDisposable", "caches scoped service"). A regression there would slip through.Want me to draft
flowassertions for the existing DI002–DI005 bridge fixtures?🤖 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 `@tests/test_ownir.py` around lines 824 - 833, Add regression coverage for the remaining DI finding families in the existing ownir tests, not just DI001. Extend the assertions around the fixture checks that exercise DI002, DI003, DI004, and DI005 so they validate the full flow/codeFlows shape produced by di_path_steps, including the correct step ordering and family-specific end_label values such as “weakly captures scoped service”, “leaks transient IDisposable”, and “caches scoped service”. Use the existing finding lookups in the test block near the DI001 assertion as the place to mirror the same flow validation for each component family.
🤖 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 `@tests/test_ownir.py`:
- Around line 824-833: Add regression coverage for the remaining DI finding
families in the existing ownir tests, not just DI001. Extend the assertions
around the fixture checks that exercise DI002, DI003, DI004, and DI005 so they
validate the full flow/codeFlows shape produced by di_path_steps, including the
correct step ordering and family-specific end_label values such as “weakly
captures scoped service”, “leaks transient IDisposable”, and “caches scoped
service”. Use the existing finding lookups in the test block near the DI001
assertion as the place to mirror the same flow validation for each component
family.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 864edf07-d91a-4ce6-97f7-64a08a988730
📒 Files selected for processing (3)
ownlang/ownir.pytests/run_tests.pytests/test_ownir.py
The wiring added a flow to every DI family but the tests only pinned DI001. Assert the full reachability slice for DI002 (weak captive), DI003 (transient IDisposable), DI004 (service-locator) and DI005 (scope-cached) bridge fixtures: each ends with its family-specific end_label, and DI004/DI005 explicitly pin the invariant that the slice begins at the REGISTRATION site even though the finding anchors at the call/store site. ownir bridge checks: 157/157. ruff + mypy --strict clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
What & why
The reachability-slice follow-up promised in #118, now that the evidence builders (
ownlang/evidence.py) are onmain. #118 added the model + builders; this wires them through the OwnIR SARIF path so a DI captive actually ships its ordered retention slice (the "why is this held, and through what?" tracerelatedLocationscan't express), and runs the diagnostics tests in CI.Changes
ownlang/ownir.py—Findinggains an orderedflowfield;_sarif_resultemits it as SARIFcodeFlows;_di_findingsbuilds each DI001–DI005 flow from the captive's dependencypathand a name→registration-site map viaevidence.di_path_steps. A captive now carries both its consuming-ctorrelatedLocationand the orderedsingleton → [transient →]* scopedslice.tests/test_ownir.py— assert the EmailSender direct (2-hop) and ReportService transitive (3-hop, transient pass-through)codeFlows, alongside the existing relatedLocations checks (now 151 bridge checks).tests/run_tests.py— foldtests/test_diagnostics.py(added in P-015: structured reachability-slice evidence for diagnostics #118 but not yet aggregated) into the suite, so the empty-evidence invariant + slice ordering run in CI.Testing
Run locally before pushing:
python tests/run_tests.py→ full suite green (ownir: 151/151,diagnostics: PASS)ruff check .→ cleanmypy(--strictonownlang) → cleanThis closes the two follow-ups noted in #118 (the
ownir.build_sarifwiring and therun_tests.pyaggregation); both were deferred there only because they couldn't be edited safely over the API — done here against a local checkout where the suite runs.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests