flow: opt-in --body-throw-edges tier — body-level (no-try) dispose-not-called-on-throw#95
Merged
Merged
Conversation
…ose-not-called-on-throw PR #94 modelled the SOUND slice (explicit throw + the existing in-try edges). This adds the OPT-IN firehose for full CodeQL cs/dispose-not-called-on-throw parity on the no-try slice: when --body-throw-edges is set, InjectThrowEdge also treats any ESCAPING body-level may-throw call/`new` as a throw point (synthesizing a bare method exit as its continuation), not only those inside a `try`. OFF by default: it is the CA2000 firehose (flags even harmless MemoryStream/ StringWriter dispose-on-throw), so the shipped posture stays low-FP; the oracle flips it on to MEASURE full recall without shifting the default. Read deep in InjectThrowEdge via a static Program.BodyThrowEdges (set from the flag) rather than threaded through the whole LowerFlow* recursion — the explicit-throw case (onThrow stays null at body level) is untouched, so PR #94's behaviour is unaffected. own-check.sh forwards --body-throw-edges. New sample BodyThrowEdgesSample (run in BOTH modes in CI) proves the flag gates it: MayThrowLeaks 'mtbd' leaks ONLY under the flag; AdjacentDisposeClean 'adc' (no intervening may-throw) stays silent in both modes. Core handling of the {op:return} exit is unchanged (re-validated on the firehose IR shape). P-016 documents the two tiers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new opt-in Changes--body-throw-edges opt-in tier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: a6d3198f33
ℹ️ 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".
Comment on lines
+614
to
+616
| var cont = onThrow ?? (BodyThrowEdges && canEscape | ||
| ? new List<object> { new { op = "return", var = (string?)null, line = LineOf(st) } } | ||
| : null); |
There was a problem hiding this comment.
When --body-throw-edges is enabled, a may-throw statement lexically inside a finally with no local onThrow now gets a synthetic bare method exit. If that finally is nested under an outer try/finally or under a using declaration, the real exception still runs the outer cleanup, but this inserted return skips it in the IR; for example using var owner = ...; try {} finally { Log(); } can be reported as leaking even though the using cleanup runs when Log() throws. The explicit-throw path already avoids this with IsInsideFinally, but the new may-throw path does not.
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: 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 @.github/workflows/ci.yml:
- Around line 731-734: The default-off assertion only verifies that 'mtbd' is
absent from the output but does not check for 'adc', which means regressions in
'adc' reporting would not be caught. Add an additional assertion similar to the
existing grep check for 'mtbd' to also verify that 'adc' is not present in the
off-mode output, ensuring both MayThrowLeaks and AdjacentDisposeClean remain
silent when the flag is off.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 55-60: The static field BodyThrowEdges is not being reset at the
start of the program, which causes state from previous in-process invocations to
leak into subsequent runs. Reset BodyThrowEdges to false before the command-line
argument parsing loop begins (before the for loop that iterates through args),
and ensure other static configuration variables like emitEvents and flowLocals
are similarly reset to their default values to prevent sticky state across
multiple invocations in the same process.
🪄 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: f2f94efc-5a30-477e-9812-da54161c6a95
📒 Files selected for processing (5)
.github/workflows/ci.ymldocs/proposals/P-016-deep-fact-extraction.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/BodyThrowEdgesSample.csscripts/own-check.sh
…rehose (Codex P2) Symmetric to the explicit-throw fix on PR #94: under --body-throw-edges, a may-throw call lexically inside a `finally` is lowered with a null onThrow, so InjectThrowEdge synthesized a bare method exit for it — skipping any ENCLOSING finally/using cleanup and falsely flagging a resource the outer finally disposes (Codex P2: `using var o = ...; try {} finally { Log(); }`). Guard the synthesized body-level continuation with `!IsInsideFinally(st)` — the same predicate the explicit-throw path already uses. A finally-internal may-throw now gets no firehose edge (its real continuation is the outer cleanup), so the flag-on behaviour there matches flag-off. Body-level may-throws (not in a finally) unaffected. Sample MayThrowInFinallyClean ('mtf', disposed by the outer finally) pins it silent in both modes; CI asserts mtbd/mtf/adc silent in off-mode and adc/mtf silent under the flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…process state) BodyThrowEdges is static and was only ever SET, so a prior IN-PROCESS invocation with --body-throw-edges could leak into a later run that did not request it (the CLI normally runs as a fresh process, but a static set-once field is fragile if it is ever embedded/tested in-process). Reset it to false before arg parsing. The other config (emitEvents/flowLocals/reportStats) are locals, re-initialized each call, so only this static field needs the reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
pushed a commit
that referenced
this pull request
Jun 23, 2026
… recall on Npgsql v8.0.9 Bring zen-pasteur's extractor/own-check/core current with main (PR #94/#95 — the explicit-throw model + the opt-in --body-throw-edges tier), plumb body_throw_edges= through oracle.yml (workflow_dispatch input + sentinel parse + own-check append), and flip it on for the Npgsql v8.0.9 oracle. Clean before/after vs the baseline (Own.NET 10 / CodeQL 19 / Infer# 73, Agree 0): measures how much body-level dispose-not-called-on-throw recall the firehose recovers (new Agreements) vs the CA2000 noise it adds (own-only jump) — both halves of the opt-in tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why (the tiered plan, part 2)
PR #94 landed the sound default: explicit
throwmodelled as an abnormal exit (no-trydispose-not-called-on-throw+ un-bailing validation-throw methods). This is the opt-in second tier for full CodeQLcs/dispose-not-called-on-throwrecall on the no-try slice — the part the oracle flagged oracle-only on Serilog (MessageTemplateTextFormatter, a may-throw rendering call) and Npgsql.What
When
--body-throw-edgesis set,InjectThrowEdgealso treats any escaping body-level may-throw call/newas a throw point (synthesizing a bare method exit as its continuation), not only those inside atry. That reproduces CA2000 / CodeQL's broad dispose-on-throw recall.Off by default — on purpose. It's the CA2000 firehose (flags even harmless
MemoryStream/StringWriterdispose-on-throw), so the shipped posture stays below CA2000. The oracle flips it on to measure full recall without shifting the default. Graduating it to default would need a "managed-only, no OS-handle" dispose-optional expansion (noted in P-016).Design
InjectThrowEdgevia a staticProgram.BodyThrowEdges(set once from the flag) rather than threading a bool through the wholeLowerFlow*recursion.onThrowstays null at body level → the explicit-throw case from PR flow: model explicitthrowas an abnormal exit (body-level dispose-not-called-on-throw) #94 is untouched; the firehose only affects may-throw calls. The two tiers compose without interaction.canEscapegates it so a catch-all-suppressed region still injects nothing.Tests
New
BodyThrowEdgesSample.cs, run in both modes in CI (kept separate so the flag doesn't flood everyFlowLocalsSampleacquire/use/dispose):--body-throw-edgesMayThrowLeaks(mtbd)AdjacentDisposeClean(adc)adcproves the edge needs an intervening may-throw — it's not "flag any undisposed-looking local under the flag". Core handling of the{op:return}exit is unchanged (re-validated locally on the firehose IR shape);own-check.shforwards the flag; P-016 documents the two tiers.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
--body-throw-edges) to report additional disposal-path issues when exceptions can bypass normal cleanup.