morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #3897] [MERGED] fix(javascript): keep releases through a replaced handle as candidates #5957

New issue

Closed

opened 2026-10-31 01:10:44 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:10:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3897
Author: @kaoru0822-kitauji
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/3313-replaced-resource-handles

📝 Commits (1)

00b2bad fix(javascript): keep releases through a replaced handle as candidates

📊 Changes

2 files changed (+104 additions, -4 deletions)

View changed files

➕ src/domain/javascript/javascriptSemanticAnalysis.resource_handles.test.ts (+59 -0)
📝 src/domain/javascript/javascriptSemanticResources.ts (+45 -4)

📄 Description

Summary

Fixes #3313. A close/destroy/end call was linked to a resource acquisition through the receiver's lexical binding alone. After stream = { destroy() {} }, stream.destroy() still produced a resolved releases edge to the original createReadStream and entered the default ownership trace.

Problem and expected behavior

Smallest trigger:

let stream = fs.createReadStream("input.txt");
stream = { destroy() {} };
stream.destroy();

This yields a resolved release of the stream. A resolved edge requires unique static target identity. Here the binding may hold a different value at the call, so the release must stay a candidate, while the unchanged-handle control keeps its resolved release.

Change and scope

Complete only on a unique value: collectReleases in javascriptSemanticResources.ts marks a linked release complete only when every value written to the handle binding (initializers and reference initializers) is one of these:
- the linked acquisition call itself, through parentheses, TypeScript wrappers or await, with no destructuring projection;
- a nullish placeholder (null, void …, an unmodified undefined). As in the common let h = null; try { h = await open() } finally { await h?.close() }, such a value cannot be released, because the call is skipped or throws.
Otherwise partial: any other write, including a conditional, later or destructuring replacement, makes the release partial. The existing graph projection then emits a candidate releases edge, which the default ownership trace excludes.
Not included: flow-sensitive ordering. A replacement after the release call still makes it partial, so this errs toward uncertainty rather than a false resolved edge.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScript semantic resources (javascriptSemanticResources.ts), resourceOperations[].resolution for releases.
Contribution scope fit and linked maintainer decision: an ordinary fix within existing boundaries, so not applicable.
CLI and MCP/tool-catalog contract: no schema change.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: releases through a binding that may hold another value become candidate edges instead of resolved ones.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.

Expected effects:

Read existing files or captured data.

Evidence and regression coverage

Tests added: src/domain/javascript/javascriptSemanticAnalysis.resource_handles.test.ts.
- Unconditional, conditional and destructuring replacements produce a partial release. All 3 fail on main.
- A declaration, a later assignment, an awaited fs/promises handle and a nullish placeholder all keep a complete release.
Base reproduction (executed): the issue's analyze-javascript-application → trace-javascript-semantics ownership trace, main cf0a38ad9 vs. this branch.

Fixture	main	branch	Trace bytes (main → branch)
reassigned handle	`acquires:resolved@L2`, `releases:resolved@L4`; status `found`	`acquires:resolved@L2`; status `ambiguous`	9,360 → 8,263
unchanged handle	`acquires:resolved@L2`, `releases:resolved@L3`; status `found`	unchanged	9,358 → 9,358

Real-app semantic IR comparison (executed) with analyzeParsedJavaScriptSemantics on every JS file of Discord, Notion, Signal, Claude, Tabby and VS Code out/. That is 3,300 files, compared between main and this branch.
- All files are identical.
- An earlier draft without the nullish-placeholder rule changed exactly one release: a Claude chunk's let n = null; try { n = await open(…) } finally { await n?.close() }. That finding is why the rule was added.
- One Claude chunk throws RangeError on both builds; that is the pre-existing #3839.
Semantic analysis time, main → branch in ms, single runs:

Corpus	Main (ms)	Branch (ms)
Discord	29	27
Notion	6,990	6,794
Signal	18,586	18,259
Claude	38,109	38,739
Tabby	11,540	10,845
VS Code `out/`	16,869	16,896

The check runs only at release sites that already link an acquisition, and scans that binding's initializers.

Remaining proof gaps:
- No MCP-adapter run; the CLI and MCP share the semantic graph producer.
- Timings come from one machine.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run test:local -- src/domain/javascript/javascriptSemanticAnalysis.resource_handles.test.ts: 7 passed. On main, the 3 replacement cases fail.
npm run test:local -- src/application/javascript/JavaScriptSemanticGraphResources.test.ts: 12 passed.
161 JavaScript/domain test files via npm run test:local: 2,989 passed.
npm run check:fast: typecheck and lint pass. The pre-push hook also passed.
The CLI ownership-trace reproduction and corpus comparison above.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis).
Package or release metadata impact: none.
Security, privacy, process, or containment review: no target interaction. Private app bundles are referenced only by name and path.

Author readiness

