[PR #4836] [CLOSED] fix(process): gate PTY launch on cancellation after executable sampling (#4512) #6435

Closed
opened 2026-10-31 01:14:26 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4836
Author: @DeryFerd
Created: 10/17/2026
Status: ❌ Closed

Base: main ← Head: fix/4512-prelaunch-cancellation


📝 Commits (1)

  • 014afa3 fix(process): gate PTY launch on cancellation after executable sampling

📊 Changes

2 files changed (+117 additions, -0 deletions)

View changed files

📝 src/process/capture/ProcessHarness.ts (+5 -0)
➕ tests/process-global/processPrelaunchCancellation.test.ts (+112 -0)

📄 Description

Summary

Fixes #4512. Process capture could reach its PTY launch after the caller had already cancelled during the final executable metadata read or descriptor close. The sampler still returned a complete digest, and the caller consumed it and started the target anyway. The launch boundary now rechecks the caller's signal after the sample settles, so a cancelled request takes the existing typed cancellation path and makes zero launch attempts.

Problem and expected behavior

The smallest trigger is a cancellation that lands in the last awaited step of executable sampling. startCaptureRuntime in src/process/capture/ProcessHarness.ts awaits observeSelectedExecutable at line 271 and calls spawn at line 293 with nothing in between that reads the signal. Inside the sampler (src/process/capture/ProcessCaptureLifecycle.ts:512), the last internal check runs before the final handle.stat({ bigint: true }) at line 552, and the finally block then awaits handle?.close() at line 572. Either await can settle after the caller aborts.

I split that window into two schedules and measured each one against a real node:fs/promises handle on main. Aborting after the final descriptor stat still rejects with cancelled, because the sampler's own check runs afterwards. Aborting inside the descriptor close does not, because no check follows it. That second schedule resolved with a complete digest and signal.aborted === true, which is the exact state the caller then treated as permission to launch.

The violated invariant is in docs/tool-design.md#own-the-lifecycle: target admission, active execution, and resource release stay separate, and a completed prerequisite establishes only its own fact. A finished executable sample proves the file's bytes and identity. It does not authorize starting that program for an operation that has already been cancelled. The expected behavior is a typed cancellation, the existing startup cleanup, and no PTY launch.

Change and scope

One statement at the consuming boundary. assertNotCancelled(options.signal) now runs immediately after observeSelectedExecutable resolves, before the terminal renderer, sampler, and PTY are constructed. The helper already existed in this file's imports (src/process/capture/ProcessScenarioRuntimeValidation.ts:121) and throws the same processCaptureCancelled() error that every other startup check uses, so the failure reaches the existing cancellation and cleanup paths without a new error type.

The sampler is unchanged. It still returns the complete digest and identity it observed, still closes its descriptor, and still rejects when cancellation arrives before its last check. I deliberately did not move the check into the sampler or change its return value, because completed file observations remain valid evidence and discarding them would lose facts the caller can still report.

Not included: cancellation delivery from individual CLI or MCP adapters. This change covers the shared harness both surfaces call. Adapter-specific signal forwarding is a separate concern and was not tested here.

Ordinary defect fix within existing process-capture ownership. A new provider or protection-recovery scope decision is not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: shared process-capture startup in src/process/capture/ProcessHarness.ts; the earliest changed stage is the launch decision after executable sampling settles.
  • Contribution scope fit and linked maintainer decision: ordinary lifecycle fix inside existing boundaries; no maintainer scope decision applies.
  • CLI and MCP/tool-catalog contract: none. Input schemas, advertised operations, and result shapes are unchanged. A cancelled request returns the same typed ProcessCaptureError it already returned for earlier cancellation points.
  • Provider, bridge, target-format, or platform compatibility: none. The check is portable and uses the caller's existing AbortSignal. Native Windows capture remains unavailable for the reason already documented in ProcessCaptureCapability.ts.
  • Evidence, artifact, provenance, or reconstruction contract: completed executable sample facts are still produced and still reachable; the change only removes the launch that followed them. No Evidence field changes.
  • Target selection, authority, upstream behavior, and cleanup details: no new authority. A cancelled request never starts a target, so it never acquires a PID to signal. Startup resources that were already created still go through cleanupFailedStartup and releaseProcessResources exactly as before.
  • Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible.

  • 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/process-global/processPrelaunchCancellation.test.ts adds two cases against the exported captureProcessScenario boundary. The first aborts inside the real descriptor close during executable sampling, replaces the native PTY entry point with a throwing sentinel, and asserts the sentinel was never called and the result is a typed cancellation. The second is the non-aborted control and asserts the sentinel is reached exactly once, which proves the test would notice a harness that never launches anything.

The fixture keeps real open, stat, and close calls and wraps only the returned handle to synchronize the abort with that one boundary. Nothing reproduces launch logic in a helper. The native module is mocked at @lydell/node-pty so the sentinel throws before any child exists.

Base reproduction or other evidence: on the unpatched base the first case fails with expected 1 to be +0, meaning the launch sentinel was called once for an already-cancelled request. The control passes on the same base, so the failure is specific to the cancellation schedule and not to the fixture. With the check added, both cases pass.

User-visible CLI/MCP output: none observed directly. The practical effect is that a cancelled capture_process_scenario returns the existing cancellation error instead of starting the selected program and discovering the cancellation during later monitoring.

Remaining proof gaps: I did not run a native PTY capture on Linux or macOS, and I did not exercise public CLI or MCP cancellation delivery. This host is Windows, where native process capture is unavailable by design, so the portable startup logic runs against the controlled "linux" platform seam while the filesystem calls stay real. The frequency of this interleaving under normal filesystem timing is unknown; the regression pins the ordering rather than a timing distribution.

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

Host: Windows 11 x64, Node 24.21.0, npm 11.9.0. This checkout lacks the packaged Windows native addon, so native capture lanes are unavailable here.

  • npx vitest run --project process-global tests/process-global/processPrelaunchCancellation.test.ts — 2 passed.
  • Red-green check — reverting the one added statement makes the cancellation case fail with expected 1 to be +0; restoring it makes both cases pass. The control passes in both states.
  • npx vitest run --project process-boundary --project process-global tests/boundary/process/processCaptureLifecycle.test.ts tests/boundary/process/selectedExecutableCancellation.test.ts tests/boundary/process/processPreparation.test.ts tests/process-global/processPrelaunchCancellation.test.ts — 5 passed, 24 skipped (the PTY-dependent cases skip on this host).
  • npm run check:fast — typecheck and lint passed, 2 of 2 tasks.
  • git diff --check — no whitespace errors.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: none. No provider engine or browser lane was involved.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: this reduces authority. A cancelled request can no longer cross into execution, so no target is started and no PID is acquired. The change adds no new input, provider, or network surface.

Author readiness

Human owner: @DeryFerd

  • 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/4836 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/17/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/4512-prelaunch-cancellation` --- ### 📝 Commits (1) - [`014afa3`](https://github.com/morluto/rea/commit/014afa38929d46d58067b617c707759cd753cf8d) fix(process): gate PTY launch on cancellation after executable sampling ### 📊 Changes **2 files changed** (+117 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/process/capture/ProcessHarness.ts` (+5 -0) ➕ `tests/process-global/processPrelaunchCancellation.test.ts` (+112 -0) </details> ### 📄 Description ## Summary Fixes #4512. Process capture could reach its PTY launch after the caller had already cancelled during the final executable metadata read or descriptor close. The sampler still returned a complete digest, and the caller consumed it and started the target anyway. The launch boundary now rechecks the caller's signal after the sample settles, so a cancelled request takes the existing typed cancellation path and makes zero launch attempts. ## Problem and expected behavior The smallest trigger is a cancellation that lands in the last awaited step of executable sampling. `startCaptureRuntime` in `src/process/capture/ProcessHarness.ts` awaits `observeSelectedExecutable` at line 271 and calls `spawn` at line 293 with nothing in between that reads the signal. Inside the sampler (`src/process/capture/ProcessCaptureLifecycle.ts:512`), the last internal check runs before the final `handle.stat({ bigint: true })` at line 552, and the `finally` block then awaits `handle?.close()` at line 572. Either await can settle after the caller aborts. I split that window into two schedules and measured each one against a real `node:fs/promises` handle on `main`. Aborting after the final descriptor `stat` still rejects with `cancelled`, because the sampler's own check runs afterwards. Aborting inside the descriptor `close` does not, because no check follows it. That second schedule resolved with a complete digest and `signal.aborted === true`, which is the exact state the caller then treated as permission to launch. The violated invariant is in `docs/tool-design.md#own-the-lifecycle`: target admission, active execution, and resource release stay separate, and a completed prerequisite establishes only its own fact. A finished executable sample proves the file's bytes and identity. It does not authorize starting that program for an operation that has already been cancelled. The expected behavior is a typed cancellation, the existing startup cleanup, and no PTY launch. ## Change and scope One statement at the consuming boundary. `assertNotCancelled(options.signal)` now runs immediately after `observeSelectedExecutable` resolves, before the terminal renderer, sampler, and PTY are constructed. The helper already existed in this file's imports (`src/process/capture/ProcessScenarioRuntimeValidation.ts:121`) and throws the same `processCaptureCancelled()` error that every other startup check uses, so the failure reaches the existing cancellation and cleanup paths without a new error type. The sampler is unchanged. It still returns the complete digest and identity it observed, still closes its descriptor, and still rejects when cancellation arrives before its last check. I deliberately did not move the check into the sampler or change its return value, because completed file observations remain valid evidence and discarding them would lose facts the caller can still report. Not included: cancellation delivery from individual CLI or MCP adapters. This change covers the shared harness both surfaces call. Adapter-specific signal forwarding is a separate concern and was not tested here. Ordinary defect fix within existing process-capture ownership. A new provider or protection-recovery scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: shared process-capture startup in `src/process/capture/ProcessHarness.ts`; the earliest changed stage is the launch decision after executable sampling settles. - Contribution scope fit and linked maintainer decision: ordinary lifecycle fix inside existing boundaries; no maintainer scope decision applies. - CLI and MCP/tool-catalog contract: none. Input schemas, advertised operations, and result shapes are unchanged. A cancelled request returns the same typed `ProcessCaptureError` it already returned for earlier cancellation points. - Provider, bridge, target-format, or platform compatibility: none. The check is portable and uses the caller's existing `AbortSignal`. Native Windows capture remains unavailable for the reason already documented in `ProcessCaptureCapability.ts`. - Evidence, artifact, provenance, or reconstruction contract: completed executable sample facts are still produced and still reachable; the change only removes the launch that followed them. No Evidence field changes. - Target selection, authority, upstream behavior, and cleanup details: no new authority. A cancelled request never starts a target, so it never acquires a PID to signal. Startup resources that were already created still go through `cleanupFailedStartup` and `releaseProcessResources` exactly as before. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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/process-global/processPrelaunchCancellation.test.ts` adds two cases against the exported `captureProcessScenario` boundary. The first aborts inside the real descriptor `close` during executable sampling, replaces the native PTY entry point with a throwing sentinel, and asserts the sentinel was never called and the result is a typed cancellation. The second is the non-aborted control and asserts the sentinel is reached exactly once, which proves the test would notice a harness that never launches anything. The fixture keeps real `open`, `stat`, and `close` calls and wraps only the returned handle to synchronize the abort with that one boundary. Nothing reproduces launch logic in a helper. The native module is mocked at `@lydell/node-pty` so the sentinel throws before any child exists. Base reproduction or other evidence: on the unpatched base the first case fails with `expected 1 to be +0`, meaning the launch sentinel was called once for an already-cancelled request. The control passes on the same base, so the failure is specific to the cancellation schedule and not to the fixture. With the check added, both cases pass. User-visible CLI/MCP output: none observed directly. The practical effect is that a cancelled `capture_process_scenario` returns the existing cancellation error instead of starting the selected program and discovering the cancellation during later monitoring. Remaining proof gaps: I did not run a native PTY capture on Linux or macOS, and I did not exercise public CLI or MCP cancellation delivery. This host is Windows, where native process capture is unavailable by design, so the portable startup logic runs against the controlled `"linux"` platform seam while the filesystem calls stay real. The frequency of this interleaving under normal filesystem timing is unknown; the regression pins the ordering rather than a timing distribution. 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 Host: Windows 11 x64, Node 24.21.0, npm 11.9.0. This checkout lacks the packaged Windows native addon, so native capture lanes are unavailable here. - `npx vitest run --project process-global tests/process-global/processPrelaunchCancellation.test.ts` — 2 passed. - Red-green check — reverting the one added statement makes the cancellation case fail with `expected 1 to be +0`; restoring it makes both cases pass. The control passes in both states. - `npx vitest run --project process-boundary --project process-global tests/boundary/process/processCaptureLifecycle.test.ts tests/boundary/process/selectedExecutableCancellation.test.ts tests/boundary/process/processPreparation.test.ts tests/process-global/processPrelaunchCancellation.test.ts` — 5 passed, 24 skipped (the PTY-dependent cases skip on this host). - `npm run check:fast` — typecheck and lint passed, 2 of 2 tasks. - `git diff --check` — no whitespace errors. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none. No provider engine or browser lane was involved. - Package or release metadata impact: none. - Security, privacy, process, or containment review: this reduces authority. A cancelled request can no longer cross into execution, so no target is started and no PID is acquired. The change adds no new input, provider, or network surface. ## Author readiness Human owner: @DeryFerd - [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 - [ ] 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:14:26 +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#6435
No description provided.