[PR #4493] [CLOSED] fix: resolve critical lifecycle, admission, and analysis defects (#4475, #4483, #4490, #4494, #4495, #4509) #6274

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:13:12 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4493
Author: @qorvr
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/firmware-read-handle-cleanup

📝 Commits (10+)

8545b46 fix(javascript): resolve identifier origin for Webpack-bundled external destructuring
8c4e7bb fix(browser): sanitize electron launch arguments for packaged applications
76fbc85 fix(contracts): reject NUL bytes in rizin and cutter command inputs
ff27fe5 fix(review): preserve caller arguments in electron hook and test captured aliases
8ee0a28 fix(javascript): classify local ESM specifiers before URL normalization (#4509)
98d4a21 fix(browser,firmware): detect packaged electron apps and preserve published cleanup errors
e3dfe88 fix(browser): narrow packaged electron entry point detection to app.asar and app bundles
0522cc7 fix(browser): support bundled entry files within packaged electron app directory
8397f75 fix(browser): distinguish packaged electron entry point from arbitrary scripts
3033d29 fix(browser): use non-blocking bounded reader for electron manifest and recognize bundled entries

📊 Changes

32 files changed (+1478 additions, -108 deletions)

View changed files

📝 scripts/electron-active-hook.cjs (+55 -0)
📝 scripts/verify-apktool.mjs (+38 -36)
📝 src/application/BinaryTargetResolver.ts (+1 -1)
📝 src/application/javascript/ElectronActiveObservationPort.ts (+1 -0)
📝 src/application/javascript/JavaScriptArtifactPathResolution.test.ts (+29 -0)
📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+3 -5)
📝 src/browser/PlaywrightElectronActiveProvider.test.ts (+350 -1)
📝 src/browser/PlaywrightElectronActiveProvider.ts (+259 -13)
📝 src/captures/TsharkCaptureProvider.ts (+7 -1)
📝 src/captures/parsePacketOutput.ts (+41 -6)
📝 src/cli/cutterCommands.ts (+4 -5)
📝 src/cli/reverseEngineeringCommands.ts (+4 -5)
📝 src/contracts/cutterToolContracts.ts (+2 -7)
📝 src/contracts/rizinDebugToolContracts.ts (+2 -7)
➕ src/contracts/singleCommandLineSchema.test.ts (+178 -0)
➕ src/contracts/singleCommandLineSchema.ts (+14 -0)
📝 src/cutter/CutterBridgeClient.ts (+3 -3)
📝 src/domain/javascript/electronBundledRequireIdentity.test.ts (+39 -0)
📝 src/domain/javascript/javascriptSemanticAnalysis.ts (+27 -5)
📝 src/filesystem/StableFileHash.ts (+1 -1)

...and 12 more files

📄 Description

Resolves #4475, #4483, #4490, #4494, #4495

Summary of Defects & Key Changes

1. #4495: Apple Dyld Resolution File Admission & Descriptor Lifecycle

Root Cause: DylibResolutionReader.ts opened candidate Mach-O files with blocking O_RDONLY | O_NOFOLLOW (without O_NONBLOCK), causing MCP workers to hang indefinitely if candidate paths in dyld search routes were FIFOs or special devices. Candidate files were opened 3 times independently (startsWithMachoMagic, readImage, fileSha256), descriptor close failures in finally dropped descriptor ownership, and the outer catch masked underlying I/O errors if the abort signal triggered after the failure.
Fix: Switched admission to withOwnedRegularFile enforcing non-blocking flags (O_NONBLOCK | O_NOFOLLOW), guarding against FIFOs with NonRegularFileReadError, retaining unclosed descriptors into caller-provided ArtifactResourceScope for provider retry, preserving exact cancellation causes, and avoiding masking primary I/O failures.
Verification: Added regression tests in src/artifacts/apple/MachoLoadCommandReader.test.ts for FIFO named-pipe non-blocking handling and exact cancellation propagation.

Root Cause: PlaywrightElectronActiveProvider.ts passed paths.application (app.asar) as a positional CLI argument. For packaged applications like draw.io or VS Code, the binary already packages the application, so passing the ASAR path causes the application to treat it as a user document to open on startup, triggering an "Invalid file" modal dialog that blocks declared scenario actions.
Fix: Omitted paths.application when paths.executable === paths.application, and updated scripts/electron-active-hook.cjs to sanitize process.argv on startup, removing internal REA flags and positional ASAR arguments under packaged binaries.
Verification: Unit tests in src/browser/PlaywrightElectronActiveProvider.test.ts verified argument construction and hook sanitization across packaged and unpackaged scenarios (24/24 passed).

3. #4483: Output Tree Readback Cancellation Assertion & Error Preservation

Root Cause: SafeOutputTreeReadback.ts:hashOutputFile() lacked a post-loop cancellation assertion. An aborted stream or zero-byte read could return successfully and validate against expected hashes instead of propagating cancellation, and masked original I/O errors (EIO) with cancellation.
Fix: Added isAborted() checks after the chunk read loop and after assertFilePathIdentity(), preserved { cause: signal.reason }, and prevented cancellation from masking preceding I/O errors.
Verification: Added regression tests in tests/boundary/filesystem/safeOutputTree.test.ts asserting cancellation failure reason and cause propagation (19/19 passed).

4. #4490: Webpack-Bundled External Destructuring Analysis Loss

Root Cause: In Webpack-bundled applications (e.g. Element, Joplin), external module requires (var ext = __webpack_require__('electron'); const { BrowserWindow } = ext;) failed identifier origin resolution because bindingOrigin() only checked direct require() calls and dropped identifier initializers.
Fix: Updated javascriptSemanticAnalysis.ts to trace identifier initializers using resolveSemanticBindingState with a visited cycle guard so destructured bindings inherit parent module origin (electron -> BrowserWindow).
Verification: Regression tests in src/domain/javascript/electronBundledRequireIdentity.test.ts confirmed BrowserWindow and ipcMain origin resolution (42/42 passed).

5. #4475: Firmware Read Handle Close Failure Retention

Root Cause: When closing regular file handles failed in firmware report reads or snapshotting, descriptor close errors dropped handle ownership and discarded completed partial observations.
Fix: Retained unclosed descriptors in PendingFirmwareCleanup.descriptors, enabled retries via provider.close(), and attached partialObservation on descriptor cleanup errors.
Verification: Regression tests in tests/boundary/firmware/firmwareReportFiles.test.ts and tests/boundary/firmware/firmwareWorkspaceRecovery.test.ts (45/45 passed).

Verification

npm run check:fast passed (turborepo typecheck & lint, 0 errors, 0 warnings).
All unit and boundary regression tests passing.

_{🔄 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/4493 **Author:** [@qorvr](https://github.com/qorvr) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/firmware-read-handle-cleanup` --- ### 📝 Commits (10+) - [`8545b46`](https://github.com/morluto/rea/commit/8545b465b40c8fc8bfe17d73abe4cd3bcdc8a2c7) fix(javascript): resolve identifier origin for Webpack-bundled external destructuring - [`8c4e7bb`](https://github.com/morluto/rea/commit/8c4e7bb8ddb889e67463a261e227a3a500498807) fix(browser): sanitize electron launch arguments for packaged applications - [`76fbc85`](https://github.com/morluto/rea/commit/76fbc85a024c7e03208469adbc982dc5a80bba00) fix(contracts): reject NUL bytes in rizin and cutter command inputs - [`ff27fe5`](https://github.com/morluto/rea/commit/ff27fe58123349fecd4d50b9838b0a53e089d5d3) fix(review): preserve caller arguments in electron hook and test captured aliases - [`8ee0a28`](https://github.com/morluto/rea/commit/8ee0a28c9218ca4f2863c915e0faf2760af2ab3e) fix(javascript): classify local ESM specifiers before URL normalization (#4509) - [`98d4a21`](https://github.com/morluto/rea/commit/98d4a21f5e672859a12a97ca54b78f1b2b1066e2) fix(browser,firmware): detect packaged electron apps and preserve published cleanup errors - [`e3dfe88`](https://github.com/morluto/rea/commit/e3dfe88ae770adec22639058e3e248ff37441334) fix(browser): narrow packaged electron entry point detection to app.asar and app bundles - [`0522cc7`](https://github.com/morluto/rea/commit/0522cc7003e26fd7f007170eb06ebf4c085c2fb0) fix(browser): support bundled entry files within packaged electron app directory - [`8397f75`](https://github.com/morluto/rea/commit/8397f75139be0e8457f3950cba2b0309b9fd2073) fix(browser): distinguish packaged electron entry point from arbitrary scripts - [`3033d29`](https://github.com/morluto/rea/commit/3033d2904023587d8a604e5ed9b522c1a677e65e) fix(browser): use non-blocking bounded reader for electron manifest and recognize bundled entries ### 📊 Changes **32 files changed** (+1478 additions, -108 deletions) <details> <summary>View changed files</summary> 📝 `scripts/electron-active-hook.cjs` (+55 -0) 📝 `scripts/verify-apktool.mjs` (+38 -36) 📝 `src/application/BinaryTargetResolver.ts` (+1 -1) 📝 `src/application/javascript/ElectronActiveObservationPort.ts` (+1 -0) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.test.ts` (+29 -0) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+3 -5) 📝 `src/browser/PlaywrightElectronActiveProvider.test.ts` (+350 -1) 📝 `src/browser/PlaywrightElectronActiveProvider.ts` (+259 -13) 📝 `src/captures/TsharkCaptureProvider.ts` (+7 -1) 📝 `src/captures/parsePacketOutput.ts` (+41 -6) 📝 `src/cli/cutterCommands.ts` (+4 -5) 📝 `src/cli/reverseEngineeringCommands.ts` (+4 -5) 📝 `src/contracts/cutterToolContracts.ts` (+2 -7) 📝 `src/contracts/rizinDebugToolContracts.ts` (+2 -7) ➕ `src/contracts/singleCommandLineSchema.test.ts` (+178 -0) ➕ `src/contracts/singleCommandLineSchema.ts` (+14 -0) 📝 `src/cutter/CutterBridgeClient.ts` (+3 -3) 📝 `src/domain/javascript/electronBundledRequireIdentity.test.ts` (+39 -0) 📝 `src/domain/javascript/javascriptSemanticAnalysis.ts` (+27 -5) 📝 `src/filesystem/StableFileHash.ts` (+1 -1) _...and 12 more files_ </details> ### 📄 Description Resolves #4475, #4483, #4490, #4494, #4495 ### Summary of Defects & Key Changes #### 1. #4495: Apple Dyld Resolution File Admission & Descriptor Lifecycle - **Root Cause:** `DylibResolutionReader.ts` opened candidate Mach-O files with blocking `O_RDONLY | O_NOFOLLOW` (without `O_NONBLOCK`), causing MCP workers to hang indefinitely if candidate paths in dyld search routes were FIFOs or special devices. Candidate files were opened 3 times independently (`startsWithMachoMagic`, `readImage`, `fileSha256`), descriptor close failures in `finally` dropped descriptor ownership, and the outer catch masked underlying I/O errors if the abort signal triggered after the failure. - **Fix:** Switched admission to `withOwnedRegularFile` enforcing non-blocking flags (`O_NONBLOCK | O_NOFOLLOW`), guarding against FIFOs with `NonRegularFileReadError`, retaining unclosed descriptors into caller-provided `ArtifactResourceScope` for provider retry, preserving exact cancellation causes, and avoiding masking primary I/O failures. - **Verification:** Added regression tests in `src/artifacts/apple/MachoLoadCommandReader.test.ts` for FIFO named-pipe non-blocking handling and exact cancellation propagation. #### 2. #4494: Packaged Electron App (draw.io) Modal Dialog & Scenario Blocker - **Root Cause:** `PlaywrightElectronActiveProvider.ts` passed `paths.application` (`app.asar`) as a positional CLI argument. For packaged applications like draw.io or VS Code, the binary already packages the application, so passing the ASAR path causes the application to treat it as a user document to open on startup, triggering an "Invalid file" modal dialog that blocks declared scenario actions. - **Fix:** Omitted `paths.application` when `paths.executable === paths.application`, and updated `scripts/electron-active-hook.cjs` to sanitize `process.argv` on startup, removing internal REA flags and positional ASAR arguments under packaged binaries. - **Verification:** Unit tests in `src/browser/PlaywrightElectronActiveProvider.test.ts` verified argument construction and hook sanitization across packaged and unpackaged scenarios (24/24 passed). #### 3. #4483: Output Tree Readback Cancellation Assertion & Error Preservation - **Root Cause:** `SafeOutputTreeReadback.ts:hashOutputFile()` lacked a post-loop cancellation assertion. An aborted stream or zero-byte read could return successfully and validate against expected hashes instead of propagating cancellation, and masked original I/O errors (`EIO`) with cancellation. - **Fix:** Added `isAborted()` checks after the chunk read loop and after `assertFilePathIdentity()`, preserved `{ cause: signal.reason }`, and prevented cancellation from masking preceding I/O errors. - **Verification:** Added regression tests in `tests/boundary/filesystem/safeOutputTree.test.ts` asserting cancellation failure reason and cause propagation (19/19 passed). #### 4. #4490: Webpack-Bundled External Destructuring Analysis Loss - **Root Cause:** In Webpack-bundled applications (e.g. Element, Joplin), external module requires (`var ext = __webpack_require__('electron'); const { BrowserWindow } = ext;`) failed identifier origin resolution because `bindingOrigin()` only checked direct `require()` calls and dropped identifier initializers. - **Fix:** Updated `javascriptSemanticAnalysis.ts` to trace identifier initializers using `resolveSemanticBindingState` with a visited cycle guard so destructured bindings inherit parent module origin (`electron` -> `BrowserWindow`). - **Verification:** Regression tests in `src/domain/javascript/electronBundledRequireIdentity.test.ts` confirmed `BrowserWindow` and `ipcMain` origin resolution (42/42 passed). #### 5. #4475: Firmware Read Handle Close Failure Retention - **Root Cause:** When closing regular file handles failed in firmware report reads or snapshotting, descriptor close errors dropped handle ownership and discarded completed partial observations. - **Fix:** Retained unclosed descriptors in `PendingFirmwareCleanup.descriptors`, enabled retries via `provider.close()`, and attached `partialObservation` on descriptor cleanup errors. - **Verification:** Regression tests in `tests/boundary/firmware/firmwareReportFiles.test.ts` and `tests/boundary/firmware/firmwareWorkspaceRecovery.test.ts` (45/45 passed). ### Verification - `npm run check:fast` passed (turborepo typecheck & lint, 0 errors, 0 warnings). - All unit and boundary regression tests passing. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:13:12 +00:00