Human owner:

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3897 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3313-replaced-resource-handles` --- ### 📝 Commits (1) - [`00b2bad`](https://github.com/morluto/rea/commit/00b2bad2d04df845bce9a703ab3c84472844f34a) fix(javascript): keep releases through a replaced handle as candidates ### 📊 Changes **2 files changed** (+104 additions, -4 deletions) <details> <summary>View changed files</summary> ➕ `src/domain/javascript/javascriptSemanticAnalysis.resource_handles.test.ts` (+59 -0) 📝 `src/domain/javascript/javascriptSemanticResources.ts` (+45 -4) </details> ### 📄 Description ## Summary Fixes #3313. A `close`/`destroy`/`end` call was linked to a resource acquisition through the receiver's lexical binding alone. After `stream = { destroy() {} }`, `stream.destroy()` still produced a resolved `releases` edge to the original `createReadStream` and entered the default ownership trace. ## Problem and expected behavior Smallest trigger: ```js let stream = fs.createReadStream("input.txt"); stream = { destroy() {} }; stream.destroy(); ``` This yields a resolved release of the stream. A resolved edge requires unique static target identity. Here the binding may hold a different value at the call, so the release must stay a candidate, while the unchanged-handle control keeps its resolved release. ## Change and scope - **Complete only on a unique value:** `collectReleases` in `javascriptSemanticResources.ts` marks a linked release `complete` only when every value written to the handle binding (initializers and reference initializers) is one of these: - the linked acquisition call itself, through parentheses, TypeScript wrappers or `await`, with no destructuring projection; - a nullish placeholder (`null`, `void …`, an unmodified `undefined`). As in the common `let h = null; try { h = await open() } finally { await h?.close() }`, such a value cannot be released, because the call is skipped or throws. - **Otherwise partial:** any other write, including a conditional, later or destructuring replacement, makes the release `partial`. The existing graph projection then emits a candidate `releases` edge, which the default ownership trace excludes. - **Not included:** flow-sensitive ordering. A replacement after the release call still makes it partial, so this errs toward uncertainty rather than a false resolved edge. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScript semantic resources (`javascriptSemanticResources.ts`), `resourceOperations[].resolution` for releases. - Contribution scope fit and linked maintainer decision: an ordinary fix within existing boundaries, so not applicable. - CLI and MCP/tool-catalog contract: no schema change. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: releases through a binding that may hold another value become candidate edges instead of resolved ones. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added: `src/domain/javascript/javascriptSemanticAnalysis.resource_handles.test.ts`. - Unconditional, conditional and destructuring replacements produce a partial release. All 3 fail on main. - A declaration, a later assignment, an awaited `fs/promises` handle and a nullish placeholder all keep a complete release. - Base reproduction (executed): the issue's `analyze-javascript-application` → `trace-javascript-semantics` ownership trace, main cf0a38ad9 vs. this branch. | Fixture | main | branch | Trace bytes (main → branch) | |---|---|---|---| | reassigned handle | `acquires:resolved@L2`, `releases:resolved@L4`; status `found` | `acquires:resolved@L2`; status `ambiguous` | 9,360 → 8,263 | | unchanged handle | `acquires:resolved@L2`, `releases:resolved@L3`; status `found` | unchanged | 9,358 → 9,358 | - Real-app semantic IR comparison (executed) with `analyzeParsedJavaScriptSemantics` on every JS file of Discord, Notion, Signal, Claude, Tabby and VS Code `out/`. That is 3,300 files, compared between main and this branch. - All files are identical. - An earlier draft without the nullish-placeholder rule changed exactly one release: a Claude chunk's `let n = null; try { n = await open(…) } finally { await n?.close() }`. That finding is why the rule was added. - One Claude chunk throws `RangeError` on both builds; that is the pre-existing #3839. - Semantic analysis time, main → branch in ms, single runs: | Corpus | Main (ms) | Branch (ms) | |---|---:|---:| | Discord | 29 | 27 | | Notion | 6,990 | 6,794 | | Signal | 18,586 | 18,259 | | Claude | 38,109 | 38,739 | | Tabby | 11,540 | 10,845 | | VS Code `out/` | 16,869 | 16,896 | The check runs only at release sites that already link an acquisition, and scans that binding's initializers. - Remaining proof gaps: - No MCP-adapter run; the CLI and MCP share the semantic graph producer. - Timings come from one machine. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:local -- src/domain/javascript/javascriptSemanticAnalysis.resource_handles.test.ts`: 7 passed. On main, the 3 replacement cases fail. - `npm run test:local -- src/application/javascript/JavaScriptSemanticGraphResources.test.ts`: 12 passed. - 161 JavaScript/domain test files via `npm run test:local`: 2,989 passed. - `npm run check:fast`: typecheck and lint pass. The pre-push hook also passed. - The CLI ownership-trace reproduction and corpus comparison above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis). - Package or release metadata impact: none. - Security, privacy, process, or containment review: no target interaction. Private app bundles are referenced only by name and path. ## Author readiness Human owner:  - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:10:44 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:45:35 +00:00

[PR #6020] [MERGED] fix(process): keep structural capture issues alongside semantic ones #7125

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5957

No description provided.

Rows
Columns