[PR #608] [MERGED] fix(native): reject signal-terminated command captures #2108

New issue

Closed

opened 2026-10-16 21:14:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-16 21:14:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/608
Author: @rudycelekli
Created: 10/5/2026
Status: ✅ Merged
Merged: 10/5/2026
Merged by: @morluto

Base: main ← Head: campaign/rea-signalled-native-command-20261005

📝 Commits (2)

e3756a9 fix(native): reject signal-terminated command captures
cd51bb0 chore: sync upstream CI fixture correction

📊 Changes

2 files changed (+30 additions, -1 deletions)

View changed files

📝 src/native/CommandRunner.ts (+1 -1)
➕ tests/boundary/providers/native/nativeCommandTermination.test.ts (+29 -0)

📄 Description

Summary

Reject incomplete native command captures from signal-terminated children.

No issue is linked: this is a newly reproduced correctness defect found while reviewing current upstream; the regression and affected production seam are included for review.

Problem and expected behavior

acceptNonZero accepts a child killed by SIGTERM even though it has no completed exit code. Allowing ordinary nonzero exit codes must not admit interrupted captures as successful observations.

Change and scope

Keep signal termination classified as a nonzero-exit failure while preserving ordinary accepted nonzero exit codes.

Contract and boundary impact

Semantic owner and earliest changed stage: XcrunCommandRunner captureProcess close handling.
CLI and MCP/tool-catalog contract: existing operations and schemas remain compatible; Signal termination returns NativeCommandFailure(reason=nonzero-exit), preventing a successful incomplete capture.
Provider, bridge, target-format, or platform compatibility: Allowlisted native process runner and its resolver port.
Evidence, artifact, provenance, or reconstruction contract: existing source identity and explicit incomplete/unsupported evidence remain preserved.
Process execution, authorization, cleanup, or containment impact: Only the owned test child terminates itself. Existing process authorization, cancellation and allowlisting remain enforced.
Generated metadata (docs/product-catalog.json), package, or installation impact: no metadata or dependency changes required.

Evidence and regression coverage

Tests added or updated: tests/boundary/providers/native/nativeCommandTermination.test.ts.
Base reproduction or other evidence: Executed actual Node subprocesses through the production runner: upstream reports SIGTERM termination as successful; the fix rejects it. A separate child completing with exit code 1 remains accepted. Executable identity uses its actual SHA-256, without module mocks. Affected base is ebb4c45.
User-visible CLI/MCP output: Signal termination returns NativeCommandFailure(reason=nonzero-exit), preventing a successful incomplete capture.
Remaining proof gaps: POSIX signal verification ran on macOS. Linux CI has not been run here; Windows is explicitly skipped because native signal semantics differ. No real Hopper/Ghidra process was involved.

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

Latest upstream sync: cd51bb0ffa0cb142ccd4db44f4d9e6a9a4cce869 incorporates current owner main, including the source-map fixture correction from #609. At this head, the original regression plus webSourceMapFetcher.test.ts, npm run check, and npm run check:fast pass. The full local suite was not repeated for this routine sync under the current contribution policy. The implementation checks below were performed before this upstream sync and do not imply a full-suite result for the new head.

npm run check:pr — not run; this focused correctness fix follows the current upstream routine-PR verification policy, which makes the complete local suite optional. CI owns full coverage.
npm run check:ci — passed
npm run verify:package — passed
npm pack --dry-run — passed
npm exec -- vitest run tests/boundary/providers/native/nativeCommandTermination.test.ts --maxWorkers=1 — passed
npm run check:fast — passed
npm run check — passed

Focused regression and reproduction commands ran with the outcomes described above.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: POSIX signal verification ran on macOS. Linux CI has not been run here; Windows is explicitly skipped because native signal semantics differ. No real Hopper/Ghidra process was involved.
Package or release metadata impact: no version, dependency, generated catalog or installation changes.
Security, privacy, process, or containment review: Only the owned test child terminates itself. Existing process authorization, cancellation and allowlisting remain enforced. Final diff contains source and regression tests; local fixtures, binaries, caches and logs are excluded.

Review checklist

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.

AI assistance was used to investigate, implement, and test this change. Commits are signed and include a DCO sign-off.

_{🔄 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/608 **Author:** [@rudycelekli](https://github.com/rudycelekli) **Created:** 10/5/2026 **Status:** ✅ Merged **Merged:** 10/5/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `campaign/rea-signalled-native-command-20261005` --- ### 📝 Commits (2) - [`e3756a9`](https://github.com/morluto/rea/commit/e3756a9270a5234a0057bec72f4d5f4506eae3d9) fix(native): reject signal-terminated command captures - [`cd51bb0`](https://github.com/morluto/rea/commit/cd51bb0ffa0cb142ccd4db44f4d9e6a9a4cce869) chore: sync upstream CI fixture correction ### 📊 Changes **2 files changed** (+30 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/native/CommandRunner.ts` (+1 -1) ➕ `tests/boundary/providers/native/nativeCommandTermination.test.ts` (+29 -0) </details> ### 📄 Description ## Summary Reject incomplete native command captures from signal-terminated children. No issue is linked: this is a newly reproduced correctness defect found while reviewing current upstream; the regression and affected production seam are included for review. ## Problem and expected behavior acceptNonZero accepts a child killed by SIGTERM even though it has no completed exit code. Allowing ordinary nonzero exit codes must not admit interrupted captures as successful observations. ## Change and scope Keep signal termination classified as a nonzero-exit failure while preserving ordinary accepted nonzero exit codes. ## Contract and boundary impact - Semantic owner and earliest changed stage: XcrunCommandRunner captureProcess close handling. - CLI and MCP/tool-catalog contract: existing operations and schemas remain compatible; Signal termination returns NativeCommandFailure(reason=nonzero-exit), preventing a successful incomplete capture. - Provider, bridge, target-format, or platform compatibility: Allowlisted native process runner and its resolver port. - Evidence, artifact, provenance, or reconstruction contract: existing source identity and explicit incomplete/unsupported evidence remain preserved. - Process execution, authorization, cleanup, or containment impact: Only the owned test child terminates itself. Existing process authorization, cancellation and allowlisting remain enforced. - Generated metadata (`docs/product-catalog.json`), package, or installation impact: no metadata or dependency changes required. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/providers/native/nativeCommandTermination.test.ts`. - Base reproduction or other evidence: Executed actual Node subprocesses through the production runner: upstream reports SIGTERM termination as successful; the fix rejects it. A separate child completing with exit code 1 remains accepted. Executable identity uses its actual SHA-256, without module mocks. Affected base is ebb4c45. - User-visible CLI/MCP output: Signal termination returns NativeCommandFailure(reason=nonzero-exit), preventing a successful incomplete capture. - Remaining proof gaps: POSIX signal verification ran on macOS. Linux CI has not been run here; Windows is explicitly skipped because native signal semantics differ. No real Hopper/Ghidra process was involved. 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 Latest upstream sync: `cd51bb0ffa0cb142ccd4db44f4d9e6a9a4cce869` incorporates current owner main, including the source-map fixture correction from #609. At this head, the original regression plus `webSourceMapFetcher.test.ts`, `npm run check`, and `npm run check:fast` pass. The full local suite was not repeated for this routine sync under the current contribution policy. The implementation checks below were performed before this upstream sync and do not imply a full-suite result for the new head. - `npm run check:pr` — not run; this focused correctness fix follows the current upstream [routine-PR verification policy](https://github.com/morluto/rea/pull/599), which makes the complete local suite optional. CI owns full coverage. - `npm run check:ci` — passed - `npm run verify:package` — passed - `npm pack --dry-run` — passed - `npm exec -- vitest run tests/boundary/providers/native/nativeCommandTermination.test.ts --maxWorkers=1` — passed - `npm run check:fast` — passed - `npm run check` — passed Focused regression and reproduction commands ran with the outcomes described above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: POSIX signal verification ran on macOS. Linux CI has not been run here; Windows is explicitly skipped because native signal semantics differ. No real Hopper/Ghidra process was involved. - Package or release metadata impact: no version, dependency, generated catalog or installation changes. - Security, privacy, process, or containment review: Only the owned test child terminates itself. Existing process authorization, cancellation and allowlisting remain enforced. Final diff contains source and regression tests; local fixtures, binaries, caches and logs are excluded. ## Review checklist - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. AI assistance was used to investigate, implement, and test this change. Commits are signed and include a DCO sign-off. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-16 21:14:22 +00:00