morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #619] [CLOSED] fix(application): report the real PTY backend probe failure #2119

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/619
Author: @aniruddhaadak80
Created: 10/5/2026
Status: ❌ Closed

Base: main ← Head: fix/application-report-real-pty-backend-failure

📝 Commits (1)

9faea62 fix(application): report the real PTY backend probe failure

📊 Changes

2 files changed (+151 additions, -3 deletions)

View changed files

➕ src/application/ProcessCaptureCapability.test.ts (+79 -0)
📝 src/application/ProcessCaptureCapability.ts (+72 -3)

📄 Description

Summary

probeProcessCaptureCapability used a bare catch {}, so the reason it reported for an unavailable native PTY backend was a hardcoded sentence — the native PTY backend could not start a probe process — regardless of what actually failed.

The try covers three structurally different failures, and the message named only one of them. A user whose optional native dependency was never installed, or whose prebuilt backend was built against a different Node ABI, was told the backend failed to start a probe process — a claim about the probe shell, at a point in the sequence where the probe never ran. That directs remediation at the wrong thing: inspecting or replacing a shell that was never the problem.

Fixes #568

Problem and expected behavior

Smallest trigger: any error thrown inside the probe try — an ERR_MODULE_NOT_FOUND from the dynamic import, an ERR_DLOPEN_FAILED from binding load, or an OS error from spawn.

Violated invariant (AGENTS.md, Boundary Contracts): Preserve meaningful failure reasons through application and adapter layers and generic catches must not erase actionable validation details. A bare catch with a hardcoded string erases the distinction between an unavailable dependency, a load failure, and a real spawn failure — precisely the three cases the section names as needing to stay distinct.

Observed before this change — identical output for three different causes:

spawn failure  -> the native PTY backend could not start a probe process
missing package -> the native PTY backend could not start a probe process
abi failure     -> the native PTY backend could not start a probe process

Change and scope

The catch binds the error and routes it through a new exported pure classifier, nativePtyProbeFailureReason(failure, probeShell, host). It reports the observed failure kind and remediation appropriate to that kind:

missing optional package (ERR_MODULE_NOT_FOUND) — names @lydell/node-pty and the running platform/arch, and directs a reinstall for that platform
ABI / binding load failure (ERR_DLOPEN_FAILED, or a load failure reported without a code whose detail matches a native-loader message such as NODE_MODULE_VERSION or invalid ELF header) — names the running Node version and platform/arch, and directs rebuilding the backend for the running ABI
spawn failure (an OS error code) — names the code, the errno when present, and the probe shell actually used

An unrecognized failure keeps a useful non-empty reason that includes whatever detail was reported, rather than falling back to the misleading spawn claim.

probeShell is hoisted to a const so the reason can name it; the value passed to spawn is unchanged.

Not included: the missing timeout and missing kill()/dispose around await terminal.onExit in the same function is a separate defect that needs its own change and review, and is deliberately untouched here.

Contract and boundary impact

Semantic owner and earliest changed stage: src/application/ProcessCaptureCapability.ts — the capability-probe stage of the process-capture application boundary, at the optional-dependency import and probe spawn.
CLI and MCP/tool-catalog contract: unchanged. No tool is added, removed, or reshaped; no schema changes; TOOL_CONTRACTS and the generated catalog are untouched. Only the free-text reason string on available: false changes.
Provider, bridge, target-format, or platform compatibility: unchanged. The probe still runs on the same platforms with the same shell and argv; only the reported reason varies.
Evidence, artifact, provenance, or reconstruction contract: unchanged. This reason string is diagnostic text, not recorded evidence about a target; no artifact identity or provenance field is affected.
Process execution, authorization, cleanup, or containment impact: none. No new spawn, no new process authority, no cleanup path touched.
Generated metadata (docs/product-catalog.json), package, or installation impact: none. No contract signature changed.

Evidence and regression coverage

