[PR #5816] [MERGED] fix(hopper): keep an observed launcher exit over a later cancellation #7082

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5816
Author: @XiaoBinGan
Created: 10/21/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: fix/hopper-observed-launcher-exit


📝 Commits (4)

  • 148d339 fix(hopper): keep an observed launcher exit over a later cancellation
  • fcd01c4 Merge remote-tracking branch 'gh/main' into fix/hopper-observed-launcher-exit
  • a43bf27 Merge remote-tracking branch 'gh/main' into fix/hopper-observed-launcher-exit
  • 82b9d69 fix(hopper): arbitrate startup exits through process owner

📊 Changes

3 files changed (+272 additions, -25 deletions)

View changed files

📝 src/hopper/HopperClient.ts (+98 -24)
📝 tests/boundary/providers/hopper/hopperClient.startupWaiters.test.ts (+53 -1)
➕ tests/boundary/providers/hopper/hopperClientObservedExitPriority.test.ts (+121 -0)

📄 Description

Summary

HopperClient.#connect projected every interrupted wait as #startupInterruption without consulting the launcher the owner already held. A launcher that exited 3 before the caller aborted was therefore reported as HopperCancelledError, losing the exit code, stage and launcher diagnostics.

Fixes #5429

Problem and expected behavior

  • Smallest trigger: an owned launcher-process launcher exits 3 on its own; the supervisor records the exit; a setImmediate abort follows. start(signal) returns HopperCancelledError instead of the HopperProcessError the identical no-abort control returns (exitCode: 3, stage: "launch").
  • Violated invariant: docs/tool-design.md requires cancellation to stay distinguishable from a physical failure the owner already observed, and requires the observed outcome to remain primary.
  • Expected: a launcher exit or signal the owned process already holds stays primary when the interruption arrives afterwards; a cancellation observed first stays a cancellation; provider state, launcherFailure diagnostics and cleanup ownership keep their behaviour.

Change and scope

  • src/hopper/HopperClient.ts: a new #observedLauncherExit reports the exit code (including a signalled child) the owned process already holds, and #interruptionFailure projects the launcher startup failure before any interruption. The #connect loop and its two wait sites use it; the socket, health and authentication paths are untouched.
  • tests/boundary/providers/hopper/hopperClientObservedExitPriority.test.ts covers the report (launcher that exits 3, abort scheduled from its own exit listener) and the no-abort control.
  • Not included: no change to the bridge protocol, socket handling, health handshake or the ownership/cleanup contract.

Contract and boundary impact

  • Semantic owner and earliest changed stage: Hopper startup outcome projection in #connect.
  • Contribution scope fit and linked maintainer decision: not applicable — an ordinary fix inside an existing boundary.
  • CLI and MCP/tool-catalog contract: no schema or tool-catalog change; a failed Hopper startup now reports its launcher exit instead of a cancellation.
  • Provider, bridge, target-format, or platform compatibility: Hopper only; external (non-owning) launcher handling keeps its existing branches.
  • Evidence, artifact, provenance, or reconstruction contract: the launcher exit code and failure diagnostics are preserved instead of being replaced.
  • Target selection, authority, upstream behavior, and cleanup details: unchanged; no process is signalled differently.
  • Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).

  • 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: hopperClientObservedExitPriority.test.ts (new).
  • Base reproduction or other evidence: on d532b1812 the after-exit case fails with expected 'HopperCancelledError' to be 'HopperProcessError', matching #5429, while the control passes. After the change both pass and the error carries exitCode: 3 and stage: "launch".
  • User-visible CLI/MCP output (if applicable): a failed Hopper startup now reports the launcher exit and its diagnostics instead of cancelled.
  • Remaining proof gaps: the launcher is an authored process.exit(3) peer, not a real Hopper launch; no real Hopper installation was exercised.

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

  • npx vitest run tests/boundary/providers/hopper src/hopper — 35 files, 253 passed, 6 skipped, including the new test and the existing non-owning launcher signal case that shares this projection.
  • npm run format:check — all matched files use the correct format.
  • npm run verify:test-discovery — all 1721 test files discovered exactly once.
  • npm run typecheck — no diagnostic in the changed files (the remaining errors in this checkout are pre-existing protobuf-module resolution failures).
  • npm run lint — 0 errors.

Compatibility, safety, and release

  • Breaking changes or migration steps: a client that treated an aborted launch as cancelled now receives the physical launcher failure; cancellation still wins when it is observed first.
  • Real Hopper coverage: none claimed.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; no signal, ownership or containment change.

Author readiness

Human owner: @XiaoBinGan

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

Maintainer blocker verification

Startup arbitration now records the first outcome observed by the existing ProviderProcessSupervisor owner, including exit observed during the final lineage handshake. A physical launcher exit cannot be overwritten by later cancellation; cancellation observed first remains primary even if cleanup subsequently kills the launcher. Launcher and detached-helper lifetime semantics remain distinct.

The focused Hopper startup suite passed 12 tests and npm run check:fast passed on Node 24.18.0/npm 11.16.0. Both event orderings and late handshake are covered. No real Hopper installation was exercised; these are supervised-process boundary fixtures.


