[PR #3407] [MERGED] refactor(pcap): give TShark selection and version admission one owner #5725

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3407
Author: @555aaditya
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: refactor/tshark-single-owner


📝 Commits (10+)

  • bbecf6f refactor(pcap): give TShark selection and version admission one owner
  • f4eeba3 fix(pcap): execute the TShark the shared locator selects
  • e74e4d8 Merge remote-tracking branch 'origin/main' into refactor/tshark-single-owner
  • a22adc7 Merge origin/main into refactor/tshark-single-owner
  • 658c29b chore: restore main's copies of files the merge hook reformatted
  • cb6dc4f fix(pcap): share selected tool identity verification
  • 211d4b2 perf(pcap): retain compact executable identity
  • 4924706 fix(pcap): report missing selected tools clearly
  • 5e046bb fix(pcap): read historical capture evidence
  • f2cffbd fix(pcap): preserve executable read failures

📊 Changes

21 files changed (+1038 additions, -129 deletions)

View changed files

📝 docs/packet-captures.md (+9 -2)
📝 docs/pcap-analysis.md (+10 -1)
📝 src/application/CapabilityInventory.ts (+1 -1)
📝 src/application/captures/PacketCaptureService.test.ts (+1 -0)
📝 src/captures/TsharkCaptureProvider.ts (+27 -18)
📝 src/contracts/captures/captureToolContracts.ts (+2 -2)
📝 src/domain/analysisView/pcapCaptureView.test.ts (+17 -0)
📝 src/domain/analysisView/pcapCaptureView.ts (+2 -2)
📝 src/domain/captures/packetCapture.ts (+5 -0)
📝 src/domain/pcap/pcapAnalysis.ts (+27 -0)
📝 src/pcap/PcapConfiguration.test.ts (+131 -2)
📝 src/pcap/PcapConfiguration.ts (+128 -54)
📝 src/pcap/PcapProvider.ts (+200 -39)
📝 src/server/createServer.ts (+20 -1)
📝 tests/boundary/captures/captureProvider.test.ts (+67 -2)
📝 tests/boundary/cli/pcapCli.test.ts (+1 -1)
➕ tests/boundary/mcp/packetCaptureAvailability.test.ts (+104 -0)
📝 tests/boundary/mcp/pcapMcp.test.ts (+6 -0)
📝 tests/boundary/pcap/pcapProvider.test.ts (+40 -1)
📝 tests/boundary/pcap/pcapProviderCancellation.test.ts (+134 -2)

...and 1 more files

📄 Description

Summary

Refs #3403, milestone 1. Share caller-selected TShark/Capinfos selection, version parsing and executable identity facts between pcap and packet-capture workflows, including server availability.

An absolute environment path is honored; an invalid relative value is refused. Without a configured value, selection uses an executable in an absolute PATH directory where the platform locator supports it. Both providers reuse the shared size/SHA owner and verify the selected binary before and after a run. Retained identity contains byte count/digest, not whole executable buffers.

Current inspect_pcap_client output includes resolved path, digest and verified/unresolved status for each tool. The advertised output schema requires these facts; historical retained Evidence is read through a separate compatibility schema that supplies null/unresolved for absent old fields. It never invents verified identity.

Executable-read cancellation and failed descriptor cleanup pass through ArtifactAnalysisResources, preserving the primary cancellation, cleanup observations and retry owner. Missing configured tools remain typed capability failures instead of unreadable output.

Validation

On pinned Node 24.18.0/npm 11.16.0, current focused schema/view/MCP/cross-platform tests passed 17 tests with 2 platform skips; npm run check:fast passed. Earlier focused configuration/provider sets passed 84 tests, with 25 host-gated skips.

The real Linux TShark/Capinfos lane 39682764813 passed on ancestor 4924706e, including focused Linux tests and verify-packet-capture.mjs. The later changes fix retained-Evidence compatibility, current fixtures and read-error mapping. The fresh exact-head real-provider run 39685186476 also passed on f2cffbdf, including Linux focused tests and verify-packet-capture.mjs. This is current-head Linux provider verification; Windows remains outside that proof.

Remaining coverage

Windows PATH/PATHEXT identity remains explicitly unresolved where the locator cannot establish an absolute binary. Packet-capture host support stays Linux-only. Shared Windows native bounds and CPU-ceiling alignment remain later #3403 work. This pass adds no target execution or mutation authority; provider commands inspect caller-selected capture files. Host TShark/Capinfos are unavailable locally.

Compatibility

Breaking change for inspect_pcap_* on Linux and macOS: with REA_TSHARK_COMMAND or REA_CAPINFOS_COMMAND unset, the tool must now be in an absolute PATH entry. Previously the bare name went to the process launcher, which also searched relative PATH entries. A setup whose TShark/Capinfos is reachable only through a relative PATH entry now fails with tshark_missing / capinfos_missing. Migration: put the tool in an absolute PATH directory or set the absolute REA_TSHARK_COMMAND / REA_CAPINFOS_COMMAND. Absolute environment paths, absolute PATH installs and Windows PATH/PATHEXT selection behave as before. inspect_packet_* previously accepted only an absolute REA_TSHARK_COMMAND, so it gains PATH installs and loses nothing.

Review follow-ups (merge of main at b8c1b4531)

  • Resolved the import conflicts with main's parseAnalysisViewResult (pcapCaptureView.ts) and privateWorkspaceFailure (TsharkCaptureProvider.ts), keeping pcapEvidenceSchemas.
  • A missing selected tool now names its configured path again ("The selected tshark executable /nonexistent/tshark could not be found at its configured path."). This restores the public CLI refusal that tests/boundary/cli/pcapCli.test.ts checks.
  • New tests:
    • A 3.x TShark banner and a non-Wireshark banner through the packet provider. Each pins the capability-unavailable reason and that only the version probe ran.
    • The inspect_packet_capture session availability rule over MCP: PATH-only, absolute and relative REA_TSHARK_COMMAND, plus the no-absolute-PATH-entry case. The first three are Linux-gated.
  • main's new late-cancellation service fixture now has tool.selection_source.
  • On macOS arm64 with Node 26.5.0: npm run test:fast passed (13,966 passed, 23 skipped). The pcap/captures/MCP set passed (154 passed, 39 host-gated skipped). The pcap CLI boundary test passed with a fresh build, and npm run check:fast passed. I didn't run the Linux-gated availability cases or real TShark locally.

Author readiness

Human owner: @555aaditya

  • 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

The shared selection/version boundary now also owns executable identity projection and same-run size/SHA checks for both packet-capture and pcap consumers. It retains compact byte-count/digest facts rather than whole executable buffers between calls. Absolute configured paths remain caller-selected; invalid relative environment paths fail rather than silently selecting PATH. Windows PATH/PATHEXT resolution remains explicitly unresolved where the locator cannot establish an absolute binary identity.

On Node 24.18.0/npm 11.16.0, the latest focused configuration/pcap/capture set passed 84 tests, with 25 platform-gated skips; npm run check:fast passed. TShark/Capinfos are unavailable locally. An exact-head hosted real-provider workflow has been requested separately; queued execution is not provider proof. Existing file-only analysis authority suffices for this correction; no new target execution/injection capability is added.


🔄 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/3407 **Author:** [@555aaditya](https://github.com/555aaditya) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `refactor/tshark-single-owner` --- ### 📝 Commits (10+) - [`bbecf6f`](https://github.com/morluto/rea/commit/bbecf6fb0551509e0b1e4790e7114a7399622b34) refactor(pcap): give TShark selection and version admission one owner - [`f4eeba3`](https://github.com/morluto/rea/commit/f4eeba365486b20ead0772bff44c0a0a3bf76486) fix(pcap): execute the TShark the shared locator selects - [`e74e4d8`](https://github.com/morluto/rea/commit/e74e4d8f0b8c56a712dd86bb7d91019deb55d804) Merge remote-tracking branch 'origin/main' into refactor/tshark-single-owner - [`a22adc7`](https://github.com/morluto/rea/commit/a22adc78092f3668fd5c3329aca1975eb5651713) Merge origin/main into refactor/tshark-single-owner - [`658c29b`](https://github.com/morluto/rea/commit/658c29b7fc8770252de3970e1713aea0457a1912) chore: restore main's copies of files the merge hook reformatted - [`cb6dc4f`](https://github.com/morluto/rea/commit/cb6dc4f3eff27ed2576c85a2f631b3811dc38dbb) fix(pcap): share selected tool identity verification - [`211d4b2`](https://github.com/morluto/rea/commit/211d4b221997cd0b5bbb3cf481b7c66f85d5c84c) perf(pcap): retain compact executable identity - [`4924706`](https://github.com/morluto/rea/commit/4924706e25106f3a0e463d3268cdee7906872cf9) fix(pcap): report missing selected tools clearly - [`5e046bb`](https://github.com/morluto/rea/commit/5e046bb374dca1d6342aba66bf910bde0c806e66) fix(pcap): read historical capture evidence - [`f2cffbd`](https://github.com/morluto/rea/commit/f2cffbdfdcac24755c38e71b923b8d7a43fb3c98) fix(pcap): preserve executable read failures ### 📊 Changes **21 files changed** (+1038 additions, -129 deletions) <details> <summary>View changed files</summary> 📝 `docs/packet-captures.md` (+9 -2) 📝 `docs/pcap-analysis.md` (+10 -1) 📝 `src/application/CapabilityInventory.ts` (+1 -1) 📝 `src/application/captures/PacketCaptureService.test.ts` (+1 -0) 📝 `src/captures/TsharkCaptureProvider.ts` (+27 -18) 📝 `src/contracts/captures/captureToolContracts.ts` (+2 -2) 📝 `src/domain/analysisView/pcapCaptureView.test.ts` (+17 -0) 📝 `src/domain/analysisView/pcapCaptureView.ts` (+2 -2) 📝 `src/domain/captures/packetCapture.ts` (+5 -0) 📝 `src/domain/pcap/pcapAnalysis.ts` (+27 -0) 📝 `src/pcap/PcapConfiguration.test.ts` (+131 -2) 📝 `src/pcap/PcapConfiguration.ts` (+128 -54) 📝 `src/pcap/PcapProvider.ts` (+200 -39) 📝 `src/server/createServer.ts` (+20 -1) 📝 `tests/boundary/captures/captureProvider.test.ts` (+67 -2) 📝 `tests/boundary/cli/pcapCli.test.ts` (+1 -1) ➕ `tests/boundary/mcp/packetCaptureAvailability.test.ts` (+104 -0) 📝 `tests/boundary/mcp/pcapMcp.test.ts` (+6 -0) 📝 `tests/boundary/pcap/pcapProvider.test.ts` (+40 -1) 📝 `tests/boundary/pcap/pcapProviderCancellation.test.ts` (+134 -2) _...and 1 more files_ </details> ### 📄 Description ## Summary Refs #3403, milestone 1. Share caller-selected TShark/Capinfos selection, version parsing and executable identity facts between pcap and packet-capture workflows, including server availability. An absolute environment path is honored; an invalid relative value is refused. Without a configured value, selection uses an executable in an absolute PATH directory where the platform locator supports it. Both providers reuse the shared size/SHA owner and verify the selected binary before and after a run. Retained identity contains byte count/digest, not whole executable buffers. Current `inspect_pcap_client` output includes resolved path, digest and verified/unresolved status for each tool. The advertised output schema requires these facts; historical retained Evidence is read through a separate compatibility schema that supplies null/unresolved for absent old fields. It never invents verified identity. Executable-read cancellation and failed descriptor cleanup pass through `ArtifactAnalysisResources`, preserving the primary cancellation, cleanup observations and retry owner. Missing configured tools remain typed capability failures instead of unreadable output. ## Validation On pinned Node 24.18.0/npm 11.16.0, current focused schema/view/MCP/cross-platform tests passed 17 tests with 2 platform skips; `npm run check:fast` passed. Earlier focused configuration/provider sets passed 84 tests, with 25 host-gated skips. The real Linux TShark/Capinfos lane [39682764813](https://github.com/morluto/rea/actions/runs/39682764813) passed on ancestor `4924706e`, including focused Linux tests and `verify-packet-capture.mjs`. The later changes fix retained-Evidence compatibility, current fixtures and read-error mapping. The fresh exact-head [real-provider run 39685186476](https://github.com/morluto/rea/actions/runs/39685186476) also passed on `f2cffbdf`, including Linux focused tests and `verify-packet-capture.mjs`. This is current-head Linux provider verification; Windows remains outside that proof. ## Remaining coverage Windows PATH/PATHEXT identity remains explicitly unresolved where the locator cannot establish an absolute binary. Packet-capture host support stays Linux-only. Shared Windows native bounds and CPU-ceiling alignment remain later #3403 work. This pass adds no target execution or mutation authority; provider commands inspect caller-selected capture files. Host TShark/Capinfos are unavailable locally. ## Compatibility Breaking change for `inspect_pcap_*` on Linux and macOS: with `REA_TSHARK_COMMAND` or `REA_CAPINFOS_COMMAND` unset, the tool must now be in an **absolute** PATH entry. Previously the bare name went to the process launcher, which also searched relative PATH entries. A setup whose TShark/Capinfos is reachable only through a relative PATH entry now fails with `tshark_missing` / `capinfos_missing`. Migration: put the tool in an absolute PATH directory or set the absolute `REA_TSHARK_COMMAND` / `REA_CAPINFOS_COMMAND`. Absolute environment paths, absolute PATH installs and Windows PATH/PATHEXT selection behave as before. `inspect_packet_*` previously accepted only an absolute `REA_TSHARK_COMMAND`, so it gains PATH installs and loses nothing. ## Review follow-ups (merge of `main` at b8c1b4531) - Resolved the import conflicts with `main`'s `parseAnalysisViewResult` (`pcapCaptureView.ts`) and `privateWorkspaceFailure` (`TsharkCaptureProvider.ts`), keeping `pcapEvidenceSchemas`. - A missing selected tool now names its configured path again ("The selected tshark executable /nonexistent/tshark could not be found at its configured path."). This restores the public CLI refusal that `tests/boundary/cli/pcapCli.test.ts` checks. - New tests: - A 3.x TShark banner and a non-Wireshark banner through the packet provider. Each pins the capability-unavailable reason and that only the version probe ran. - The `inspect_packet_capture` session availability rule over MCP: PATH-only, absolute and relative `REA_TSHARK_COMMAND`, plus the no-absolute-PATH-entry case. The first three are Linux-gated. - `main`'s new late-cancellation service fixture now has `tool.selection_source`. - On macOS arm64 with Node 26.5.0: `npm run test:fast` passed (13,966 passed, 23 skipped). The pcap/captures/MCP set passed (154 passed, 39 host-gated skipped). The pcap CLI boundary test passed with a fresh build, and `npm run check:fast` passed. I didn't run the Linux-gated availability cases or real TShark locally. ## Author readiness <!-- The human owner completes these declarations before requesting review. The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. AI agents must leave these declarations to a human to dictate to them or there could be catastrophic consequences for many users of REA. Draft PRs may leave them unchecked. These declarations record initial readiness when opening the PR for review; later pushes do not require a new human sign-off. Keep the description and validation evidence current as the work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: @555aaditya - [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 <!-- Draft PRs may leave pending items unchecked. Resolve material scope and authority questions before integration. --> - [ ] 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-review --> ### Maintainer blocker verification The shared selection/version boundary now also owns executable identity projection and same-run size/SHA checks for both packet-capture and pcap consumers. It retains compact byte-count/digest facts rather than whole executable buffers between calls. Absolute configured paths remain caller-selected; invalid relative environment paths fail rather than silently selecting PATH. Windows PATH/PATHEXT resolution remains explicitly unresolved where the locator cannot establish an absolute binary identity. On Node 24.18.0/npm 11.16.0, the latest focused configuration/pcap/capture set passed 84 tests, with 25 platform-gated skips; `npm run check:fast` passed. TShark/Capinfos are unavailable locally. An exact-head hosted real-provider workflow has been requested separately; queued execution is not provider proof. Existing file-only analysis authority suffices for this correction; no new target execution/injection capability is added. --- <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:57 +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#5725
No description provided.