feat(ownts): real-world FP benchmark + срезано 17/28 ложных срабатываний - #149
Conversation
Ran OwnTS against a real OSS corpus (modern arrow-ESM hook libraries from npm:
@mantine/hooks, @react-hookz/web, @uidotdev/usehooks, usehooks-ts; ~108 useEffect
across 81 files — github.com egress is blocked, npm is allowed). The honest
baseline: 28 findings, 0 confirmed leaks, 28 false positives. These libraries DO
clean up — just with patterns the fixture-tuned spike never modeled.
docs/notes/ownts-oss-benchmark.md records the method, the result, and the
prioritized FP catalog. This commit then teaches the FRONTEND (not the core) the
provable cleanup patterns — only ever marking a resource released when the
teardown is visible:
- AbortController `{ signal }` listeners released by `controller.abort()`.
- Handle in a ref / pre-declared `let` (`ref.current = setTimeout`, `let i; i =
setInterval`) — _lhs_token now finds the matching clear*.
- Cleanup `return () => …` inside an if/try/.then() block — _cleanup_span now
keys on FUNCTION depth, not brace depth (a return in a nested callback still is
not the effect cleanup).
- Bare `recv.subscribe(...)` released by `recv.unsubscribe()`/`.disconnect()`.
Result: 28 -> 11 findings (17 FP eliminated, 61%); correctly-silent effects
80/108 -> 97/108. The 11 residual are documented as the genuinely-hard cases
(aliased receiver, cross-effect/named-helper cleanup, one-shot setTimeout, a
cached-node listener) — none fixed by guessing a release.
EffectRealWorld.tsx pins the four fixed patterns (zero findings); Dashboard /
EffectEdges still flag real leaks, proving no over-suppression. CI step added.
Full suite green (effects 31/31, ownir 194/194); ruff + mypy --strict clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi
|
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates OwnTS cleanup/release detection, adds React effect fixtures with multiple cleanup patterns, adds test and CI checks for expected verdict counts, and documents the benchmark run and results. ChangesReact effect cleanup benchmark
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 694221ef9f
ℹ️ 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".
| if c == "r" and fdepth == 1: | ||
| m = ret_re.match(mbody, i) | ||
| if m: |
There was a problem hiding this comment.
Keep conditional cleanups from silencing acquires
When the cleanup return is inside a nested block, this accepts it solely because function depth is 1, and extract() then treats the cleanup as if it applies to the whole effect. In an effect that acquires before the branch, e.g. const id = setInterval(...); if (enabled) return () => clearInterval(id);, the enabled === false path still creates the timer but returns no cleanup, yet OwnTS now emits released: true and the core reports zero findings. Please keep conditional returns path-sensitive, or only apply them to acquires dominated by the same branch.
Useful? React with 👍 / 👎.
| recv = _receiver(setup, pos) | ||
| return bool(recv and re.search( | ||
| rf"{re.escape(recv)}\s*\.\s*(?:unsubscribe|disconnect)\s*\(", cleanup)) |
There was a problem hiding this comment.
Match bare subscribe cleanups by subscription key
For bare receiver.subscribe(...) calls, matching only the receiver means any receiver.unsubscribe(...)/disconnect() in cleanup suppresses the finding, regardless of the subscription key. In libraries where unsubscribe takes the same target/event/handler arguments (the new fixture itself uses ro.unsubscribe(target, handler)), ro.subscribe(a, h1); return () => ro.unsubscribe(b, h2); leaves the original subscription alive but is emitted as released, so OWN001 is missed. Please compare the arguments or fail closed for argument-keyed APIs.
Useful? React with 👍 / 👎.
| am = _LISTENER.match(setup[pos:].lstrip()) | ||
| opts = (am.group(3) or "") if am else "" | ||
| if re.search(r"\bsignal\b", opts) and re.search(r"\.abort\s*\(", cleanup): | ||
| return True |
There was a problem hiding this comment.
Tie AbortSignal cleanup to the aborted controller
This only checks that the listener options mention signal and that some .abort() appears in cleanup, but it does not tie the signal to the aborted controller. With { signal: props.signal } or { signal: c1.signal } plus return () => c2.abort(), the listener remains registered while OwnTS marks it released and suppresses OWN001. Please match the signal expression/destructured signal back to the controller being aborted, or fail closed when that relationship is not visible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/ownts/test_ownts.py (1)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a false-negative control for the new release matchers.
This only pins the happy path. Please add a focused negative case where the wrong controller or wrong
unsubscribe(...)must still report a leak, otherwise the broadened_is_releasedheuristics can silently over-suppress findings.🤖 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 `@frontend/ownts/test_ownts.py` around lines 60 - 64, Add a negative control in the ownts tests so the broadened release heuristics do not over-suppress leaks. Update the assertions around codes("EffectRealWorld.tsx") in test_ownts.py or add a nearby focused case that uses the same release patterns but with the wrong AbortController signal or a mismatched observer.unsubscribe call, and assert it still reports a leak instead of being treated as released. Keep the check targeted to the _is_released matcher behavior so the false-negative guard clearly exercises the new matcher logic.
🤖 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 `@docs/notes/ownts-oss-benchmark.md`:
- Around line 128-132: The reproduce command is not reproducible because it
packs the latest available package versions instead of the specific builds used
for the benchmark. Update the npm pack step in the note to pin each package to
the exact versions named in the Method section, and keep the rest of the
extraction and triage workflow the same so future reruns use the same inputs.
In `@frontend/ownts/ownts.py`:
- Around line 138-142: The receiver-only subscribe cleanup check in the matching
logic is too permissive. Tighten the condition in the function that builds the
`recv`/`cleanup` regex so it only matches a real cleanup for the same receiver
and subscription, not any `unsubscribe`/`disconnect` text. Add a proper left
boundary for the receiver name to prevent substring matches like
`otherro.unsubscribe(...)`, and make the cleanup check verify the same
subscription target instead of treating any receiver method call as release.
- Around line 147-153: The AbortController check in the listener-release logic
is too broad because it treats any cleanup .abort() as proof that a signal-bound
listener is released. Update the matching in the relevant function around the
_LISTENER regex handling so the abort in cleanup is tied to the same controller
or signal source used in setup, rather than just checking for re.search on
signal and .abort anywhere. Make sure the safe-return path only triggers when
the listener’s signal and the aborting controller actually correspond.
---
Nitpick comments:
In `@frontend/ownts/test_ownts.py`:
- Around line 60-64: Add a negative control in the ownts tests so the broadened
release heuristics do not over-suppress leaks. Update the assertions around
codes("EffectRealWorld.tsx") in test_ownts.py or add a nearby focused case that
uses the same release patterns but with the wrong AbortController signal or a
mismatched observer.unsubscribe call, and assert it still reports a leak instead
of being treated as released. Keep the check targeted to the _is_released
matcher behavior so the false-negative guard clearly exercises the new matcher
logic.
🪄 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: a2b69c68-a9d6-4b84-aa4f-97cc51506d54
📒 Files selected for processing (5)
.github/workflows/ci.ymldocs/notes/ownts-oss-benchmark.mdfrontend/ownts/examples/EffectRealWorld.tsxfrontend/ownts/ownts.pyfrontend/ownts/test_ownts.py
…bbit)
The broadened cleanup matchers from this PR could over-suppress (false negatives).
Reviewers found three; all are now tied to the specific resource:
- AbortController: `{ signal }` is resolved back to its controller (via
`const signal = c.signal` / `const { signal } = c`) and release requires
`<that controller>.abort()`, not any `.abort()`. So `{signal: c1.signal}` +
`c2.abort()` still leaks.
- Observer subscribe: a bare `recv.subscribe(args)` is released only by
`recv.unsubscribe(args)` on an EXACT receiver match (iterate + compare in
Python — no substring `otherro`, no interpolated regex / ReDoS) with the SAME
argument list (`disconnect()` with no args tears down everything). So
`ro.subscribe(a,h)` + `ro.unsubscribe(b,h)` still leaks.
- Conditional cleanup: _cleanup_span now returns a coverage bound; a `return`
inside a branch (or a braceless `if (x) return …`) only releases acquires it
dominates (co-guarded in the same block). So `setInterval(...)` then
`if (enabled) return () => clearInterval(id)` still leaks on the !enabled path.
EffectLeakControl.tsx pins all three as OWN001 (a false-negative control beside
the EffectRealWorld happy path) + a CI step; the OSS benchmark is unchanged at 11
(the real cases are co-guarded / properly keyed). Pinned the npm versions in the
benchmark note's reproduce command. Full suite green; ruff + mypy --strict clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi
Что и зачем
Прогнал OwnTS по реальному OSS-корпусу (современные arrow-ESM hook-библиотеки из npm:
@mantine/hooks,@react-hookz/web,@uidotdev/usehooks,usehooks-ts— ~108useEffectв 81 файле;github.comзакрыт egress-политикой, npm разрешён) и ответил на gating-вопрос P-020 «низкий ли FP на реальном коде».Честный baseline: 28 находок, 0 подтверждённых утечек, 28 ложных. Эти библиотеки чистят за собой — просто паттернами, которые спайк, заточенный на синтетические фикстуры, не моделировал.
docs/notes/ownts-oss-benchmark.mdфиксирует методику, результат и приоритизированный каталог FP. Затем учу фронтенд (не ядро) доказуемым паттернам очистки — помечаю released только когда вижу teardown:{ signal }→ release черезcontroller.abort().let(ref.current = setTimeout,let i; i = setInterval) —_lhs_tokenнаходит соответствующийclear*.returnиз вложенного блока (if/try/.then()) —_cleanup_spanперешёл с brace-depth на function-depth (returnво вложенном колбэке по-прежнему не cleanup эффекта).recv.subscribe(...)→recv.unsubscribe()/.disconnect()по тому же получателю.Результат
Срезано 17 FP (61%), корректно-молчащих эффектов 80/108 → 97/108. Оставшиеся 11 — задокументированные тяжёлые случаи (алиас получателя, cross-effect/named-helper cleanup, one-shot
setTimeout, listener на кэшированном<script>-узле); ни один не «починен» угадыванием release.Тип изменения
Как проверено
python tests/run_tests.py— зелёный (effects 31/31,ownir 194/194)ruff check .иmypy(--strict поownlang) — чистоpython frontend/ownts/test_ownts.py(новая фикстураEffectRealWorld.tsx→ 0;Dashboard/EffectEdgesвсё ещё ловят реальные утечки → нет over-suppression)Связанные issue
Refs P-020 (item 7 — benchmark на OSS). Closes — нет.
Чеклист
EffectRealWorld.tsx+ CI-шаг)docs/notes/ownts-oss-benchmark.md— before/after + каталог)feat:)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
OWN001leaks) and extended CI to verify the benchmark case stays clean.