[PR #6486] [MERGED] fix(process): record no normal exit code for signal-terminated targets #7365

Closed
opened 2026-11-01 21:47:21 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6486
Author: @mhamzasakhi
Created: 10/23/2026
Status: ✅ Merged
Merged: 10/23/2026
Merged by: @morluto

Base: main ← Head: fix/process-capture-signal-exit-code


📝 Commits (1)

  • d62e490 fix(process): record no normal exit code for signal-terminated targets

📊 Changes

5 files changed (+134 additions, -24 deletions)

View changed files

📝 docs/process-capture.md (+4 -0)
📝 src/process/capture/ProcessCaptureLifecycle.ts (+29 -14)
📝 src/process/capture/ProcessHarness.ts (+2 -9)
➕ tests/acceptance/process/processSignalExit.test.ts (+94 -0)
📝 tests/boundary/process/processCaptureLifecycle.test.ts (+5 -1)

📄 Description

Summary

Fixes #6445. capture-process returned exit.code: 0 for a target that a scenario signal event killed with SIGKILL or SIGTERM, next to the nonzero exit.signal. A consumer reading the numeric code as a normal successful exit misreads a killed target.

Problem and expected behavior

The capture's exit was projected in three places:

  • buildCaptureResult (ProcessCaptureLifecycle.ts) and the retained observation (ProcessHarness.ts) kept any nonnegative PTY exitCode for an exited capture without consulting the signal.
  • Finalization kept a code only when the signal was 0, the rule docs/process-capture.md documents for finalization.exit_code.

Expected: a signal-terminated target has no normal exit code (code: null), keeping reason: "exited" and the observed signal. Normal exits keep their codes, including 0 and 3. Deadline exits stay null.

Change and scope

  • One owner for the exit projection in ProcessCaptureLifecycle.ts:
    • normalExitCode: a code only when it is nonnegative and no nonzero signal ended the target.
    • captureExitObservation: the {code, signal, reason, finalization} record.
  • The result builder and the retained observation both use captureExitObservation, replacing two hand-built copies, and finalization uses normalExitCode. The three sites can no longer drift apart.
  • docs/process-capture.md now states the rule for exited captures, next to the existing deadline and finalization rules.
  • Not included: processCaptureValidation still checks code consistency only for deadline exits. Tightening it to reject code with a nonzero signal would refuse previously retained or imported Evidence, so that compatibility decision is left to maintainers.

Contribution scope: a correctness fix to existing capture Evidence. No new execution, signalling or target authority; a maintainer scope decision is not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: process-capture exit projection.

  • Contribution scope fit and linked maintainer decision: in scope; not applicable.

  • CLI and MCP/tool-catalog contract: no schema change. exit.code was already nullable. Only signal-terminated exited captures change, from a numeric code to null.

  • Provider, bridge, target-format, or platform compatibility: none. Same projection on every capture host.

  • Evidence, artifact, provenance, or reconstruction contract: new captures of signal-terminated targets carry code: null. Existing Evidence is unchanged and still validates.

  • Target selection, authority, upstream behavior, and cleanup details: unchanged. No change to signalling, finalization timing or cleanup.

  • Generated metadata, package, or installation impact: none.

  • Read existing files or captured data.

  • Launch or attach to a process.

  • Inject instrumentation or run scripts/code.

  • Change process memory, registers, files, or configuration.

  • Contact network services.

  • Recover protected code or work around protections.

  • Documentation or usability only; no target interaction.

  • Not sure yet; investigation or maintainer guidance needed.

The launch box reflects the existing capture-process behavior these tests exercise; this PR adds no new launch path.

Evidence and regression coverage

  • Tests added or updated:

    • tests/acceptance/process/processSignalExit.test.ts (new, public CLI and MCP):
      • CLI, the report's cases plus an exit(0) control: SIGKILL gives {code: null, signal: 9}, SIGTERM gives {code: null, signal: 15}, exit(3) gives 3, exit(0) gives 0, and a deadline gives null with reason timeout.
      • MCP: SIGKILL through capture_process_scenario gives {code: null, signal: 9, reason: "exited"}. The reporter did not test MCP.
    • tests/boundary/process/processCaptureLifecycle.test.ts: the existing SIGTERM signal-event case checked only signal and reason. It now also asserts code: null.
  • Base reproduction (executed on Linux x64; the original report was macOS only): before the change, the report's script printed kill {"code":0,"signal":9,"reason":"exited"} and term {"code":0,"signal":15,"reason":"exited"}. With the two producer files reverted, both new acceptance tests and the strengthened boundary case fail.

  • User-visible CLI output after the change: SIGKILL gives {"code":null,"signal":9,"reason":"exited"}, and the exit(3) control gives {"code":3,"signal":0,"reason":"exited"}.

  • Remaining proof gaps: macOS and Windows were not run in this PR; the report observed the defect on macOS with the same producer code.

  • 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

Run on Ubuntu 24.04 x64 with Node 24.18.0 and npm 11.16.0:

  • npm run test:focused -- with the 2 changed test files: 25 passed. With the fix reverted: 2 failed.
  • npm run test:focused -- with every process-capture test file (src/process/capture, src/domain/process, tests/acceptance/process, and boundary tests that reference process capture; 65 files): 578 passed, 4 skipped, 0 failed.
  • npm run check:fast: passed.
  • npm run docs:check: passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none in the schema. Consumers that read exit.code === 0 as success for a signal-terminated capture now receive null. exit.signal is unchanged.
  • Real Hopper/Ghidra, browser, or OS coverage: Linux x64 process capture through the real CLI and MCP.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no change to what is launched, signalled or cleaned up.

Author readiness

Human owner: @mhamzasakhi

  • 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.

🔄 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/6486 **Author:** [@mhamzasakhi](https://github.com/mhamzasakhi) **Created:** 10/23/2026 **Status:** ✅ Merged **Merged:** 10/23/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/process-capture-signal-exit-code` --- ### 📝 Commits (1) - [`d62e490`](https://github.com/morluto/rea/commit/d62e490b8407e9f46555e980adebd10a670b2a21) fix(process): record no normal exit code for signal-terminated targets ### 📊 Changes **5 files changed** (+134 additions, -24 deletions) <details> <summary>View changed files</summary> 📝 `docs/process-capture.md` (+4 -0) 📝 `src/process/capture/ProcessCaptureLifecycle.ts` (+29 -14) 📝 `src/process/capture/ProcessHarness.ts` (+2 -9) ➕ `tests/acceptance/process/processSignalExit.test.ts` (+94 -0) 📝 `tests/boundary/process/processCaptureLifecycle.test.ts` (+5 -1) </details> ### 📄 Description ## Summary Fixes #6445. `capture-process` returned `exit.code: 0` for a target that a scenario `signal` event killed with SIGKILL or SIGTERM, next to the nonzero `exit.signal`. A consumer reading the numeric code as a normal successful exit misreads a killed target. ## Problem and expected behavior The capture's exit was projected in three places: - `buildCaptureResult` (`ProcessCaptureLifecycle.ts`) and the retained observation (`ProcessHarness.ts`) kept any nonnegative PTY `exitCode` for an `exited` capture without consulting the signal. - Finalization kept a code only when the signal was `0`, the rule `docs/process-capture.md` documents for `finalization.exit_code`. Expected: a signal-terminated target has no normal exit code (`code: null`), keeping `reason: "exited"` and the observed signal. Normal exits keep their codes, including `0` and `3`. Deadline exits stay `null`. ## Change and scope - One owner for the exit projection in `ProcessCaptureLifecycle.ts`: - `normalExitCode`: a code only when it is nonnegative and no nonzero signal ended the target. - `captureExitObservation`: the `{code, signal, reason, finalization}` record. - The result builder and the retained observation both use `captureExitObservation`, replacing two hand-built copies, and finalization uses `normalExitCode`. The three sites can no longer drift apart. - `docs/process-capture.md` now states the rule for `exited` captures, next to the existing deadline and finalization rules. - Not included: `processCaptureValidation` still checks code consistency only for deadline exits. Tightening it to reject `code` with a nonzero signal would refuse previously retained or imported Evidence, so that compatibility decision is left to maintainers. Contribution scope: a correctness fix to existing capture Evidence. No new execution, signalling or target authority; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: process-capture exit projection. - Contribution scope fit and linked maintainer decision: in scope; not applicable. - CLI and MCP/tool-catalog contract: no schema change. `exit.code` was already nullable. Only signal-terminated `exited` captures change, from a numeric code to `null`. - Provider, bridge, target-format, or platform compatibility: none. Same projection on every capture host. - Evidence, artifact, provenance, or reconstruction contract: new captures of signal-terminated targets carry `code: null`. Existing Evidence is unchanged and still validates. - Target selection, authority, upstream behavior, and cleanup details: unchanged. No change to signalling, finalization timing or cleanup. - Generated metadata, package, or installation impact: none. - [ ] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. The launch box reflects the existing `capture-process` behavior these tests exercise; this PR adds no new launch path. ## Evidence and regression coverage - Tests added or updated: - `tests/acceptance/process/processSignalExit.test.ts` (new, public CLI and MCP): - CLI, the report's cases plus an `exit(0)` control: SIGKILL gives `{code: null, signal: 9}`, SIGTERM gives `{code: null, signal: 15}`, `exit(3)` gives `3`, `exit(0)` gives `0`, and a deadline gives `null` with reason `timeout`. - MCP: SIGKILL through `capture_process_scenario` gives `{code: null, signal: 9, reason: "exited"}`. The reporter did not test MCP. - `tests/boundary/process/processCaptureLifecycle.test.ts`: the existing SIGTERM signal-event case checked only `signal` and `reason`. It now also asserts `code: null`. - Base reproduction (executed on Linux x64; the original report was macOS only): before the change, the report's script printed `kill {"code":0,"signal":9,"reason":"exited"}` and `term {"code":0,"signal":15,"reason":"exited"}`. With the two producer files reverted, both new acceptance tests and the strengthened boundary case fail. - User-visible CLI output after the change: SIGKILL gives `{"code":null,"signal":9,"reason":"exited"}`, and the `exit(3)` control gives `{"code":3,"signal":0,"reason":"exited"}`. - Remaining proof gaps: macOS and Windows were not run in this PR; the report observed the defect on macOS with the same producer code. - [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 Run on Ubuntu 24.04 x64 with Node 24.18.0 and npm 11.16.0: - `npm run test:focused --` with the 2 changed test files: 25 passed. With the fix reverted: 2 failed. - `npm run test:focused --` with every process-capture test file (`src/process/capture`, `src/domain/process`, `tests/acceptance/process`, and boundary tests that reference process capture; 65 files): 578 passed, 4 skipped, 0 failed. - `npm run check:fast`: passed. - `npm run docs:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none in the schema. Consumers that read `exit.code === 0` as success for a signal-terminated capture now receive `null`. `exit.signal` is unchanged. - Real Hopper/Ghidra, browser, or OS coverage: Linux x64 process capture through the real CLI and MCP. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no change to what is launched, signalled or cleaned up. ## Author readiness Human owner: @mhamzasakhi - [x] 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. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:47:21 +00:00
Sign in to join this conversation.
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#7365
No description provided.