[PR #2830] fix(javascript): resolve spread and constant kill-signal arguments #5423

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2830
Author: @ahmed0magdy
Created: 10/13/2026
Status: 🔄 Open

Base: main ← Head: contrib/child-process-call-facts

📝 Commits (5)

e02cd2a fix(javascript): keep unresolved child kill signals unknown
f2a8405 fix(javascript): unwrap default child kill signals
4c51319 fix(javascript): preserve default void kill signals
753d308 Merge remote-tracking branch 'origin/main' into contrib/child-process-call-facts
05429ce fix(javascript): resolve literal spreads and const aliases in kill signals

📊 Changes

2 files changed (+95 additions, -6 deletions)

View changed files

📝 src/domain/javascript/javascriptSemanticAnalysis.native_bindings.test.ts (+42 -0)
📝 src/domain/javascript/javascriptSemanticChildProcesses.ts (+53 -6)

📄 Description

Summary

Follow-up to #2571 and #2572. The original fixes from this PR landed on main in 04672f2 ("Reviewed fixes include … #2830 …"), and both issues are closed. After merging main back into the branch, this PR now carries only the fixes for the three review findings raised on it, in commit 05429ce.

Problem and expected behavior

killSignalName reported null (unknown) for two argument forms where Node's behavior is fully determined by the source:

child.kill(...[]): a spread of an empty array literal passes no runtime argument, so Node applies its SIGTERM default. Before, the first raw Babel argument was a SpreadElement, which produced null.
const signal = undefined; child.kill(signal): an immutable alias to undefined also gets the SIGTERM default. Before, signal was neither the global undefined spelling nor a literal, which produced null.

Change and scope

All changes are in src/domain/javascript/javascriptSemanticChildProcesses.ts:

firstRuntimeArgument: walks the call arguments and expands spreads of array literals, including nested ones, to find the first argument Node actually receives.
- ...[] gives SIGTERM, ...["SIGINT"] gives SIGINT, and an array hole (...[,]) gives SIGTERM.
- A spread of anything that isn't an array literal (a parameter, a const array variable) stays null.
immutableAliasValue: follows an identifier to its initializer only when the binding is not mutable, semanticStableInitializer gives a single unconditional initializer, and there is no destructuring projection. It follows alias chains with a cycle guard.
- This is the same rule other producers already apply (for example javascriptSemanticPromises.ts). mutable is false only for const.
- const s = undefined gives SIGTERM, and const a = "SIGKILL"; const b = a gives SIGKILL.
- let, var, reassigned bindings, and an alias of a shadowed undefined all stay null.
The unused semanticResolutionBlocked import is removed. The merge had brought it back on this branch.

Contribution scope: an ordinary fix within existing boundaries, so a maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: src/domain/javascript/javascriptSemanticChildProcesses.ts (domain semantic IR construction)
Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable
CLI and MCP/tool-catalog contract: none (no schema or IR type change; signalName is string | null)
Provider, bridge, target-format, or platform compatibility: none
Evidence, artifact, provenance, or reconstruction contract: values only. Some formerly unknown signals are now resolved where the source fully determines them.
Target selection, authority, upstream behavior, and cleanup details: not applicable (static analysis; nothing is executed)
Generated metadata, package, or installation impact: none

Expected effects:

Read existing files or captured data.

Evidence and regression coverage

Tests added: one test, "expands literal spreads and follows only immutable signal aliases", in the child-process signals block of src/domain/javascript/javascriptSemanticAnalysis.native_bindings.test.ts.
- Positive cases: ...[], ...["SIGINT"], a const undefined alias, and a const string alias chain.
- Negative cases, all null: a spread parameter, a spread const array variable, let, a reassigned let, and a shadowed undefined.
Base reproduction: the test fails on the merge commit's source (1 failed, 10 passed) and passes with the fix.
Remaining proof gaps: spreads of non-literal arrays and numeric signals (kill(9)) still report null. That is intentional: the source doesn't establish the value.

Validation performed

Run with Node 24.18.0 after npm ci:

Focused tests: 3 files, 26 tests passed.
npm run test:local -- on all JavaScript domain and application tests: 77 files, 1,851 tests passed.
npm run check:fast: passed.
The pre-commit and pre-push hooks: passed.
docs:check: not run, because no contract or generated metadata changed.

Compatibility, safety, and release

Breaking changes or migration steps: none
Real Hopper/Ghidra, browser, or OS coverage: not applicable
Package or release metadata impact: none
Security, privacy, process, or containment review: not applicable

AI assistance: written with Claude Code; the commits carry a Co-authored-by trailer.

_{🔄 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/2830 **Author:** [@ahmed0magdy](https://github.com/ahmed0magdy) **Created:** 10/13/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `contrib/child-process-call-facts` --- ### 📝 Commits (5) - [`e02cd2a`](https://github.com/morluto/rea/commit/e02cd2a1b9d7ac75300141aff634036230d1e6bd) fix(javascript): keep unresolved child kill signals unknown - [`f2a8405`](https://github.com/morluto/rea/commit/f2a84058b7903b03582cc2da642389fc5e300c2d) fix(javascript): unwrap default child kill signals - [`4c51319`](https://github.com/morluto/rea/commit/4c513190fdbcb20d97a3d15e0c6616df1e66dfe4) fix(javascript): preserve default void kill signals - [`753d308`](https://github.com/morluto/rea/commit/753d308ce219d317c324cdd8deb50e89aa781406) Merge remote-tracking branch 'origin/main' into contrib/child-process-call-facts - [`05429ce`](https://github.com/morluto/rea/commit/05429ce9c5054eae549dbe41058611d17a1907b0) fix(javascript): resolve literal spreads and const aliases in kill signals ### 📊 Changes **2 files changed** (+95 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptSemanticAnalysis.native_bindings.test.ts` (+42 -0) 📝 `src/domain/javascript/javascriptSemanticChildProcesses.ts` (+53 -6) </details> ### 📄 Description ## Summary Follow-up to #2571 and #2572. The original fixes from this PR landed on main in 04672f2 ("Reviewed fixes include … #2830 …"), and both issues are closed. After merging main back into the branch, this PR now carries only the fixes for the three review findings raised on it, in commit 05429ce. ## Problem and expected behavior `killSignalName` reported `null` (unknown) for two argument forms where Node's behavior is fully determined by the source: - **`child.kill(...[])`:** a spread of an empty array literal passes no runtime argument, so Node applies its SIGTERM default. Before, the first raw Babel argument was a `SpreadElement`, which produced `null`. - **`const signal = undefined; child.kill(signal)`:** an immutable alias to `undefined` also gets the SIGTERM default. Before, `signal` was neither the global `undefined` spelling nor a literal, which produced `null`. ## Change and scope All changes are in `src/domain/javascript/javascriptSemanticChildProcesses.ts`: - **`firstRuntimeArgument`:** walks the call arguments and expands spreads of array literals, including nested ones, to find the first argument Node actually receives. - `...[]` gives SIGTERM, `...["SIGINT"]` gives SIGINT, and an array hole (`...[,]`) gives SIGTERM. - A spread of anything that isn't an array literal (a parameter, a `const` array variable) stays `null`. - **`immutableAliasValue`:** follows an identifier to its initializer only when the binding is not `mutable`, `semanticStableInitializer` gives a single unconditional initializer, and there is no destructuring projection. It follows alias chains with a cycle guard. - This is the same rule other producers already apply (for example `javascriptSemanticPromises.ts`). `mutable` is false only for `const`. - `const s = undefined` gives SIGTERM, and `const a = "SIGKILL"; const b = a` gives SIGKILL. - `let`, `var`, reassigned bindings, and an alias of a shadowed `undefined` all stay `null`. - The unused `semanticResolutionBlocked` import is removed. The merge had brought it back on this branch. Contribution scope: an ordinary fix within existing boundaries, so a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/domain/javascript/javascriptSemanticChildProcesses.ts` (domain semantic IR construction) - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable - CLI and MCP/tool-catalog contract: none (no schema or IR type change; `signalName` is `string | null`) - Provider, bridge, target-format, or platform compatibility: none - Evidence, artifact, provenance, or reconstruction contract: values only. Some formerly unknown signals are now resolved where the source fully determines them. - Target selection, authority, upstream behavior, and cleanup details: not applicable (static analysis; nothing is executed) - Generated metadata, package, or installation impact: none Expected effects: - [x] Read existing files or captured data. ## Evidence and regression coverage - **Tests added:** one test, "expands literal spreads and follows only immutable signal aliases", in the `child-process signals` block of `src/domain/javascript/javascriptSemanticAnalysis.native_bindings.test.ts`. - Positive cases: `...[]`, `...["SIGINT"]`, a `const` undefined alias, and a `const` string alias chain. - Negative cases, all `null`: a spread parameter, a spread `const` array variable, `let`, a reassigned `let`, and a shadowed `undefined`. - **Base reproduction:** the test fails on the merge commit's source (1 failed, 10 passed) and passes with the fix. - **Remaining proof gaps:** spreads of non-literal arrays and numeric signals (`kill(9)`) still report `null`. That is intentional: the source doesn't establish the value. ## Validation performed Run with Node 24.18.0 after `npm ci`: - Focused tests: 3 files, 26 tests passed. - `npm run test:local --` on all JavaScript domain and application tests: 77 files, 1,851 tests passed. - `npm run check:fast`: passed. - The pre-commit and pre-push hooks: passed. - `docs:check`: not run, because no contract or generated metadata changed. ## Compatibility, safety, and release - Breaking changes or migration steps: none - Real Hopper/Ghidra, browser, or OS coverage: not applicable - Package or release metadata impact: none - Security, privacy, process, or containment review: not applicable --- AI assistance: written with Claude Code; the commits carry a `Co-authored-by` trailer. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

bsakaguchi referenced this issue

2026-11-01 21:44:07 +00:00

[PR #5430] [CLOSED] fix(native): declare filesystem write effect for inspect_signature #6928