Tests added or updated: new src/application/ProcessCaptureCapability.test.ts (vitest project services, which covers src/application/**/*.test.ts per vitest.config.ts:41). The file previously had no colocated test. Six tests drive the classifier with synthetic errors carrying the documented code/errno shapes: one per failure kind, one asserting the missing-package reason names both @lydell/node-pty and the platform/arch, and one asserting an unrecognized error still yields a non-empty useful reason rather than throwing.
Base reproduction or other evidence: the real probe short-circuits on win32 before reaching the try, so the pre-fix reproduction ran the verbatim try/catch with only the two non-failable-on-demand seams substituted. Before: all three causes returned the identical hardcoded string. After: missing package names @lydell/node-pty and linux-x64; ABI failure names Node 24.18.0 and linux-x64; spawn failure names EACCES, errno -13, and /bin/sh.
User-visible CLI/MCP output: reason reaches the user through WindowsCapabilities.ts into doctor, so rea doctor on a host missing the native backend previously printed the native PTY backend could not start a probe process and now names the actual cause and its remediation.
Remaining proof gaps: no real broken-install path was executed. The classifier is validated against synthetic errors with the loader's documented code/errno shapes, not against a genuinely uninstalled or mismatched-ABI backend on a POSIX host.

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 --project services src/application/ProcessCaptureCapability.test.ts → 6 passed
npm run check:fast → 2/2 tasks successful, 0 errors, 98 warnings (unchanged baseline; no new warnings)
git stash push -- src/application/ProcessCaptureCapability.ts then re-running the new test file → 6 failed / 0 passed
npx vitest run --project services → 307 passed, 6 failed, 1 skipped. The 6 failures are Linux Hopper deb/rpm/arch, Mac Hopper install, ProcessEvidence environment preservation, and a ProcessScenario symlink test — all inherited host red on this Windows machine. Confirmed by stashing the source change and re-running those same 4 files: identical 6 failures.

Compatibility, safety, and release

Breaking changes or migration steps: none. The probe's success path, the available/backend fields, and the ownership-unavailability reason are all unchanged. Only the diagnostic text on available: false differs, and it now says more.
Real Hopper/Ghidra, browser, or OS coverage: none performed. No real Hopper run (npm run verify:hopper, verify:hopper:linux) was attempted; this change does not touch a provider. The POSIX probe path was exercised only through synthetic host values, since the probe short-circuits before the try on Windows.
Package or release metadata impact: none. CHANGELOG.md is release-please generated from PR titles and is not edited by hand.
Security, privacy, process, or containment review: no new I/O, no new dependency, no new subprocess. The classifier is pure and introduces no environment or filesystem reads of its own.

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.

