[PR #4486] [CLOSED] fix(cutter): admit bridge descriptors without blocking on pipes #6271

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

📋 Pull Request Information

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

Base: main ← Head: fix/4471-cutter-descriptor-nonblocking-admission


📝 Commits (1)

  • 5333a85 fix(cutter): admit bridge descriptors without blocking on pipes

📊 Changes

5 files changed (+734 additions, -155 deletions)

View changed files

📝 src/contracts/cutterToolContracts.ts (+3 -0)
📝 src/cutter/CutterBridgeClient.ts (+259 -105)
📝 src/cutter/CutterBridgeService.ts (+125 -50)
📝 tests/boundary/providers/CutterBridgeClient.test.ts (+165 -0)
📝 tests/boundary/providers/CutterBridgeService.test.ts (+182 -0)

📄 Description

Summary

POSIX Cutter descriptor discovery opened every filename matching the descriptor pattern with blocking O_RDONLY | O_NOFOLLOW and only then checked info.isFile(). A FIFO without a writer never reaches that check, so discovery, command selection and completion lookup could remain pending before the bounded socket request started. This change admits each candidate through the canonical nonblocking regular-file primitive, propagates the caller's cancellation signal through the discovery chain, and retains a descriptor whose close could not be confirmed so a later cleanup can retry it.

Refs #4471.

Problem and expected behavior

The trigger is a descriptor-shaped filename that is not a regular file, or a discovery call that is cancelled while it runs. src/cutter/CutterBridgeClient.ts #readDescriptors enumerated matching names and, on POSIX, called open(..., O_RDONLY | O_NOFOLLOW) before any type check, so a writer-free FIFO blocked that open indefinitely. Separately, listSessions, listDescriptors, #find and #readDescriptors did not accept or forward the caller's AbortSignal, and a descriptor whose close failed was discarded with .catch(() => undefined).

The violated invariant is that candidate admission must not block on a non-regular file, that a cancelled discovery must settle promptly before a socket probe, and that a failed descriptor close must keep its ownership. The expected behavior is that a non-regular candidate is skipped without hanging, that an authenticated regular descriptor stays discoverable, that a cancelled discovery rejects before any socket request, and that a failed close is retained and retried rather than discarded.

Change and scope

#readDescriptors now admits each POSIX candidate with openRegularFile(..., { symlinks: "reject", signal }), which opens with O_NONBLOCK | O_NOCTTY, validates the opened descriptor as a regular file, and retains the handle on a failed close. NonRegularFileReadError skips the candidate; RegularFileAdmissionFailure and a failed OwnedFileHandle.close() retain the owner in #descriptorOwners. The existing size, ownership (uid) and permission (mode & 0o077) checks are preserved, and the Windows path keeps its separate native descriptor reader.

listSessions, listDescriptors, #find and #readDescriptors accept an optional AbortSignal and call throwIfAborted at the directory read, before each candidate read, and before each status socket probe. The client gains retryCleanup(), which retries every retained descriptor close and reports whether all owners are confirmed closed.

CutterBridgeService.listSessions forwards the signal, and execute refuses to start a command while a retained descriptor cleanup is unconfirmed, returning a capability-unavailable error naming the retained resource. The CLI list-cutter-sessions command now wraps discovery in withCommandCancellation, and the MCP list_cutter_sessions tool forwards the request signal. The separate socket-probe cancellation report (#3946) and the firmware admission pattern (#3887) are not addressed here.

Contract and boundary impact

  • Semantic owner and earliest changed stage: Cutter descriptor admission in src/cutter/CutterBridgeClient.ts; the earliest changed stage is candidate open before descriptor parsing.
  • Contribution scope fit and linked maintainer decision: ordinary defect fix within existing Cutter client and service ownership; a separate maintainer scope decision is not applicable.
  • CLI and MCP/tool-catalog contract: no tool, schema, or catalog change; list-cutter-sessions and list_cutter_sessions still report the same session fields and now honor cancellation.
  • Provider, bridge, target-format, or platform compatibility: POSIX admission path only; Windows keeps its native reader and POSIX-only checks are unchanged.
  • Evidence, artifact, provenance, or reconstruction contract: descriptor parsing, session identity and provenance are unchanged; a command is withheld while a retained descriptor cleanup is unconfirmed.
  • Target selection, authority, upstream behavior, and cleanup details: a failed descriptor close retains its owner through OwnedFileHandle and is retried by retryCleanup(); no target execution is authorized from a filename.
  • 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/boundary/providers/CutterBridgeClient.test.ts gains a POSIX-gated case that creates a writer-free FIFO named like a descriptor alongside a live regular descriptor and asserts discovery settles with the live session and no hang, using the existing readWithoutFifoWriter fixture so a regression releases the blocking reader; and a case asserting a pre-aborted listSessions rejects. tests/boundary/providers/CutterBridgeService.test.ts gains a case that a retained descriptor cleanup blocks a command until retryCleanup confirms release, and a case that the discovery signal is forwarded to the bridge port.
  • Base reproduction or other evidence: the report is source-derived and no runtime reproduction was performed by the reporter. This change was not reproduced on a POSIX host here either.
  • User-visible CLI/MCP output (if applicable): not run.
  • Remaining proof gaps: no POSIX FIFO reproduction was executed locally. This host is Windows x64, where mkfifo (MSYS) does not create a FIFO visible to native Node, so the FIFO case runs only in POSIX CI. Four pre-existing cases in CutterBridgeClient.test.ts also fail on Windows identically with this change stashed (directory-read mocking, O_NOFOLLOW-absent mocking, and platform checks), so that file is not a clean local signal. The O_NOFOLLOW-absent and close-failure cases mock node:fs/node:fs/promises, which the moved admission logic still imports, but their behavior under POSIX CI was not confirmed here.

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 tsc --noEmit -p tsconfig.json — 0 errors.
  • npx oxlint on the changed files — 0 errors.
  • npx oxfmt --check on the changed files — format clean.
  • npx vitest run tests/boundary/providers/CutterBridgeService.test.ts --project boundary — 11 passed, including the new cleanup-gating and signal-forwarding cases.
  • npx vitest run tests/boundary/providers/CutterBridgeClient.test.ts tests/boundary/providers/CutterBridgeService.test.ts --project boundary — 37 passed, 4 failed, 1 skipped on Windows. The 4 failures reproduce identically with this change stashed, so they are pre-existing on this host. The FIFO case is skipped on Windows.
  • POSIX FIFO and cancellation verification — not executed locally (see remaining proof gaps).

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: none; POSIX admission change with no runtime reproduction yet.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: admission now uses the canonical nonblocking primitive with O_NONBLOCK | O_NOCTTY and O_NOFOLLOW; ownership, mode and byte-limit checks are preserved, a failed close retains its owner, and a retained descriptor withholds a new command until release.

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/4486 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/4471-cutter-descriptor-nonblocking-admission` --- ### 📝 Commits (1) - [`5333a85`](https://github.com/morluto/rea/commit/5333a8536ff2f3d8cf7298d99ae6ce88c96c9439) fix(cutter): admit bridge descriptors without blocking on pipes ### 📊 Changes **5 files changed** (+734 additions, -155 deletions) <details> <summary>View changed files</summary> 📝 `src/contracts/cutterToolContracts.ts` (+3 -0) 📝 `src/cutter/CutterBridgeClient.ts` (+259 -105) 📝 `src/cutter/CutterBridgeService.ts` (+125 -50) 📝 `tests/boundary/providers/CutterBridgeClient.test.ts` (+165 -0) 📝 `tests/boundary/providers/CutterBridgeService.test.ts` (+182 -0) </details> ### 📄 Description ## Summary POSIX Cutter descriptor discovery opened every filename matching the descriptor pattern with blocking `O_RDONLY | O_NOFOLLOW` and only then checked `info.isFile()`. A FIFO without a writer never reaches that check, so discovery, command selection and completion lookup could remain pending before the bounded socket request started. This change admits each candidate through the canonical nonblocking regular-file primitive, propagates the caller's cancellation signal through the discovery chain, and retains a descriptor whose close could not be confirmed so a later cleanup can retry it. Refs #4471. ## Problem and expected behavior The trigger is a descriptor-shaped filename that is not a regular file, or a discovery call that is cancelled while it runs. `src/cutter/CutterBridgeClient.ts` `#readDescriptors` enumerated matching names and, on POSIX, called `open(..., O_RDONLY | O_NOFOLLOW)` before any type check, so a writer-free FIFO blocked that open indefinitely. Separately, `listSessions`, `listDescriptors`, `#find` and `#readDescriptors` did not accept or forward the caller's `AbortSignal`, and a descriptor whose close failed was discarded with `.catch(() => undefined)`. The violated invariant is that candidate admission must not block on a non-regular file, that a cancelled discovery must settle promptly before a socket probe, and that a failed descriptor close must keep its ownership. The expected behavior is that a non-regular candidate is skipped without hanging, that an authenticated regular descriptor stays discoverable, that a cancelled discovery rejects before any socket request, and that a failed close is retained and retried rather than discarded. ## Change and scope `#readDescriptors` now admits each POSIX candidate with `openRegularFile(..., { symlinks: "reject", signal })`, which opens with `O_NONBLOCK | O_NOCTTY`, validates the opened descriptor as a regular file, and retains the handle on a failed close. `NonRegularFileReadError` skips the candidate; `RegularFileAdmissionFailure` and a failed `OwnedFileHandle.close()` retain the owner in `#descriptorOwners`. The existing size, ownership (`uid`) and permission (`mode & 0o077`) checks are preserved, and the Windows path keeps its separate native descriptor reader. `listSessions`, `listDescriptors`, `#find` and `#readDescriptors` accept an optional `AbortSignal` and call `throwIfAborted` at the directory read, before each candidate read, and before each status socket probe. The client gains `retryCleanup()`, which retries every retained descriptor close and reports whether all owners are confirmed closed. `CutterBridgeService.listSessions` forwards the signal, and `execute` refuses to start a command while a retained descriptor cleanup is unconfirmed, returning a capability-unavailable error naming the retained resource. The CLI `list-cutter-sessions` command now wraps discovery in `withCommandCancellation`, and the MCP `list_cutter_sessions` tool forwards the request signal. The separate socket-probe cancellation report (#3946) and the firmware admission pattern (#3887) are not addressed here. ## Contract and boundary impact - Semantic owner and earliest changed stage: Cutter descriptor admission in `src/cutter/CutterBridgeClient.ts`; the earliest changed stage is candidate open before descriptor parsing. - Contribution scope fit and linked maintainer decision: ordinary defect fix within existing Cutter client and service ownership; a separate maintainer scope decision is not applicable. - CLI and MCP/tool-catalog contract: no tool, schema, or catalog change; `list-cutter-sessions` and `list_cutter_sessions` still report the same session fields and now honor cancellation. - Provider, bridge, target-format, or platform compatibility: POSIX admission path only; Windows keeps its native reader and POSIX-only checks are unchanged. - Evidence, artifact, provenance, or reconstruction contract: descriptor parsing, session identity and provenance are unchanged; a command is withheld while a retained descriptor cleanup is unconfirmed. - Target selection, authority, upstream behavior, and cleanup details: a failed descriptor close retains its owner through `OwnedFileHandle` and is retried by `retryCleanup()`; no target execution is authorized from a filename. - 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. - [ ] 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/providers/CutterBridgeClient.test.ts` gains a POSIX-gated case that creates a writer-free FIFO named like a descriptor alongside a live regular descriptor and asserts discovery settles with the live session and no hang, using the existing `readWithoutFifoWriter` fixture so a regression releases the blocking reader; and a case asserting a pre-aborted `listSessions` rejects. `tests/boundary/providers/CutterBridgeService.test.ts` gains a case that a retained descriptor cleanup blocks a command until `retryCleanup` confirms release, and a case that the discovery signal is forwarded to the bridge port. - Base reproduction or other evidence: the report is source-derived and no runtime reproduction was performed by the reporter. This change was not reproduced on a POSIX host here either. - User-visible CLI/MCP output (if applicable): not run. - Remaining proof gaps: **no POSIX FIFO reproduction was executed locally.** This host is Windows x64, where `mkfifo` (MSYS) does not create a FIFO visible to native Node, so the FIFO case runs only in POSIX CI. Four pre-existing cases in `CutterBridgeClient.test.ts` also fail on Windows identically with this change stashed (directory-read mocking, `O_NOFOLLOW`-absent mocking, and platform checks), so that file is not a clean local signal. The `O_NOFOLLOW`-absent and close-failure cases mock `node:fs`/`node:fs/promises`, which the moved admission logic still imports, but their behavior under POSIX CI was not confirmed here. 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 tsc --noEmit -p tsconfig.json` — 0 errors. - `npx oxlint` on the changed files — 0 errors. - `npx oxfmt --check` on the changed files — format clean. - `npx vitest run tests/boundary/providers/CutterBridgeService.test.ts --project boundary` — 11 passed, including the new cleanup-gating and signal-forwarding cases. - `npx vitest run tests/boundary/providers/CutterBridgeClient.test.ts tests/boundary/providers/CutterBridgeService.test.ts --project boundary` — 37 passed, 4 failed, 1 skipped on Windows. The 4 failures reproduce identically with this change stashed, so they are pre-existing on this host. The FIFO case is skipped on Windows. - POSIX FIFO and cancellation verification — not executed locally (see remaining proof gaps). ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none; POSIX admission change with no runtime reproduction yet. - Package or release metadata impact: none. - Security, privacy, process, or containment review: admission now uses the canonical nonblocking primitive with `O_NONBLOCK | O_NOCTTY` and `O_NOFOLLOW`; ownership, mode and byte-limit checks are preserved, a failed close retains its owner, and a retained descriptor withholds a new command until release. ## 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:13:11 +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#6271
No description provided.