fix(ownts): literal-proof useEffect-парсер + строгий матчинг release для addEventListener#145
Merged
Merged
Conversation
…match Follow-up to the merged #144, addressing CodeRabbit's last two Major findings on the OwnTS spike's parser. 1. String/bracket-aware parsing. _expr_end / _effect_callback / _split_cleanup counted raw punctuation, so a string like fetch("/a,b") or a `{` inside a literal truncated the body or split the deps, and the deps regex tore `[items[i]]` at its inner bracket — feeding garbage facts to OWN001/EFF001. Now the source is string-masked once (_mask_strings blanks literal CONTENT, keeping delimiters, length and newlines) and every structural scan runs on the masked copy; the dependency array is matched by balanced brackets with a top-level comma split. Display snippets still come from the original text. 2. addEventListener release now matches the receiver and capture/options, not just the handler: window.addEventListener("x", h, true) is no longer treated as released by removeEventListener("x", h) (option dropped) or by a removeEventListener on a different target — both still leak. Coverage: new fixture EffectHardening.tsx (string commas/braces + nested dep bracket + options-dropped listener -> OWN001 + EFF001), pinned in test_ownts.py alongside direct _is_released discrimination cases, plus a CI step. All existing fixtures unchanged; full suite green (effects 31/31, ownir 194/194, explain); ruff + mypy --strict clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi
📝 WalkthroughWalkthroughOwnTS now masks string literals during ChangesOwnTS effect hardening
Sequence Diagram(s)sequenceDiagram
participant Job as ownts-react-effects job
participant Extractor as frontend/ownts/ownts.py
participant Converter as python -m ownlang ownir
participant Checks as workflow assertions
Job->>Extractor: run on frontend/ownts/examples/EffectHardening.tsx
Extractor->>Converter: produce OwnIR facts
Converter-->>Job: OWN/EFF/DI diagnostics
Job->>Checks: verify two findings and scroll listener wording
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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://cdn.github.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: dc7ba94947
ℹ️ 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
+108
to
+110
| if opts: | ||
| pat += rf"[^)]*{re.escape(opts)}" | ||
| return bool(re.search(pat, cleanup)) |
There was a problem hiding this comment.
When addEventListener omits the third argument, the listener is registered with capture false, but this branch skips option comparison because opts is empty. A cleanup such as window.removeEventListener("x", onX, true) is therefore accepted as a release even though it targets the capture-true listener and leaves the original listener installed, suppressing the OWN001 leak. Normalize omitted options to the default capture value and compare it in both directions.
Useful? React with 👍 / 👎.
…dex)
addEventListener's third argument is identified for removal by its capture flag,
which defaults to false when omitted. The previous check only required matching
options when the acquire HAD a third arg, so add("x", h) (capture false) was
wrongly accepted as released by remove("x", h, true) (capture true), suppressing
the OWN001 leak. Now _capture_flag normalizes each side (omitted/object-without-
capture -> false; true/false; {capture:…}; non-literal -> verbatim) and the
listener is released only when receiver, handler AND capture flag all match — in
both directions. passive/once/signal don't affect removal identity.
test_ownts.py adds the symmetric case (omitted vs remove(...,true) -> leak),
{capture:true}==true, and passive-option normalization. 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
![coderabbitai[bot]](https://cdn.github.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 `@frontend/ownts/ownts.py`:
- Around line 91-110: The listener-release check in ownts.py is matching on only
parts of the add/removeEventListener call, which can incorrectly treat different
listeners as cleaned up. Update the subscription branch in the matching logic to
parse both the acquire and cleanup calls into a full listener key using the
existing addEventListener/removeEventListener handling, then compare receiver,
event name, handler, and options together. Ensure the code in the subscription
path fails closed and returns False unless all fields match exactly, including
cases where the receiver is indexed or nested.
🪄 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: e64a1af3-f92a-454e-b7b1-62dcb85ef31a
📒 Files selected for processing (4)
.github/workflows/ci.ymlfrontend/ownts/examples/EffectHardening.tsxfrontend/ownts/ownts.pyfrontend/ownts/test_ownts.py
…key, fail closed
The listener-release check compared only handler + capture, so a real leak could
be suppressed: addEventListener("scroll", h) was "released" by
removeEventListener("resize", h) (different event), and an indexed/nested
receiver like nodes[i].addEventListener(...) collapsed the receiver to empty so a
cleanup on any target matched.
Now both the acquire and each cleanup removeEventListener are parsed into the full
listener key (receiver, event, handler, capture) and a listener is released only
when ALL fields match exactly; otherwise it fails closed (leak). The receiver
parser accepts member/index chains (this.ref, nodes[i]). The cleanup scan uses a
literal regex + Python comparison (no interpolated pattern), which also clears the
ast-grep ReDoS note.
To compare event-name string literals, release is now decided on the ORIGINAL
(unmasked) setup/cleanup while acquires are still located on the masked copy
(_cleanup_span cuts the same span out of both). test_ownts.py adds the
different-event and indexed-receiver-mismatch cases. 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
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 `@frontend/ownts/ownts.py`:
- Around line 393-398: The cleanup-span detection in the expression-bodied
branch is too permissive and mistakenly treats an options object inside a call
as the cleanup block. In the span logic around mbody.find("{", m.end()) and
_match_block, only classify a cleanup as block-bodied when the first
non-whitespace character after => is {; otherwise keep the expression-body range
ending at the line break. This preserves calls like removeEventListener(...,
{capture: true}) so _listener_call and _is_released continue to parse them
correctly.
🪄 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: 3d006d8e-ab78-4454-9772-8b8b68274da9
📒 Files selected for processing (2)
frontend/ownts/ownts.pyfrontend/ownts/test_ownts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/ownts/test_ownts.py
Comment on lines
+393
to
+398
| brace = mbody.find("{", m.end()) | ||
| if brace == -1: | ||
| # `return () => clearInterval(id)` — single-expression cleanup, no block. | ||
| nl = body.find("\n", m.end()) | ||
| tail = body[m.end(): nl if nl != -1 else len(body)] | ||
| return body[: m.start()], tail | ||
| end = _match_block(body, brace) | ||
| return body[: m.start()] + body[end:], body[brace:end] | ||
| return body, "" | ||
| nl = mbody.find("\n", m.end()) | ||
| return (m.start(), nl if nl != -1 else len(mbody)) | ||
| return (m.start(), _match_block(mbody, brace)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expression-bodied cleanup with an options object is mis-spanned, causing false-positive leaks.
brace = mbody.find("{", m.end()) searches arbitrarily far for the cleanup block's {. For a single-expression cleanup whose removeEventListener passes an options object — exactly the case this PR targets — the { of the options literal is found instead:
return () => el.removeEventListener("scroll", onScroll, {capture: true})
_match_block then ends the span at the options }, truncating the trailing ). The resulting cleanup_o no longer parses via _LISTENER (which requires a closing )), so _listener_call returns None, _is_released returns False, and a correctly-released listener is reported as a leak. Block-bodied cleanups are unaffected; only expression bodies containing braces break.
Detect a block only when the first non-whitespace char after => is {:
🐛 Proposed fix
- 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, e.g. `return () => clearInterval(id)` or
+ # `return () => el.removeEventListener("x", h, {capture: true})`
+ nl = mbody.find("\n", m.end())
+ return (m.start(), nl if nl != -1 else len(mbody))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| brace = mbody.find("{", m.end()) | |
| if brace == -1: | |
| # `return () => clearInterval(id)` — single-expression cleanup, no block. | |
| nl = body.find("\n", m.end()) | |
| tail = body[m.end(): nl if nl != -1 else len(body)] | |
| return body[: m.start()], tail | |
| end = _match_block(body, brace) | |
| return body[: m.start()] + body[end:], body[brace:end] | |
| return body, "" | |
| 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, e.g. `return () => clearInterval(id)` or | |
| # `return () => el.removeEventListener("x", h, {capture: true})` | |
| nl = mbody.find("\n", m.end()) | |
| return (m.start(), nl if nl != -1 else len(mbody)) |
🤖 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/ownts.py` around lines 393 - 398, The cleanup-span detection
in the expression-bodied branch is too permissive and mistakenly treats an
options object inside a call as the cleanup block. In the span logic around
mbody.find("{", m.end()) and _match_block, only classify a cleanup as
block-bodied when the first non-whitespace character after => is {; otherwise
keep the expression-body range ending at the line break. This preserves calls
like removeEventListener(..., {capture: true}) so _listener_call and
_is_released continue to parse them correctly.
10 tasks
This was referenced Jun 27, 2026
10 tasks
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.
Что и зачем
Доводка спайка OwnTS после смерженного #144 — закрывает два последних Major-замечания CodeRabbit по парсеру (они уже после мерджа, поэтому отдельным PR).
_expr_end/_effect_callback/_split_cleanupсчитали сырую пунктуацию: строка вродеfetch("/a,b")или{внутри литерала обрезала тело/делила deps, а[items[i]]рвался по внутренней скобке → вOWN001/EFF001шли кривые факты. Теперь исходник один раз маскируется (_mask_stringsгасит содержимое литералов, сохраняя кавычки, длину и переводы строк), весь структурный разбор идёт по маске, а массив зависимостей берётся по балансу скобок с разбиением по top-level запятым. Сниппеты для сообщений по-прежнему из оригинала.addEventListenerrelease теперь требует совпадения получателя и capture/options, а не только handler:window.addEventListener("x", h, true)больше не считается снятым черезremoveEventListener("x", h)(опция отброшена) или черезremoveEventListenerна другом таргете — оба остаются утечкой.Тип изменения
Как проверено
python tests/run_tests.py— зелёный (effects 31/31,ownir 194/194,explain)ruff check .иmypy(--strict поownlang) — чистоpython frontend/ownts/test_ownts.py(новый кейсEffectHardening+ прямые проверки_is_releasedна drop-options / wrong-target)python frontend/ownts/ownts.py frontend/ownts/examples/EffectHardening.tsx --checkСвязанные issue
Follow-up к #144 (P-020 OwnTS/EFF001). Closes — нет.
Чеклист
EffectHardening.tsx,_is_released-кейсы, CI-шаг)fix:)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
useEffectdependencies and cleanup handling to be resilient to string literals and complex dependency expressions.addEventListenerrelease detection to require matching handler identity and listener options, preventing false “released” results.Tests