D5.3: broaden the BCL fresh-factory table (crypto Create() + File.OpenHandle)#136
Conversation
…+ File.OpenHandle) Extend the Tier B producer table beyond System.IO file factories. Regrouped into `_BCL_FRESH_BY_NS` (keyed by namespace so each fully-qualified identity is exact) and added: - System.IO: File.OpenHandle (SafeFileHandle). - System.Security.Cryptography: SHA1/SHA256/SHA384/SHA512/MD5.Create and Aes/RSA/ECDsa.Create — owned IDisposables; a leaked `using var sha = SHA256.Create()` is a common real leak now surfaced as OWN001 at the factory call. Matching stays precision-first: bare `Type.Method` or the FQN under the type's real namespace (System.IO.* / System.Security.Cryptography.*), `global::` stripped; a same-named type in another namespace (`MyCrypto.SHA256.Create`) does NOT match, and a first-party summary still overrides the table. `Process.Start` is deliberately excluded (static owned-Process factory but also an instance method returning bool — a bare match would fabricate ownership for `proc.Start()`). Tests: crypto leak/clean/use-after-dispose, FQN resolution, non-System look-alike rejected, Process.Start makes no claim. ownir 183/183; 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR expands Tier-B fresh-factory detection to include cryptography ChangesTier B fresh factory expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–30 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://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: fb3ebf80e6
ℹ️ 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".
| "SHA1.Create", "SHA256.Create", "SHA384.Create", "SHA512.Create", "MD5.Create", | ||
| "Aes.Create", "RSA.Create", "ECDsa.Create", |
There was a problem hiding this comment.
When a source-visible method is also named SHA256.Create/Aes.Create/RSA.Create but its summary is not fresh, direct calls are protected by _callee_returns_fresh, but a wrapper that does var r = SHA256.Create(); return r; is inferred in _infer_return_skeleton via _is_bcl_fresh_factory before summaries are consulted. Adding these bare crypto names therefore turns wrappers around same-named first-party methods into fresh factories and reports OWN001 on callers even though Tier A should override Tier B; route this path through a summary-aware check or avoid applying the bare BCL table during wrapper inference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9833699. Reproduced first: a source-visible SHA256.Create(x) that returns its parameter (non-fresh), wrapped by Make and dropped by a caller, did fabricate OWN001 on the caller — _infer_return_skeleton short-circuited to fresh via _is_bcl_fresh_factory before the summary was consulted.
Fix: _build_skeletons now passes the set of first-party method names into _infer_return_skeleton, and the bare-BCL short-circuit is suppressed when the callee is first-party (callee not in first_party and _is_bcl_fresh_factory(callee)). The wrapper then degrades to a forward, which the solver resolves through the real (non-owning) summary — caller is clean. The genuine recall path (a wrapper around an actual external BCL factory, no first-party body) is unchanged: that wrap test still asserts OWN001@10.
Added a regression covering the one-hop override (SHA256.Create first-party → Make wrapper → dropped by caller → silent). python tests/run_tests.py → ownir 184/184; ruff + mypy --strict clean.
Generated by Claude Code
A first-party method that shares a BCL factory's name (e.g. a source-visible `SHA256.Create` / `Aes.Create` / `File.OpenRead`) must be resolved through its own summary, not the curated BCL fresh-factory table. Direct calls already honored this via `_callee_returns_fresh`, but a thin wrapper that does `var r = SHA256.Create(x); return r;` was inferred in `_infer_return_skeleton` through `_is_bcl_fresh_factory` *before* summaries were consulted — turning the wrapper into a fresh factory and fabricating OWN001 on its callers even though the same-named callee has a visible, non-owning body (Codex P2). Thread the set of first-party method names into `_infer_return_skeleton` and suppress the bare-BCL short-circuit when the callee is first-party, so the wrapper degrades to a `forward` the solver resolves via the real summary. The genuine recall path (a wrapper around an actual external BCL factory) is unchanged. Adds a regression covering the one-hop override. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
![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 `@ownlang/ownir.py`:
- Around line 1260-1264: Mirror the first-party suppression used in
_infer_return_skeleton inside _callee_returns_fresh so source-compiled methods
always override the curated BCL fresh-factory table. Update the freshness check
to consult the same first_party set before falling back to
_is_bcl_fresh_factory(), especially when mos has no summary, so dropped or
overloaded first-party methods with BCL-like names are not treated as fresh on
direct calls. Keep the logic aligned with the existing first_party handling in
ownir.py and preserve the current wrapper-return behavior.
🪄 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: 4de5b480-770c-4d2c-8815-d1675e02c3d2
📒 Files selected for processing (2)
ownlang/ownir.pytests/test_ownir.py
`_infer_return_skeleton` already suppressed the BCL fresh-factory table for first-party callees, but `_callee_returns_fresh` (the shared source of truth for the leak pre-scan, the branch-hoist safety walk, and flow lowering) still fell back to the table whenever `mos` had no summary. An *overloaded* source method named like a BCL factory (e.g. a user `SHA256.Create(a, b)`) is dropped from `mos`, so a direct `var h = SHA256.Create(...)` was treated as fresh and leaked OWN001 on the caller — while a wrapper around the same method stayed silent, an asymmetry (CodeRabbit). Thread the first-party name set (computed in check_facts, covering dropped overloads) through `_callee_returns_fresh`, `_branch_hoist_safe`, `_hoisted_branch_locals`, and `_lower_flow`, and suppress the Tier B fallback for any first-party callee. Direct and wrapper-returned paths now agree. Adds a regression on the overloaded-direct-call case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
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 `@ownlang/ownir.py`:
- Around line 839-845: Normalize callee names by stripping any leading global::
before checking first-party suppression in the freshness logic. Update the
matching around the raw callee comparison used by the Tier B/BCL path so it uses
the same normalized form as the Tier B matcher, and make sure the first_party
set check and the mos lookup in ownir.py both operate on that normalized symbol
(for example in the logic near _infer_return_skeleton and the freshness checks).
This will keep source-visible calls like
System.Security.Cryptography.SHA256.Create from falling through to the BCL table
when invoked as global::System.Security.Cryptography.SHA256.Create.
🪄 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: 1cf91e4e-85ce-4b33-82f9-3e4d440a4498
📒 Files selected for processing (2)
ownlang/ownir.pytests/test_ownir.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_ownir.py
The Tier B matcher (`_is_bcl_fresh_factory`) strips a `global::` qualifier before comparing, but the Tier A guards (`mos` lookup + `first_party` check) compared the raw callee. The extractor can emit a fully-qualified call as `global::System.…SHA256.Create` while the method definition (hence `mos` / `first_party`) carries the bare form, so such a call missed both Tier A guards and fell through to the BCL table as fresh — a false OWN001 on the caller (CodeRabbit). Add `_canonical_callee_name` (the `global::`-stripped identity) and route every freshness comparison through it: `_is_bcl_fresh_factory`, the `mos`/`first_party` checks in `_callee_returns_fresh`, the `first_party` guard in `_infer_return_skeleton`, and both `first_party` set constructions. Now Tier A overrides Tier B regardless of qualification. Adds a regression on a `global::`-qualified call to a same-named first-party method. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
2 participants
What & why
Extends the Tier B producer table (shipped in #127) beyond System.IO file factories. The headline addition is crypto algorithm
Create()factories — a leakedusing var sha = SHA256.Create()(orAes/RSA/ECDsa) is one of the most common realIDisposableleaks, and was invisible (no body to inferfreshfrom).Changes (
ownlang/ownir.py)_BCL_FRESH_BY_NS(keyed by namespace, so each fully-qualified identity is exact).File.OpenHandle(SafeFileHandle).SHA1/SHA256/SHA384/SHA512/MD5.Create,Aes/RSA/ECDsa.Create.Type.Methodor the FQN under the type's real namespace (System.IO.*/System.Security.Cryptography.*),global::stripped; a same-named type in another namespace (MyCrypto.SHA256.Create) does not match, and a first-party summary still overrides the table.Process.Startdeliberately excluded — it's a static owned-Processfactory but also an instance method returningbool, so a bare match would fabricate ownership forproc.Start().Behaviour (verified)
Testing
python tests/run_tests.py→ green (ownir: 183/183);ruff+mypyclean. New tests cover crypto leak/clean/use-after-dispose, FQN resolution, look-alike rejection, and the Process.Start exclusion.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Create()-style factory methods.IDisposablefactories.global::-qualified cases.