From 23c89f2ff8a866c905406ecd99ee0413a6a4b069 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 27 Jun 2026 05:23:41 +0000 Subject: [PATCH 1/2] fix(ownts): don't mistake an options object for the cleanup block (CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the merged #145. For an EXPRESSION-bodied effect cleanup whose removeEventListener passes an options object — return () => el.removeEventListener("x", h, {capture: true}) _cleanup_span did `find("{")`, which landed on the `{` of `{capture: true}`, so _match_block ended the span at the options `}` and truncated the trailing `)`. The resulting cleanup no longer parsed via _LISTENER, so _listener_call returned None and a correctly-released listener was reported as a false OWN001 leak. Now a cleanup is treated as block-bodied only when the first non-whitespace character after `=>` is `{`; otherwise it is an expression body ending at the line break (which keeps the whole call, options object and closing `)` included). New fixture EffectExprCleanup.tsx (expect zero findings), pinned in test_ownts.py and a CI step. Full suite green (effects 31/31, ownir 194/194); ruff + mypy --strict clean; all existing fixtures unchanged. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi --- .github/workflows/ci.yml | 10 ++++++++++ frontend/ownts/examples/EffectExprCleanup.tsx | 15 +++++++++++++++ frontend/ownts/ownts.py | 17 +++++++++++------ frontend/ownts/test_ownts.py | 6 ++++++ 4 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 frontend/ownts/examples/EffectExprCleanup.tsx diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2a6cd279..b451f76d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1088,6 +1088,16 @@ jobs: || { echo "FAIL: expected one OWN001 (listener) + one EFF001 (object dep)"; exit 1; } echo "$hard" | grep -q "scroll" \ || { echo "FAIL: the leak must be the options-dropped scroll listener"; exit 1; } + - name: Expression-bodied cleanup with an options object is silent + run: | + python frontend/ownts/ownts.py frontend/ownts/examples/EffectExprCleanup.tsx \ + -o "$RUNNER_TEMP/expr.facts.json" + expr=$(python -m ownlang ownir "$RUNNER_TEMP/expr.facts.json" || true) + echo "$expr" + # the `{` of `{capture: true}` belongs to the call, not the cleanup block — + # the listener is released, so no false-positive leak + [ "$(echo "$expr" | grep -cE '\[(OWN|EFF|DI)[0-9]{3}\]')" -eq 0 ] \ + || { echo "FAIL: a properly-released listener must not be reported"; exit 1; } - name: The clean fixture (cleanups + useMemo'd dep) is silent run: | python frontend/ownts/ownts.py frontend/ownts/examples/DashboardClean.tsx \ diff --git a/frontend/ownts/examples/EffectExprCleanup.tsx b/frontend/ownts/examples/EffectExprCleanup.tsx new file mode 100644 index 00000000..98168d60 --- /dev/null +++ b/frontend/ownts/examples/EffectExprCleanup.tsx @@ -0,0 +1,15 @@ +// Regression (CodeRabbit): an EXPRESSION-bodied cleanup whose removeEventListener +// passes an options OBJECT must still parse — the `{` of `{capture: true}` is part +// of the call, not the cleanup block, so the listener is correctly released. +// python frontend/ownts/ownts.py frontend/ownts/examples/EffectExprCleanup.tsx --check +// Expect ZERO findings (no false-positive leak). +import { useEffect } from "react"; + +export function ExprCleanup() { + useEffect(() => { + window.addEventListener("scroll", onScroll, { capture: true }); + return () => window.removeEventListener("scroll", onScroll, { capture: true }); + }, []); + + return
expr cleanup
; +} diff --git a/frontend/ownts/ownts.py b/frontend/ownts/ownts.py index ef0f84c1..83c0c787 100644 --- a/frontend/ownts/ownts.py +++ b/frontend/ownts/ownts.py @@ -390,12 +390,17 @@ def _cleanup_span(mbody: str) -> tuple[int, int] | None: prefix = mbody[:m.start()] if prefix.count("{") - prefix.count("}") != 1: # 1 == the effect body's own brace continue - brace = mbody.find("{", m.end()) - if brace == -1: - # `return () => clearInterval(id)` — single-expression cleanup, no block. - nl = mbody.find("\n", m.end()) - return (m.start(), nl if nl != -1 else len(mbody)) - return (m.start(), _match_block(mbody, brace)) + rest = mbody[m.end():] + stripped = rest.lstrip() + if stripped.startswith("{"): # block-bodied cleanup: `=> { ... }` + brace = m.end() + (len(rest) - len(stripped)) + return (m.start(), _match_block(mbody, brace)) + # single-expression cleanup — ends at the line break. Crucially NOT a + # `find("{")`, which would mistake an options object inside the call (e.g. + # `=> el.removeEventListener("x", h, {capture: true})`) for the cleanup block + # and truncate the trailing `)`, breaking _listener_call into a false leak. + nl = mbody.find("\n", m.end()) + return (m.start(), nl if nl != -1 else len(mbody)) return None diff --git a/frontend/ownts/test_ownts.py b/frontend/ownts/test_ownts.py index 5c50a881..d04d806f 100644 --- a/frontend/ownts/test_ownts.py +++ b/frontend/ownts/test_ownts.py @@ -57,6 +57,12 @@ def main() -> int: hardening = codes("EffectHardening.tsx") assert hardening == ["OWN001", "EFF001"], f"EffectHardening -> {hardening}" + # an expression-bodied cleanup whose removeEventListener carries an options + # object must parse (the `{` belongs to the call, not the cleanup block) — the + # listener is released, so no false-positive leak. + expr_cleanup = codes("EffectExprCleanup.tsx") + assert expr_cleanup == [], f"EffectExprCleanup should be silent -> {expr_cleanup}" + # addEventListener release must match the receiver and capture/options, not just # the handler (dropping `true` or changing the target still leaks). sub = next(a for a in ownts.ACQUIRES if a.resource == "subscription") From 49294f18db7c42a2486249dae203ed32c0932610 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 27 Jun 2026 05:29:28 +0000 Subject: [PATCH 2/2] fix(ownts): consume full multi-line expr cleanup; fail CI on ownir crash Two follow-ups from review (CodeRabbit + Codex) on the cleanup-span fix: - Expression-bodied cleanups were cut at the FIRST newline after `=>`, so a formatted cleanup like return () => el.removeEventListener("resize", onResize); left an empty cleanup and reintroduced the false OWN001. _cleanup_span now consumes the whole expression across line breaks, stopping only at a top-level `;` or the effect body's own closing brace (depth-aware over the masked body). - The silent-case CI step used `|| true`, which would turn an OwnIR load/schema crash (rc 2, no findings printed) into a false-green zero-count. Dropped it so a parser/core regression fails the step; the other steps keep `|| true` because they legitimately expect findings (rc 1). EffectExprCleanup.tsx gains a multi-line cleanup case. Full suite green (effects 31/31, ownir 194/194); ruff + mypy --strict clean; all fixtures unchanged. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi --- .github/workflows/ci.yml | 4 ++- frontend/ownts/examples/EffectExprCleanup.tsx | 8 ++++++ frontend/ownts/ownts.py | 25 ++++++++++++++----- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b451f76d..5c20e6c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1092,7 +1092,9 @@ jobs: run: | python frontend/ownts/ownts.py frontend/ownts/examples/EffectExprCleanup.tsx \ -o "$RUNNER_TEMP/expr.facts.json" - expr=$(python -m ownlang ownir "$RUNNER_TEMP/expr.facts.json" || true) + # no `|| true`: this case expects ZERO findings (rc 0), so a parser/core + # crash (rc 2) must FAIL the step, not be swallowed into an empty result. + expr=$(python -m ownlang ownir "$RUNNER_TEMP/expr.facts.json") echo "$expr" # the `{` of `{capture: true}` belongs to the call, not the cleanup block — # the listener is released, so no false-positive leak diff --git a/frontend/ownts/examples/EffectExprCleanup.tsx b/frontend/ownts/examples/EffectExprCleanup.tsx index 98168d60..da7286d6 100644 --- a/frontend/ownts/examples/EffectExprCleanup.tsx +++ b/frontend/ownts/examples/EffectExprCleanup.tsx @@ -11,5 +11,13 @@ export function ExprCleanup() { return () => window.removeEventListener("scroll", onScroll, { capture: true }); }, []); + // a MULTI-LINE expression-bodied cleanup must not be cut at the first newline + // after `=>` (which would leave an empty cleanup and a false leak). + useEffect(() => { + el.addEventListener("resize", onResize); + return () => + el.removeEventListener("resize", onResize); + }, []); + return
expr cleanup
; } diff --git a/frontend/ownts/ownts.py b/frontend/ownts/ownts.py index 83c0c787..7ebb7313 100644 --- a/frontend/ownts/ownts.py +++ b/frontend/ownts/ownts.py @@ -395,12 +395,25 @@ def _cleanup_span(mbody: str) -> tuple[int, int] | None: if stripped.startswith("{"): # block-bodied cleanup: `=> { ... }` brace = m.end() + (len(rest) - len(stripped)) return (m.start(), _match_block(mbody, brace)) - # single-expression cleanup — ends at the line break. Crucially NOT a - # `find("{")`, which would mistake an options object inside the call (e.g. - # `=> el.removeEventListener("x", h, {capture: true})`) for the cleanup block - # and truncate the trailing `)`, breaking _listener_call into a false leak. - nl = mbody.find("\n", m.end()) - return (m.start(), nl if nl != -1 else len(mbody)) + # single-expression cleanup. Consume the WHOLE expression across line breaks + # — stopping at a top-level `;` or the effect body's own closing brace, NOT + # the first newline (which truncates a multi-line call) and NOT a `find("{")` + # (which would mistake an options object like `{capture: true}` inside the + # call for the cleanup block). Both truncations drop the closing `)` and turn + # a released listener into a false leak. + i, depth = m.end(), 0 + while i < len(mbody): + c = mbody[i] + if c in "([{": + depth += 1 + elif c in ")]}": + if depth == 0: # the effect body's closing brace — expression ends + break + depth -= 1 + elif c == ";" and depth == 0: + break + i += 1 + return (m.start(), i) return None