[PR #3265] [CLOSED] fix(target): classify cancellation during file admission #5685

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3265
Author: @SII-YDD
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/target-admission-cancellation


📝 Commits (1)

  • 136a9ed fix(target): classify cancellation during file admission

📊 Changes

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

View changed files

📝 src/application/BinaryTargetResolver.ts (+4 -0)
➕ tests/process-global/binaryTargetAdmissionCancellation.test.ts (+256 -0)

📄 Description

Summary

Fixes #3197. Cancelling file admission during its first descriptor stat now returns the existing cancelled diagnostic instead of UNKNOWN or a raw rejected cancellation value.

Problem and expected behavior

Run rea inspect sample.bin --format json or rea analyze sample.bin --format json and send SIGINT after the first real FileHandle.stat() completes, before admission continues. Previously both commands reported UNKNOWN; cancellation after the resolver's second stat already reported cancelled. SDK cancellation of open_binary could likewise leave the server-side operation rejected with its raw signal reason.

The resolver's declared result includes AnalysisCancelledError, so both cancellation checkpoints should settle with that error after successful descriptor cleanup. The issue reporter describes P2 impact; this PR does not claim an upstream P1 classification.

Change and scope

parseBinaryTarget now recognizes a caught value that is exactly the aborted signal's reason and wraps it as AnalysisCancelledError("resolve_binary_target"). openRegularFile remains a generic filesystem primitive using Node's cancellation semantics; the application resolver owns this domain normalization.

The identity check preserves unrelated I/O failures occurring alongside cancellation and does not hide the RegularFileAdmissionFailure wrapper or its retained cleanup owner. Existing tagged cancellation errors keep their metadata. No new provider, execution capability, or configuration is introduced; additional maintainer scope authorization is not applicable to this correction within the existing admission boundary.

Contract and boundary impact

  • Semantic owner and earliest changed stage: application target resolution, catching an abort from regular-file admission.

  • Contribution scope fit and linked maintainer decision: ordinary correctness fix within an existing boundary; additional scope decision not applicable.

  • CLI and MCP/tool-catalog contract: existing cancelled error shape and operation; tool names, parameters and advertised schemas unchanged.

  • Provider, bridge, target-format, or platform compatibility: provider dispatch and format selection unchanged; cancellation happens before provider creation.

  • Evidence, artifact, provenance, or reconstruction contract: source bytes unchanged; no new evidence or reconstruction representation.

  • Target selection, authority, upstream behavior, and cleanup details: caller selections unchanged; failed admission closes its real descriptor, and a failed close retains its existing error and owner for retry.

  • Generated metadata, package, or installation impact: none; generated documentation checks passed without authored metadata changes.

  • 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

  • Added ten regressions using real open/stat/close operations, controlling only cancellation timing and injected failure cases. Default, Error and string reasons cover both stat checkpoints. Controls cover unsupported input, concurrent unrelated EIO, and failed close with retained ownership and successful close retry.

  • On unmodified base 7375ab6e34e511095a74d5a67e57cec28d56fd10, four regressions failed and six controls passed. The early cancellations threw raw reasons, including the actual SDK server signal's string reason. The corrected tests all pass, including on synchronized base 29d3dcb7ea85b3be37cd81e977ae4e09cebdd2fc.

  • Real SDK open_binary uses the existing application MCP harness and real session implementation. The test validates advertised input arguments, sends SDK cancellation, waits for the server operation to settle and its descriptor to close, then pings and retries on the same connection. The retry preserves the ordinary unsupported-format diagnostic; the provider factory is never called.

  • Executed compiled CLI proof used a local timing hook forwarding real descriptor stat and delivering SIGINT at the boundary. inspect and analyze changed from UNKNOWN to {code:"cancelled", category:"cancelled", retryable:true, details:{operation:"resolve_binary_target", cleanup:"complete"}}. Late-stat cancellation and noncancelled unsupported-format controls remain unchanged.

  • Remaining proof gaps: naturally occurring race frequency is not measured; SDK testing uses the project's in-memory transport, while CLI SIGINT is executed in real child processes. No native engine or other host-platform execution is claimed. Cancellation remediation contains no new tool/parameter instructions.

  • 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 test:focused -- tests/process-global/binaryTargetAdmissionCancellation.test.ts tests/boundary/filesystem/binaryTarget.test.ts tests/boundary/filesystem/rawBinaryTarget.test.ts tests/boundary/filesystem/targetDiagnostics.test.ts src/artifacts/RegularFileAdmission.test.ts — 110 tests passed across five files.
  • npm run check:fast, npm run check, npm run check:ci — passed; includes duplicate-code, TODO and architecture gates, without changing exclusions or thresholds.
  • npm run verify:test-discovery — all 1,253 test files discovered exactly once.
  • npm run docs:check — passed, including five README consistency tests.
  • Compiled CLI SIGINT proof — five invocations executed: early/late cancellation for inspect/analyze plus noncancelled unsupported input.
  • Independent read-only review — no findings; not a substitute for CI.
  • Upstream CI — completed successfully on head 136a9ed9df5b21a9110e412462fb5e8d20549935: 13 successful jobs and 17 conditional skips. At final verification the PR's associated checks are 16 successful / 17 conditional skips; required CI passed. Its fast tier does not execute coverage.
  • Unchanged full workflow in the author's fork — completed successfully on the same head, attempt 2: 36 successful jobs / 1 conditional skip (the source-only job is not selected in full tier). Six coverage shards and the portable coverage/timing merge passed: 14,150 tests passed, 125 conditional skips, 90.57% line coverage. The existing reverse-tools coverage and mutation tasks also passed; this does not claim mutation scoring of the new admission guard. The Apple lane's existing target-free sessionMcp.test.ts case initially hit its 10-second timeout. Its six tests passed unchanged locally (the timed-out case took 1.87 seconds), and the original Apple job and dependent required summary passed after one unchanged rerun. The initial failure is retained; no code, timeout, assertion, exclusion or threshold was altered to obtain this result. Fork checks are supplemental evidence, not upstream required checks.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. This restores the existing typed cancellation contract.
  • Real Hopper/Ghidra, browser, or OS coverage: macOS file admission and compiled CLI executed; no provider changes or claims of real-engine validation.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new target execution or network access; tests preserve source bytes and explicitly assert no provider creation. Existing shared harness, temporary-directory registration and owned cleanup are reused.
  • AI assistance: implementation, test design, investigation and review were assisted by OpenAI Codex; the commands and results above were executed, rather than inferred from generated code.

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/3265 **Author:** [@SII-YDD](https://github.com/SII-YDD) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/target-admission-cancellation` --- ### 📝 Commits (1) - [`136a9ed`](https://github.com/morluto/rea/commit/136a9ed9df5b21a9110e412462fb5e8d20549935) fix(target): classify cancellation during file admission ### 📊 Changes **2 files changed** (+260 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/application/BinaryTargetResolver.ts` (+4 -0) ➕ `tests/process-global/binaryTargetAdmissionCancellation.test.ts` (+256 -0) </details> ### 📄 Description ## Summary Fixes #3197. Cancelling file admission during its first descriptor stat now returns the existing `cancelled` diagnostic instead of `UNKNOWN` or a raw rejected cancellation value. ## Problem and expected behavior Run `rea inspect sample.bin --format json` or `rea analyze sample.bin --format json` and send SIGINT after the first real `FileHandle.stat()` completes, before admission continues. Previously both commands reported `UNKNOWN`; cancellation after the resolver's second stat already reported `cancelled`. SDK cancellation of `open_binary` could likewise leave the server-side operation rejected with its raw signal reason. The resolver's declared result includes `AnalysisCancelledError`, so both cancellation checkpoints should settle with that error after successful descriptor cleanup. The issue reporter describes P2 impact; this PR does not claim an upstream P1 classification. ## Change and scope `parseBinaryTarget` now recognizes a caught value that is exactly the aborted signal's `reason` and wraps it as `AnalysisCancelledError("resolve_binary_target")`. `openRegularFile` remains a generic filesystem primitive using Node's cancellation semantics; the application resolver owns this domain normalization. The identity check preserves unrelated I/O failures occurring alongside cancellation and does not hide the `RegularFileAdmissionFailure` wrapper or its retained cleanup owner. Existing tagged cancellation errors keep their metadata. No new provider, execution capability, or configuration is introduced; additional maintainer scope authorization is not applicable to this correction within the existing admission boundary. ## Contract and boundary impact - Semantic owner and earliest changed stage: application target resolution, catching an abort from regular-file admission. - Contribution scope fit and linked maintainer decision: ordinary correctness fix within an existing boundary; additional scope decision not applicable. - CLI and MCP/tool-catalog contract: existing cancelled error shape and operation; tool names, parameters and advertised schemas unchanged. - Provider, bridge, target-format, or platform compatibility: provider dispatch and format selection unchanged; cancellation happens before provider creation. - Evidence, artifact, provenance, or reconstruction contract: source bytes unchanged; no new evidence or reconstruction representation. - Target selection, authority, upstream behavior, and cleanup details: caller selections unchanged; failed admission closes its real descriptor, and a failed close retains its existing error and owner for retry. - Generated metadata, package, or installation impact: none; generated documentation checks passed without authored metadata changes. - [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 - Added ten regressions using real open/stat/close operations, controlling only cancellation timing and injected failure cases. Default, Error and string reasons cover both stat checkpoints. Controls cover unsupported input, concurrent unrelated EIO, and failed close with retained ownership and successful close retry. - On unmodified base `7375ab6e34e511095a74d5a67e57cec28d56fd10`, four regressions failed and six controls passed. The early cancellations threw raw reasons, including the actual SDK server signal's string reason. The corrected tests all pass, including on synchronized base `29d3dcb7ea85b3be37cd81e977ae4e09cebdd2fc`. - Real SDK `open_binary` uses the existing application MCP harness and real session implementation. The test validates advertised input arguments, sends SDK cancellation, waits for the server operation to settle and its descriptor to close, then pings and retries on the same connection. The retry preserves the ordinary unsupported-format diagnostic; the provider factory is never called. - Executed compiled CLI proof used a local timing hook forwarding real descriptor stat and delivering SIGINT at the boundary. `inspect` and `analyze` changed from `UNKNOWN` to `{code:"cancelled", category:"cancelled", retryable:true, details:{operation:"resolve_binary_target", cleanup:"complete"}}`. Late-stat cancellation and noncancelled unsupported-format controls remain unchanged. - Remaining proof gaps: naturally occurring race frequency is not measured; SDK testing uses the project's in-memory transport, while CLI SIGINT is executed in real child processes. No native engine or other host-platform execution is claimed. Cancellation remediation contains no new tool/parameter instructions. - [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 test:focused -- tests/process-global/binaryTargetAdmissionCancellation.test.ts tests/boundary/filesystem/binaryTarget.test.ts tests/boundary/filesystem/rawBinaryTarget.test.ts tests/boundary/filesystem/targetDiagnostics.test.ts src/artifacts/RegularFileAdmission.test.ts` — 110 tests passed across five files. - `npm run check:fast`, `npm run check`, `npm run check:ci` — passed; includes duplicate-code, TODO and architecture gates, without changing exclusions or thresholds. - `npm run verify:test-discovery` — all 1,253 test files discovered exactly once. - `npm run docs:check` — passed, including five README consistency tests. - Compiled CLI SIGINT proof — five invocations executed: early/late cancellation for inspect/analyze plus noncancelled unsupported input. - Independent read-only review — no findings; not a substitute for CI. - [Upstream CI](https://github.com/morluto/rea/actions/runs/38525920062) — completed successfully on head `136a9ed9df5b21a9110e412462fb5e8d20549935`: 13 successful jobs and 17 conditional skips. At final verification the PR's associated checks are 16 successful / 17 conditional skips; required CI passed. Its fast tier does not execute coverage. - [Unchanged full workflow in the author's fork](https://github.com/SII-YDD/rea/actions/runs/38526903032) — completed successfully on the same head, attempt 2: 36 successful jobs / 1 conditional skip (the source-only job is not selected in full tier). Six coverage shards and the portable coverage/timing merge passed: 14,150 tests passed, 125 conditional skips, 90.57% line coverage. The existing reverse-tools coverage and mutation tasks also passed; this does not claim mutation scoring of the new admission guard. The Apple lane's existing target-free `sessionMcp.test.ts` case initially hit its 10-second timeout. Its six tests passed unchanged locally (the timed-out case took 1.87 seconds), and the original Apple job and dependent required summary passed after one unchanged rerun. The initial failure is retained; no code, timeout, assertion, exclusion or threshold was altered to obtain this result. Fork checks are supplemental evidence, not upstream required checks. ## Compatibility, safety, and release - Breaking changes or migration steps: none. This restores the existing typed cancellation contract. - Real Hopper/Ghidra, browser, or OS coverage: macOS file admission and compiled CLI executed; no provider changes or claims of real-engine validation. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new target execution or network access; tests preserve source bytes and explicitly assert no provider creation. Existing shared harness, temporary-directory registration and owned cleanup are reused. - AI assistance: implementation, test design, investigation and review were assisted by OpenAI Codex; the commands and results above were executed, rather than inferred from generated code. ## 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:08:37 +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#5685
No description provided.