🔄 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/5816 **Author:** [@XiaoBinGan](https://github.com/XiaoBinGan) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/hopper-observed-launcher-exit` --- ### 📝 Commits (4) - [`148d339`](https://github.com/morluto/rea/commit/148d33945f091e06b9c524dc0003d2d4744a9f29) fix(hopper): keep an observed launcher exit over a later cancellation - [`fcd01c4`](https://github.com/morluto/rea/commit/fcd01c46b6cba92c4f6288f060306c3685d6e407) Merge remote-tracking branch 'gh/main' into fix/hopper-observed-launcher-exit - [`a43bf27`](https://github.com/morluto/rea/commit/a43bf2791265ff56a5455d87a80331f476b9679e) Merge remote-tracking branch 'gh/main' into fix/hopper-observed-launcher-exit - [`82b9d69`](https://github.com/morluto/rea/commit/82b9d6920dae6fa14cedb2557fde16f1cba6b4e0) fix(hopper): arbitrate startup exits through process owner ### 📊 Changes **3 files changed** (+272 additions, -25 deletions) <details> <summary>View changed files</summary> 📝 `src/hopper/HopperClient.ts` (+98 -24) 📝 `tests/boundary/providers/hopper/hopperClient.startupWaiters.test.ts` (+53 -1) ➕ `tests/boundary/providers/hopper/hopperClientObservedExitPriority.test.ts` (+121 -0) </details> ### 📄 Description ## Summary `HopperClient.#connect` projected every interrupted wait as `#startupInterruption` without consulting the launcher the owner already held. A launcher that exited 3 before the caller aborted was therefore reported as `HopperCancelledError`, losing the exit code, stage and launcher diagnostics. Fixes #5429 ## Problem and expected behavior - Smallest trigger: an owned `launcher-process` launcher exits 3 on its own; the supervisor records the exit; a `setImmediate` abort follows. `start(signal)` returns `HopperCancelledError` instead of the `HopperProcessError` the identical no-abort control returns (`exitCode: 3`, `stage: "launch"`). - Violated invariant: `docs/tool-design.md` requires cancellation to stay distinguishable from a physical failure the owner already observed, and requires the observed outcome to remain primary. - Expected: a launcher exit or signal the owned process already holds stays primary when the interruption arrives afterwards; a cancellation observed first stays a cancellation; provider state, `launcherFailure` diagnostics and cleanup ownership keep their behaviour. ## Change and scope - `src/hopper/HopperClient.ts`: a new `#observedLauncherExit` reports the exit code (including a signalled child) the owned process already holds, and `#interruptionFailure` projects the launcher startup failure before any interruption. The `#connect` loop and its two wait sites use it; the socket, health and authentication paths are untouched. - `tests/boundary/providers/hopper/hopperClientObservedExitPriority.test.ts` covers the report (launcher that exits 3, abort scheduled from its own `exit` listener) and the no-abort control. - Not included: no change to the bridge protocol, socket handling, health handshake or the ownership/cleanup contract. ## Contract and boundary impact - Semantic owner and earliest changed stage: Hopper startup outcome projection in `#connect`. - Contribution scope fit and linked maintainer decision: not applicable — an ordinary fix inside an existing boundary. - CLI and MCP/tool-catalog contract: no schema or tool-catalog change; a failed Hopper startup now reports its launcher exit instead of a cancellation. - Provider, bridge, target-format, or platform compatibility: Hopper only; external (non-owning) launcher handling keeps its existing branches. - Evidence, artifact, provenance, or reconstruction contract: the launcher exit code and failure diagnostics are preserved instead of being replaced. - Target selection, authority, upstream behavior, and cleanup details: unchanged; no process is signalled differently. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). - [ ] 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: `hopperClientObservedExitPriority.test.ts` (new). - Base reproduction or other evidence: on `d532b1812` the after-exit case fails with `expected 'HopperCancelledError' to be 'HopperProcessError'`, matching #5429, while the control passes. After the change both pass and the error carries `exitCode: 3` and `stage: "launch"`. - User-visible CLI/MCP output (if applicable): a failed Hopper startup now reports the launcher exit and its diagnostics instead of `cancelled`. - Remaining proof gaps: the launcher is an authored `process.exit(3)` peer, not a real Hopper launch; no real Hopper installation was exercised. 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 - `npx vitest run tests/boundary/providers/hopper src/hopper` — 35 files, 253 passed, 6 skipped, including the new test and the existing non-owning launcher signal case that shares this projection. - `npm run format:check` — all matched files use the correct format. - `npm run verify:test-discovery` — all 1721 test files discovered exactly once. - `npm run typecheck` — no diagnostic in the changed files (the remaining errors in this checkout are pre-existing protobuf-module resolution failures). - `npm run lint` — 0 errors. ## Compatibility, safety, and release - Breaking changes or migration steps: a client that treated an aborted launch as `cancelled` now receives the physical launcher failure; cancellation still wins when it is observed first. - Real Hopper coverage: none claimed. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; no signal, ownership or containment change. ## Author readiness Human owner: @XiaoBinGan - [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. <!-- maintainer-blocker-review --> ### Maintainer blocker verification Startup arbitration now records the first outcome observed by the existing `ProviderProcessSupervisor` owner, including exit observed during the final lineage handshake. A physical launcher exit cannot be overwritten by later cancellation; cancellation observed first remains primary even if cleanup subsequently kills the launcher. Launcher and detached-helper lifetime semantics remain distinct. The focused Hopper startup suite passed 12 tests and `npm run check:fast` passed on Node 24.18.0/npm 11.16.0. Both event orderings and late handshake are covered. No real Hopper installation was exercised; these are supervised-process boundary fixtures. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:45:15 +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#7082
No description provided.