[PR #3601] [CLOSED] fix(process): report cleanup states when capture preparation fails #5848

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:09:53 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3601
Author: @nikolay-dementiev
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/3169-preparation-cleanup-states

📝 Commits (4)

492f339 test(process): expect cleanup states in the terminal progress of a failed preparation
c43a065 fix(process): report cleanup states when capture preparation fails
e2bc25c test(process): expect a failed preparation to report the cleanup it released
391620e test(process): cover a failed root release, the error envelope and the terminal phase of a failed preparation

📊 Changes

2 files changed (+177 additions, -2 deletions)

View changed files

📝 src/process/capture/ProcessHarness.ts (+20 -0)
📝 tests/boundary/process/processCaptureProgress.test.ts (+157 -2)

📄 Description

Summary

When capture preparation fails, the final progress line reports the disposition but omits the owned_process_group, terminal_renderer and temporary_root cleanup states that docs/process-capture.md documents for the final line. The line now carries those states, reflecting what preparation actually allocated and released. Exit code, the process_capture_failed envelope and successful captures are unchanged.

Fixes #3169

Problem and expected behavior

capture-process with TMPDIR pointing at a nonexistent directory fails during preparation (on macOS the ownership inspector's mkdtemp is the first allocation). Its last stderr line is:

{"rea_progress":{"phase":"cleanup","completed":0,"total":null,"message":"elapsed_ms=1 frames=0 samples=0 interactions=0 disposition=failed","terminal":true,"sequence":2}}

A successful capture ends with ... disposition=exited owned_process_group=cleaned terminal_renderer=cleaned temporary_root=cleaned, and docs/process-capture.md says the final line's message contains disposition=<value> and the three cleanup states, with failed among the documented dispositions. The catch around prepareProcessCapture in runProcessScenario reported the terminal state without a cleanup report, and formatProgressMessage appends the states only when one is supplied.

Expected: the final line of a failed preparation carries the three states. Nothing was allocated when preparation failed before its first allocation (not_required for each resource); when it failed after allocating the temporary root, the states are those of the release it performed.

Change and scope

src/process/capture/ProcessHarness.ts: the preparation catch now reports cleanup: preparationCleanupReport(cause). prepareProcessCapture already releases the temporary root it allocated and throws a ProcessCaptureError that carries the resulting cleanupReport; the helper returns that report when the cause has one, and otherwise a constant report with all three resources not_required, because a failure without a report happened before anything was allocated. The error that is thrown is unchanged.

Intentionally not included: no change to prepareProcessCapture, the error envelope (process_capture_failed keeps its fields), successful captures, schemas, CLI flags or documentation (not_required is part of the documented state vocabulary).

Contract and boundary impact

Semantic owner and earliest changed stage: process capture harness, at the terminal progress report of a failed preparation.
Contribution scope fit and linked maintainer decision: not applicable (fix within an existing boundary).
CLI and MCP/tool-catalog contract: none. Only the text of the final progress message on this failure path gains the documented key=value states.
Provider, bridge, target-format, or platform compatibility: platform independent; checked on macOS arm64.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details (if applicable): preparation allocates the temporary root only after the ownership inspector and snapshot; a failure before it reports not_required, a failure after it reports the release result (cleaned, or failed/unverified with its reason).
Generated metadata, package, or installation impact: none.

Expected effects:

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.

Evidence and regression coverage

Tests added or updated: tests/boundary/process/processCaptureProgress.test.ts (process-boundary lane). runCaptureCli gains an optional environment override. (1) The reporter's failure: TMPDIR, TEMP and TMP point at a missing directory; exit 1, a valid process_capture_failed envelope, exactly one monotonic terminal line with phase cleanup, disposition=failed and the three states not_required. (2) captureProcessScenario with a captureSnapshot that cancels after the snapshot, so preparation fails after the real temporary root exists: disposition=cancelled, temporary_root=cleaned, the other two not_required. (3) The same failure with a resource scope whose release reports the root as failed: the error is cleanup_incomplete and the line reports temporary_root=failed.
Base reproduction or other evidence: test (1) fails on 10aa74dfe with expected [ undefined, undefined, undefined ] to deeply equal [ "not_required", "not_required", "not_required" ]. Real run on macOS 26.5 arm64, Node 26.3.0, after npm run build:cached: before, the failure command ended with the line above and exit 1; after, it ends with {"rea_progress":{"phase":"cleanup","completed":0,"total":null,"message":"elapsed_ms=2 frames=0 samples=0 interactions=0 disposition=failed owned_process_group=not_required terminal_renderer=not_required temporary_root=not_required","terminal":true,"sequence":2}} and exit 1, with the same envelope; the control with an existing TMPDIR still ends with disposition=exited owned_process_group=cleaned terminal_renderer=cleaned temporary_root=cleaned.
User-visible CLI/MCP output (if applicable): the final progress line above.
Remaining proof gaps: Linux and Windows were not executed; on those platforms the nonexistent TMPDIR/TEMP/TMP fails at the temporary root allocation, which reports the same states by the same path.

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 check:fast, npm run check - green
npm run jscpd - 3.21% duplicated lines (threshold 3.5); npm run scan:todos - no tech-debt markers
npm run test:focused -- tests/boundary/process/processCaptureProgress.test.ts src/process/capture/ProcessHarness.test.ts tests/boundary/process/processPreparation.test.ts - 3 files, 42 passed
npm run test:fast - 521 files, 7746 passed, 13 skipped, 0 failed
npm run build:cached then npm run test:boundary - 653 files passed, 2 failed, 19 skipped (6349 tests passed, 2 failed): tests/boundary/process/processSettlementDeadline.test.ts ("still reports observed liveness as alive_at_deadline") fails identically on unmodified upstream/main; tests/boundary/providers/GdbSessionReviewRegressions.test.ts passes (19 tests) when rerun alone. processCaptureProgress.test.ts passes (9 tests).
Mutation checks, each reverted: always reporting not_required fails test (2); reading the released report only for cancellations fails test (3)

macOS arm64 only; Linux and Windows were not executed.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; real process capture exercised on macOS arm64 as above.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new capability or input; the report states what preparation allocated and released, and never claims cleaned for a root that was not created.

Author readiness

Human owner: @nikolay-dementiev

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/3601 **Author:** [@nikolay-dementiev](https://github.com/nikolay-dementiev) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3169-preparation-cleanup-states` --- ### 📝 Commits (4) - [`492f339`](https://github.com/morluto/rea/commit/492f339407b39dd66bdfe202926d476800d8374c) test(process): expect cleanup states in the terminal progress of a failed preparation - [`c43a065`](https://github.com/morluto/rea/commit/c43a065fc41202afee411e7b35801ab94de1af9b) fix(process): report cleanup states when capture preparation fails - [`e2bc25c`](https://github.com/morluto/rea/commit/e2bc25c3d13ec6840b70fd55ff09e321efa9d165) test(process): expect a failed preparation to report the cleanup it released - [`391620e`](https://github.com/morluto/rea/commit/391620e794989ea5d4df983045f930e0cd241195) test(process): cover a failed root release, the error envelope and the terminal phase of a failed preparation ### 📊 Changes **2 files changed** (+177 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/process/capture/ProcessHarness.ts` (+20 -0) 📝 `tests/boundary/process/processCaptureProgress.test.ts` (+157 -2) </details> ### 📄 Description ## Summary When capture preparation fails, the final progress line reports the disposition but omits the `owned_process_group`, `terminal_renderer` and `temporary_root` cleanup states that `docs/process-capture.md` documents for the final line. The line now carries those states, reflecting what preparation actually allocated and released. Exit code, the `process_capture_failed` envelope and successful captures are unchanged. Fixes #3169 ## Problem and expected behavior `capture-process` with `TMPDIR` pointing at a nonexistent directory fails during preparation (on macOS the ownership inspector's `mkdtemp` is the first allocation). Its last stderr line is: ``` {"rea_progress":{"phase":"cleanup","completed":0,"total":null,"message":"elapsed_ms=1 frames=0 samples=0 interactions=0 disposition=failed","terminal":true,"sequence":2}} ``` A successful capture ends with `... disposition=exited owned_process_group=cleaned terminal_renderer=cleaned temporary_root=cleaned`, and `docs/process-capture.md` says the final line's message contains `disposition=<value>` and the three cleanup states, with `failed` among the documented dispositions. The `catch` around `prepareProcessCapture` in `runProcessScenario` reported the terminal state without a cleanup report, and `formatProgressMessage` appends the states only when one is supplied. Expected: the final line of a failed preparation carries the three states. Nothing was allocated when preparation failed before its first allocation (`not_required` for each resource); when it failed after allocating the temporary root, the states are those of the release it performed. ## Change and scope - `src/process/capture/ProcessHarness.ts`: the preparation `catch` now reports `cleanup: preparationCleanupReport(cause)`. `prepareProcessCapture` already releases the temporary root it allocated and throws a `ProcessCaptureError` that carries the resulting `cleanupReport`; the helper returns that report when the cause has one, and otherwise a constant report with all three resources `not_required`, because a failure without a report happened before anything was allocated. The error that is thrown is unchanged. Intentionally not included: no change to `prepareProcessCapture`, the error envelope (`process_capture_failed` keeps its fields), successful captures, schemas, CLI flags or documentation (`not_required` is part of the documented state vocabulary). ## Contract and boundary impact - Semantic owner and earliest changed stage: process capture harness, at the terminal progress report of a failed preparation. - Contribution scope fit and linked maintainer decision: not applicable (fix within an existing boundary). - CLI and MCP/tool-catalog contract: none. Only the text of the final progress message on this failure path gains the documented `key=value` states. - Provider, bridge, target-format, or platform compatibility: platform independent; checked on macOS arm64. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): preparation allocates the temporary root only after the ownership inspector and snapshot; a failure before it reports `not_required`, a failure after it reports the release result (`cleaned`, or `failed`/`unverified` with its reason). - Generated metadata, package, or installation impact: none. Expected effects: - [x] 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. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/process/processCaptureProgress.test.ts` (process-boundary lane). `runCaptureCli` gains an optional environment override. (1) The reporter's failure: `TMPDIR`, `TEMP` and `TMP` point at a missing directory; exit 1, a valid `process_capture_failed` envelope, exactly one monotonic terminal line with phase `cleanup`, `disposition=failed` and the three states `not_required`. (2) `captureProcessScenario` with a `captureSnapshot` that cancels after the snapshot, so preparation fails after the real temporary root exists: `disposition=cancelled`, `temporary_root=cleaned`, the other two `not_required`. (3) The same failure with a resource scope whose release reports the root as `failed`: the error is `cleanup_incomplete` and the line reports `temporary_root=failed`. - Base reproduction or other evidence: test (1) fails on `10aa74dfe` with `expected [ undefined, undefined, undefined ] to deeply equal [ "not_required", "not_required", "not_required" ]`. Real run on macOS 26.5 arm64, Node 26.3.0, after `npm run build:cached`: before, the failure command ended with the line above and exit 1; after, it ends with `{"rea_progress":{"phase":"cleanup","completed":0,"total":null,"message":"elapsed_ms=2 frames=0 samples=0 interactions=0 disposition=failed owned_process_group=not_required terminal_renderer=not_required temporary_root=not_required","terminal":true,"sequence":2}}` and exit 1, with the same envelope; the control with an existing `TMPDIR` still ends with `disposition=exited owned_process_group=cleaned terminal_renderer=cleaned temporary_root=cleaned`. - User-visible CLI/MCP output (if applicable): the final progress line above. - Remaining proof gaps: Linux and Windows were not executed; on those platforms the nonexistent `TMPDIR`/`TEMP`/`TMP` fails at the temporary root allocation, which reports the same states by the same path. 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 check:fast`, `npm run check` - green - `npm run jscpd` - 3.21% duplicated lines (threshold 3.5); `npm run scan:todos` - no tech-debt markers - `npm run test:focused -- tests/boundary/process/processCaptureProgress.test.ts src/process/capture/ProcessHarness.test.ts tests/boundary/process/processPreparation.test.ts` - 3 files, 42 passed - `npm run test:fast` - 521 files, 7746 passed, 13 skipped, 0 failed - `npm run build:cached` then `npm run test:boundary` - 653 files passed, 2 failed, 19 skipped (6349 tests passed, 2 failed): `tests/boundary/process/processSettlementDeadline.test.ts` ("still reports observed liveness as alive_at_deadline") fails identically on unmodified `upstream/main`; `tests/boundary/providers/GdbSessionReviewRegressions.test.ts` passes (19 tests) when rerun alone. `processCaptureProgress.test.ts` passes (9 tests). - Mutation checks, each reverted: always reporting `not_required` fails test (2); reading the released report only for cancellations fails test (3) macOS arm64 only; Linux and Windows were not executed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; real process capture exercised on macOS arm64 as above. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new capability or input; the report states what preparation allocated and released, and never claims `cleaned` for a root that was not created. ## Author readiness Human owner: @nikolay-dementiev - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:53 +00:00