morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #5279] [Bug] Pcap command projection replaces producer failures with late cancellation and wraps abort reasons #3221

New issue

Closed

opened 2026-10-31 00:38:00 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

2026-10-31 00:38:00 +00:00

Owner

Originally created by @morluto on GitHub (Oct 19, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/5279

Before submitting

Searched open and closed PcapCommand/pcap cancellation reports and open PRs; related closed #4858 covered shared artifact admission and other producer consumers, not this PcapCommand projector. #4227 is the broader lifecycle audit.
No credentials or sensitive target data included.

Area / interface / failure stage

Process capture or runtime observation; Other (production command boundary); cleanup, cancellation, or lifecycle.

Observed behavior

Executed runPcapCommand with real, source-owned Node children and deterministic cancellation barriers:

A child writes authored producer failure to stderr and exits 7. Its launcher cleanup performs the normal owned cleanup and then aborts the caller signal. OwnedCommandFailure retains reason: "process", exit 7 and the diagnostics, but Pcap projects AnalysisCancelledError instead of the physical failure.
A child writes ready and remains active. Abort on that stdout event with null, a string, or a caller-created ProviderCleanupError. Pcap returns AnalysisCancelledError, but its cause is the OwnedCommandFailure wrapper rather than the owner's exact cancellation reason.

All four new regression cases failed on the baseline; the two existing command controls passed. No TShark, capinfos, capture file, network, or hardware was used.

Reproduction

Source baseline 1b3ac94242882e0fbf9c717806646c729abd8b91, Node 24.18.0, Darwin 25.3.0 arm64. Added the authored cases to tests/boundary/pcap/pcapCommand.test.ts and ran:

npm run test:focused -- tests/boundary/pcap/pcapCommand.test.ts

The physical-failure case supplies command: process.execPath, arguments ['-e', 'process.stderr.write("authored producer failure"); process.exitCode = 7'], a 5000 ms deadline, a 1024-byte output budget, and an authored inputPath used only for classification. The injected launcher calls spawnOwnedProviderProcess, returns its ordinary process/ownership, and wraps only final cleanup:

const cleanup = launched.cleanup ?? (() => cleanupOwnedProcessGroup(launched.ownership));
return { ...launched, cleanup: async () => {
  const outcome = await cleanup();
  controller.abort(reason);
  return outcome;
}};

The active-cancel child uses process.stdout.write("ready"); setInterval(() => {}, 1000); the launcher installs a one-shot stdout listener that calls controller.abort(reason) before returning the owned process. The assertions check typed outcome, exact reason identity, retained output, and that the foreign cleanup error does not become owned cleanup. Each new case was executed once in the baseline run.

Expected behavior

Consume the command owner's parsed outcome and exact cancellation fact. A late signal must not replace a known physical failure; an actual cancellation must preserve OwnedCommandFailure.cancellation.reason without adopting resources from an opaque caller reason. Preserve real cleanup failures separately. This follows the lifecycle invariants.

Impact and scope

Observed wrong error classification and loss of exact cancellation cause at the production command boundary. Public pcap version/capture workflows use this projector. NOT_ENOUGH_INFO: actual TShark/capinfos cancellation frequency, compiled CLI/MCP delivery, native Linux/Windows behavior, and real capture impact. This report does not claim process escape or lost packet Evidence.

Relevant output or evidence

Baseline focused run: 4 failed | 2 passed. The exit-7 case received AnalysisCancelledError; the three active-cancel cases received the OwnedCommandFailure wrapper as .cause. The owner's reason, cancellation, exit status and output remain independently present before projection.

Source-derived: PcapCommand.ts rereads signal.aborted and passes the wrapper as details.cause; OwnedCommand.ts already separates physical failure from cancellation observed during finalization. Nearby Pcap availability and snapshot projectors also resample the signal; those paths are under separate verification and are not claimed reproduced here.

Typed outcome or recovery expected

Keep the physical AnalysisProtocolError with exit/output facts in case 1; keep AnalysisCancelledError with the exact opaque reason in case 2. Bind any unresolved cleanup to its actual command/snapshot owner and preserve retry.

Additional context

REA source package version 6.7.0. No real provider was launched. Last known good / first affected release: NOT_ENOUGH_INFO. The worktree also contains an unrelated Rizin close fix; the Pcap/process-owner source used for this reproduction matches the baseline SHA.

Follow-up verification and local fix

The same cancellation/ownership pattern was independently checked through Pcap output parsing, snapshot finalization and provider consumers. The local patch now consumes typed cancellation facts, preserves physical primary failures, retains complete-output partial Evidence on failed cleanup, blocks replacement admission while cleanup remains, and binds originating and retry errors to exact command/snapshot owners. Snapshot tests cover abort during successful final close and during a rejected final close, retaining the exact caller reason and physical close diagnostic until descriptor retry resolves its marker.

Additional baseline controls: output parsing had 3 failing exact-reason cases; an isolated baseline provider with the corrected lower-level command/parser/snapshot boundaries still failed all 3 initial provider regressions; the final 8-case snapshot fixture failed 5 cases against isolated baseline source. These are controlled production-boundary results, not real Wireshark reproductions.

Final local validation on Node 24.18.0 / Darwin arm64: 113 passed, 25 skipped in 11 selected files, including the public CLI/MCP Pcap controls and shared debugger owner regressions. Typecheck/lint, changed-file formatting, git diff --check, and test discovery (1,670 files exactly once) passed. The skipped Linux/native/provider paths do not establish real TShark/capinfos or native Linux/Windows behavior. The CLI/MCP controls verify their existing fixture/catalog workflows; they do not prove cancellation delivery through every client.

The fix is local and uncommitted; no integrated fix or issue closure is claimed.

Upstream integration recheck

Re-ran the unchanged local Pcap patch on e84e17e30f7de41ad9ac801eda235574fa5d4dad, including merged shared process-ownership #5249 and Rizin #5281: 117 passed, 46 skipped in 13 selected files. This selection adds the Rizin TTY and inferior-exit suites; Linux-only exit cases are skipped on this Darwin host. check:fast and discovery (1,674 files exactly once) also pass. These are locally executed checks; #5281's real Linux/Rizin verification remains attributed to its author. The overlapping local Rizin patch was archived and replaced by the merged upstream implementation. The Pcap fix remains local and uncommitted.

Originally created by @morluto on GitHub (Oct 19, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5279 ### Before submitting - [x] Searched open and closed PcapCommand/pcap cancellation reports and open PRs; related closed #4858 covered shared artifact admission and other producer consumers, not this PcapCommand projector. #4227 is the broader lifecycle audit. - [x] No credentials or sensitive target data included. ### Area / interface / failure stage Process capture or runtime observation; Other (production command boundary); cleanup, cancellation, or lifecycle. ### Observed behavior Executed `runPcapCommand` with real, source-owned Node children and deterministic cancellation barriers: 1. A child writes `authored producer failure` to stderr and exits 7. Its launcher cleanup performs the normal owned cleanup and then aborts the caller signal. `OwnedCommandFailure` retains `reason: "process"`, exit 7 and the diagnostics, but Pcap projects `AnalysisCancelledError` instead of the physical failure. 2. A child writes `ready` and remains active. Abort on that stdout event with null, a string, or a caller-created `ProviderCleanupError`. Pcap returns `AnalysisCancelledError`, but its cause is the `OwnedCommandFailure` wrapper rather than the owner's exact cancellation reason. All four new regression cases failed on the baseline; the two existing command controls passed. No TShark, capinfos, capture file, network, or hardware was used. ### Reproduction Source baseline `1b3ac94242882e0fbf9c717806646c729abd8b91`, Node 24.18.0, Darwin 25.3.0 arm64. Added the authored cases to `tests/boundary/pcap/pcapCommand.test.ts` and ran: ```sh npm run test:focused -- tests/boundary/pcap/pcapCommand.test.ts ``` The physical-failure case supplies `command: process.execPath`, arguments `['-e', 'process.stderr.write("authored producer failure"); process.exitCode = 7']`, a 5000 ms deadline, a 1024-byte output budget, and an authored `inputPath` used only for classification. The injected launcher calls `spawnOwnedProviderProcess`, returns its ordinary process/ownership, and wraps only final cleanup: ```ts const cleanup = launched.cleanup ?? (() => cleanupOwnedProcessGroup(launched.ownership)); return { ...launched, cleanup: async () => { const outcome = await cleanup(); controller.abort(reason); return outcome; }}; ``` The active-cancel child uses `process.stdout.write("ready"); setInterval(() => {}, 1000)`; the launcher installs a one-shot stdout listener that calls `controller.abort(reason)` before returning the owned process. The assertions check typed outcome, exact reason identity, retained output, and that the foreign cleanup error does not become owned cleanup. Each new case was executed once in the baseline run. ### Expected behavior Consume the command owner's parsed outcome and exact cancellation fact. A late signal must not replace a known physical failure; an actual cancellation must preserve `OwnedCommandFailure.cancellation.reason` without adopting resources from an opaque caller reason. Preserve real cleanup failures separately. This follows [the lifecycle invariants](https://github.com/morluto/rea/blob/1b3ac94242882e0fbf9c717806646c729abd8b91/docs/tool-design.md#own-the-lifecycle). ### Impact and scope Observed wrong error classification and loss of exact cancellation cause at the production command boundary. Public pcap version/capture workflows use this projector. NOT_ENOUGH_INFO: actual TShark/capinfos cancellation frequency, compiled CLI/MCP delivery, native Linux/Windows behavior, and real capture impact. This report does not claim process escape or lost packet Evidence. ### Relevant output or evidence Baseline focused run: `4 failed | 2 passed`. The exit-7 case received `AnalysisCancelledError`; the three active-cancel cases received the `OwnedCommandFailure` wrapper as `.cause`. The owner's `reason`, `cancellation`, exit status and output remain independently present before projection. Source-derived: [`PcapCommand.ts`](https://github.com/morluto/rea/blob/1b3ac94242882e0fbf9c717806646c729abd8b91/src/pcap/PcapCommand.ts#L187-L191) rereads `signal.aborted` and passes the wrapper as `details.cause`; [`OwnedCommand.ts`](https://github.com/morluto/rea/blob/1b3ac94242882e0fbf9c717806646c729abd8b91/src/process/OwnedCommand.ts#L337-L386) already separates physical failure from cancellation observed during finalization. Nearby Pcap availability and snapshot projectors also resample the signal; those paths are under separate verification and are not claimed reproduced here. ### Typed outcome or recovery expected Keep the physical `AnalysisProtocolError` with exit/output facts in case 1; keep `AnalysisCancelledError` with the exact opaque reason in case 2. Bind any unresolved cleanup to its actual command/snapshot owner and preserve retry. ### Additional context REA source package version 6.7.0. No real provider was launched. Last known good / first affected release: NOT_ENOUGH_INFO. The worktree also contains an unrelated Rizin close fix; the Pcap/process-owner source used for this reproduction matches the baseline SHA. ### Follow-up verification and local fix The same cancellation/ownership pattern was independently checked through Pcap output parsing, snapshot finalization and provider consumers. The local patch now consumes typed cancellation facts, preserves physical primary failures, retains complete-output partial Evidence on failed cleanup, blocks replacement admission while cleanup remains, and binds originating and retry errors to exact command/snapshot owners. Snapshot tests cover abort during successful final close and during a rejected final close, retaining the exact caller reason and physical close diagnostic until descriptor retry resolves its marker. Additional baseline controls: output parsing had 3 failing exact-reason cases; an isolated baseline provider with the corrected lower-level command/parser/snapshot boundaries still failed all 3 initial provider regressions; the final 8-case snapshot fixture failed 5 cases against isolated baseline source. These are controlled production-boundary results, not real Wireshark reproductions. Final local validation on Node 24.18.0 / Darwin arm64: 113 passed, 25 skipped in 11 selected files, including the public CLI/MCP Pcap controls and shared debugger owner regressions. Typecheck/lint, changed-file formatting, `git diff --check`, and test discovery (1,670 files exactly once) passed. The skipped Linux/native/provider paths do not establish real TShark/capinfos or native Linux/Windows behavior. The CLI/MCP controls verify their existing fixture/catalog workflows; they do not prove cancellation delivery through every client. The fix is local and uncommitted; no integrated fix or issue closure is claimed. ### Upstream integration recheck Re-ran the unchanged local Pcap patch on `e84e17e30f7de41ad9ac801eda235574fa5d4dad`, including merged shared process-ownership #5249 and Rizin #5281: 117 passed, 46 skipped in 13 selected files. This selection adds the Rizin TTY and inferior-exit suites; Linux-only exit cases are skipped on this Darwin host. `check:fast` and discovery (1,674 files exactly once) also pass. These are locally executed checks; #5281's real Linux/Rizin verification remains attributed to its author. The overlapping local Rizin patch was archived and replaced by the merged upstream implementation. The Pcap fix remains local and uncommitted.

bsakaguchi

2026-10-31 00:38:00 +00:00

closed this issue
added the
triage

bug
labels

bsakaguchi commented

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

Author

Owner

@akushonkamen commented on GitHub (Oct 19, 2026):

I'm picking this up — will add the regression cases from the report to tests/boundary/pcap/pcapCommand.test.ts and fix the error projection in src/pcap/PcapCommand.ts.

@akushonkamen commented on GitHub (Oct 19, 2026): I'm picking this up — will add the regression cases from the report to `tests/boundary/pcap/pcapCommand.test.ts` and fix the error projection in `src/pcap/PcapCommand.ts`.

bsakaguchi commented

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

Author

Owner

@akushonkamen commented on GitHub (Oct 19, 2026):

Progress update: I reproduced the reported projection bugs locally (4 new regression cases in tests/boundary/pcap/pcapCommand.test.ts all failed against unmodified origin/main, matching the report's 4 failed | 2 passed baseline) and fixed the projector in src/pcap/PcapCommand.ts:

A known physical OwnedCommandFailure with reason: "process" is no longer replaced by a late caller abort observed during final cleanup — it now falls through to the physical projection (exit status, captured output and stderr diagnostics preserved, yielding AnalysisProtocolError for the authored exit-7 case).
Genuine cancellations now surface the owner's exact cancellation reason as the AnalysisCancelledError cause — OwnedCommandFailure.cancellation.reason when present, otherwise signal.reason — instead of the OwnedCommandFailure wrapper. Verified for null, string, and caller-created ProviderCleanupError reasons; the opaque caller reason is not adopted as owned cleanup.

Local verification (Darwin arm64, real Node children, no TShark):

Red: npx vitest run tests/boundary/pcap/pcapCommand.test.ts → 4 failed | 2 passed
Green (same command): 6 passed (6)
npx vitest run tests/boundary/pcap/ → 12 passed | 25 skipped
npx vitest run --project boundary → exit 0 (all passed)
tsc --noEmit: no diagnostics in the changed files (pre-existing unrelated openpgp module-resolution errors exist in this sandbox on untouched files)

Fix branch: https://github.com/akushonkamen/rea/commits/fix/5279-pcap-error-projection (HEAD d8d2b0dff, includes Fixes #5279). PR creation is deferred this round per author-readiness requirements.

@akushonkamen commented on GitHub (Oct 19, 2026): Progress update: I reproduced the reported projection bugs locally (4 new regression cases in `tests/boundary/pcap/pcapCommand.test.ts` all failed against unmodified `origin/main`, matching the report's `4 failed | 2 passed` baseline) and fixed the projector in `src/pcap/PcapCommand.ts`: - A known physical `OwnedCommandFailure` with `reason: "process"` is no longer replaced by a late caller abort observed during final cleanup — it now falls through to the physical projection (exit status, captured output and stderr diagnostics preserved, yielding `AnalysisProtocolError` for the authored exit-7 case). - Genuine cancellations now surface the owner's exact cancellation reason as the `AnalysisCancelledError` cause — `OwnedCommandFailure.cancellation.reason` when present, otherwise `signal.reason` — instead of the `OwnedCommandFailure` wrapper. Verified for null, string, and caller-created `ProviderCleanupError` reasons; the opaque caller reason is not adopted as owned cleanup. Local verification (Darwin arm64, real Node children, no TShark): - Red: `npx vitest run tests/boundary/pcap/pcapCommand.test.ts` → `4 failed | 2 passed` - Green (same command): `6 passed (6)` - `npx vitest run tests/boundary/pcap/` → `12 passed | 25 skipped` - `npx vitest run --project boundary` → exit 0 (all passed) - `tsc --noEmit`: no diagnostics in the changed files (pre-existing unrelated `openpgp` module-resolution errors exist in this sandbox on untouched files) Fix branch: https://github.com/akushonkamen/rea/commits/fix/5279-pcap-error-projection (HEAD d8d2b0dff, includes `Fixes #5279`). PR creation is deferred this round per author-readiness requirements.

bsakaguchi commented

2026-10-31 00:38:02 +00:00

Author

Owner

@morluto commented on GitHub (Oct 20, 2026):

Closing as completed: verified during REA Bot open-bug audit that the described pcap-command projection/cancellation fix is already on current main with regression coverage. No further PR needed for this report.

@morluto commented on GitHub (Oct 20, 2026): Closing as completed: verified during REA Bot open-bug audit that the described pcap-command projection/cancellation fix is already on current main with regression coverage. No further PR needed for this report.

bsakaguchi referenced this issue

2026-10-31 00:38:52 +00:00

[GH-ISSUE #5431] [Feature] Make MCP tool failures self-recovering for agents #3294

bsakaguchi referenced this issue

2026-10-31 00:49:09 +00:00

[GH-ISSUE #7327] [Bug] MCP provenance documentation lists two exceptions although both settings tools are also exempt #4354

bsakaguchi referenced this issue

2026-10-31 00:51:13 +00:00

[GH-ISSUE #7682] [Bug] Stdio frame validation errors advise restarting a still-connected MCP client #4605

bsakaguchi referenced this issue

2026-10-31 01:08:38 +00:00

[PR #3267] [CLOSED] fix(rea): bound oversized SDK validation errors #5686