_{🔄 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/619 **Author:** [@aniruddhaadak80](https://github.com/aniruddhaadak80) **Created:** 10/5/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/application-report-real-pty-backend-failure` --- ### 📝 Commits (1) - [`9faea62`](https://github.com/morluto/rea/commit/9faea62e54e7e47d715f563b413fa00fa3e8f3aa) fix(application): report the real PTY backend probe failure ### 📊 Changes **2 files changed** (+151 additions, -3 deletions) <details> <summary>View changed files</summary> ➕ `src/application/ProcessCaptureCapability.test.ts` (+79 -0) 📝 `src/application/ProcessCaptureCapability.ts` (+72 -3) </details> ### 📄 Description ## Summary `probeProcessCaptureCapability` used a bare `catch {}`, so the reason it reported for an unavailable native PTY backend was a hardcoded sentence — `the native PTY backend could not start a probe process` — regardless of what actually failed. The `try` covers three structurally different failures, and the message named only one of them. A user whose optional native dependency was never installed, or whose prebuilt backend was built against a different Node ABI, was told the backend failed to *start a probe process* — a claim about the probe shell, at a point in the sequence where the probe never ran. That directs remediation at the wrong thing: inspecting or replacing a shell that was never the problem. Fixes #568 ## Problem and expected behavior Smallest trigger: any error thrown inside the probe `try` — an `ERR_MODULE_NOT_FOUND` from the dynamic import, an `ERR_DLOPEN_FAILED` from binding load, or an OS error from `spawn`. Violated invariant (AGENTS.md, Boundary Contracts): *Preserve meaningful failure reasons through application and adapter layers* and *generic catches must not erase actionable validation details.* A bare catch with a hardcoded string erases the distinction between an unavailable dependency, a load failure, and a real spawn failure — precisely the three cases the section names as needing to stay distinct. Observed before this change — identical output for three different causes: ``` spawn failure -> the native PTY backend could not start a probe process missing package -> the native PTY backend could not start a probe process abi failure -> the native PTY backend could not start a probe process ``` ## Change and scope The catch binds the error and routes it through a new exported pure classifier, `nativePtyProbeFailureReason(failure, probeShell, host)`. It reports the observed failure kind and remediation appropriate to that kind: - **missing optional package** (`ERR_MODULE_NOT_FOUND`) — names `@lydell/node-pty` and the running platform/arch, and directs a reinstall for that platform - **ABI / binding load failure** (`ERR_DLOPEN_FAILED`, or a load failure reported without a code whose detail matches a native-loader message such as `NODE_MODULE_VERSION` or `invalid ELF header`) — names the running Node version and platform/arch, and directs rebuilding the backend for the running ABI - **spawn failure** (an OS error code) — names the code, the errno when present, and the probe shell actually used An unrecognized failure keeps a useful non-empty reason that includes whatever detail was reported, rather than falling back to the misleading spawn claim. `probeShell` is hoisted to a `const` so the reason can name it; the value passed to `spawn` is unchanged. Not included: the missing timeout and missing `kill()`/`dispose` around `await terminal.onExit` in the same function is a separate defect that needs its own change and review, and is deliberately untouched here. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/application/ProcessCaptureCapability.ts` — the capability-probe stage of the process-capture application boundary, at the optional-dependency import and probe spawn. - CLI and MCP/tool-catalog contract: unchanged. No tool is added, removed, or reshaped; no schema changes; `TOOL_CONTRACTS` and the generated catalog are untouched. Only the free-text `reason` string on `available: false` changes. - Provider, bridge, target-format, or platform compatibility: unchanged. The probe still runs on the same platforms with the same shell and argv; only the reported reason varies. - Evidence, artifact, provenance, or reconstruction contract: unchanged. This reason string is diagnostic text, not recorded evidence about a target; no artifact identity or provenance field is affected. - Process execution, authorization, cleanup, or containment impact: none. No new spawn, no new process authority, no cleanup path touched. - Generated metadata (`docs/product-catalog.json`), package, or installation impact: none. No contract signature changed. ## Evidence and regression coverage - Tests added or updated: new `src/application/ProcessCaptureCapability.test.ts` (vitest project `services`, which covers `src/application/**/*.test.ts` per `vitest.config.ts:41`). The file previously had no colocated test. Six tests drive the classifier with synthetic errors carrying the documented `code`/`errno` shapes: one per failure kind, one asserting the missing-package reason names both `@lydell/node-pty` and the platform/arch, and one asserting an unrecognized error still yields a non-empty useful reason rather than throwing. - Base reproduction or other evidence: the real probe short-circuits on `win32` before reaching the `try`, so the pre-fix reproduction ran the verbatim `try`/`catch` with only the two non-failable-on-demand seams substituted. Before: all three causes returned the identical hardcoded string. After: missing package names `@lydell/node-pty` and `linux-x64`; ABI failure names Node `24.18.0` and `linux-x64`; spawn failure names `EACCES`, `errno -13`, and `/bin/sh`. - User-visible CLI/MCP output: `reason` reaches the user through `WindowsCapabilities.ts` into `doctor`, so `rea doctor` on a host missing the native backend previously printed `the native PTY backend could not start a probe process` and now names the actual cause and its remediation. - Remaining proof gaps: no real broken-install path was executed. The classifier is validated against synthetic errors with the loader's documented `code`/`errno` shapes, not against a genuinely uninstalled or mismatched-ABI backend on a POSIX host. 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 --project services src/application/ProcessCaptureCapability.test.ts` → 6 passed - `npm run check:fast` → 2/2 tasks successful, 0 errors, 98 warnings (unchanged baseline; no new warnings) - `git stash push -- src/application/ProcessCaptureCapability.ts` then re-running the new test file → 6 failed / 0 passed - `npx vitest run --project services` → 307 passed, 6 failed, 1 skipped. The 6 failures are Linux Hopper deb/rpm/arch, Mac Hopper install, ProcessEvidence environment preservation, and a ProcessScenario symlink test — all inherited host red on this Windows machine. Confirmed by stashing the source change and re-running those same 4 files: identical 6 failures. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The probe's success path, the `available`/`backend` fields, and the ownership-unavailability reason are all unchanged. Only the diagnostic text on `available: false` differs, and it now says more. - Real Hopper/Ghidra, browser, or OS coverage: none performed. No real Hopper run (`npm run verify:hopper`, `verify:hopper:linux`) was attempted; this change does not touch a provider. The POSIX probe path was exercised only through synthetic host values, since the probe short-circuits before the `try` on Windows. - Package or release metadata impact: none. `CHANGELOG.md` is release-please generated from PR titles and is not edited by hand. - Security, privacy, process, or containment review: no new I/O, no new dependency, no new subprocess. The classifier is pure and introduces no environment or filesystem reads of its own. ## 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. --- <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:31 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:00:16 +00:00

[PR #2119] [MERGED] docs: update Chinese README guides #5042

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#2119

No description provided.

Rows
Columns