Repository navigation
refactor(pcap): give TShark selection and version admission one owner - #3407
Merged
Merged
Conversation
The pcap and packet-capture families wrapped the same caller-supplied TShark with diverging rules. inspect_pcap_* accepted an absolute REA_TSHARK_COMMAND or tshark from PATH, while inspect_packet_* and the server availability policy required an absolute REA_TSHARK_COMMAND and parsed the version banner with a separate regex. PcapConfiguration now owns the rule: selectWiresharkTool selects an absolute environment path or the bare name on PATH and refuses relative values; locateWiresharkTool resolves a PATH selection to the first absolute PATH entry holding an executable file; parseWiresharkVersion admits Wireshark 4.x banners for both providers. TsharkCaptureProvider and the session availability policy consume these facts instead of reinterpreting REA_TSHARK_COMMAND, so all four tools accept and refuse the same configurations. inspect_packet_* still resolves, hashes and rechecks the located executable, and records selection_source in its tool Evidence. Host support and resource bounds are unchanged; unifying them is the next milestone in morluto#3403. Refs morluto#3403
555aaditya
marked this pull request as ready for review
October 14, 2026 10:47
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
 [Medium impact] The reviewed changes appear safe to merge; the remaining cleanup finding is fixed. SummaryThe PR shares TShark selection, version checks, and executable identity checks across pcap and packet-capture tools.
Reviews (12) · Last reviewed commit: "fix(pcap): keep a failed executable re-r..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
resolvePcapTools still returned bare tool names for PATH selections, so the operating system could run a tool from a relative PATH entry while the packet tools skipped that entry and selected a different install. On POSIX hosts, resolve PATH selections through locateWiresharkTool and run that absolute executable, failing with the existing tshark_missing or capinfos_missing code when no absolute PATH entry holds the tool. Both selections are validated before PATH is consulted, so a relative environment value still reports its own configuration error. Windows keeps PATH/PATHEXT resolution in the process launcher. Refs morluto#3403
This was referenced Oct 14, 2026
2 tasks done
Contributor
Author
This was referenced Oct 20, 2026
Resolve the node:fs/promises import conflict in pcapProviderCrossPlatform.test.ts by importing open, symlink and writeFile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
|
Merged latest What changed and why
Validation (local macOS, merged head)
|
The pre-commit oxlint --fix touched two files this PR does not change; keep them identical to main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
|
Follow-up: 658c29b restores |
CassieFox23
reviewed
Oct 21, 2026
CassieFox23
left a comment
CassieFox23
left a comment
Contributor
There was a problem hiding this comment.
TL;DR: The change gives TShark selection one owner, and the shape is sound. Three things need work before merge. The PR body says no earlier setup breaks, but one now fails. A 3.x banner has no test. The new availability rule has no test either.
Findings
Should fix
- The body says old setups all still work. The diff shows one that does not.
resolvePcapToolsnow skips relative PATH entries. It throwstshark_missingorcapinfos_missingwhen no absolute entry holds the tool. The new test withPATH: "relative-only"shows this. Before, the bare name went to the process launcher. From the diff alone, a setup with the tool only in a relative PATH entry now fails forinspect_pcap_*. This may be the right rule. Please list it under breaking changes in the body. See PcapConfiguration.ts. - With a 3.x banner,
parseWiresharkVersionthrows. The error isPcapConfigurationFailure. It returns null for a non-Wireshark banner or empty output. The old regex gave one plain Error for both cases. The catch at L401 sends it down the generic failure path, which reports the error's own message. So a 3.x banner now gets different wording. No test in the diff sends a 3.x banner through the packet provider. Please add one. See TsharkCaptureProvider.ts. - The new
tsharkLocatablerule changes what the session reports. It affects which tools show as available. The body says PATH-only installs now show as available. No test in the diff covers this rule. Please cover a PATH-only install, an absoluteREA_TSHARK_COMMANDand a relative one. See createServer.ts.
Nit
- The locate test has a local
const relative. It hides therelativefromnode:path. It is also an absolute path to a directory the test never creates. A new name would be clearer. See PcapConfiguration.test.ts. - docs/pcap-analysis.md: The edited paragraph now has one very long line. It joins the Windows sentence to "Client inspection reads no capture file." Please rewrap it.
Looks good
- One rule selects the tool. One rule locates it. They replace three copies. Both tools are checked before PATH is read. A relative environment value is refused, not looked up. See PcapConfiguration.ts.
- The cross-provider test compares tshark with the packet rule. They pick the same absolute file, even with a relative decoy ahead of it. See PcapConfiguration.test.ts.
- The resolve and hash flow is kept.
selection_sourceis added, with tests for both theenvironmentandpathcases. See TsharkCaptureProvider.ts. - The tool text and the docs move together. It covers the capability fix text. See captureToolContracts.ts.
Not run
- No build and no tests. This is a read-only review of the diff against main.
Thanks, 555aaditya, for pulling three separate reads of REA_TSHARK_COMMAND into one owner. You also added a test that checks the tools pick the same executable.
🤖 Written with Claude Code (Sonnet 5.5, verified by Opus 5.5), posted with my OK 🦊
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Resolve import conflicts with main's retained-result validation helper (b8c1b45) and workspace-failure mapping (morluto#6229): pcapCaptureView keeps the historical-Evidence compatibility schema (pcapEvidenceSchemas) and parses it through main's parseAnalysisViewResult; TsharkCaptureProvider keeps both the shared Wireshark identity imports and privateWorkspaceFailure.
Address review follow-ups on the shared TShark owner: - Send a 3.x TShark banner and a non-Wireshark banner through the packet provider, pinning the capability-unavailable reason for each and that only the version probe ran before refusal. - Cover the session availability rule for inspect_packet_capture over MCP: a PATH-only install and an absolute REA_TSHARK_COMMAND are available on Linux, a relative REA_TSHARK_COMMAND is refused even when PATH holds TShark, and no absolute PATH entry reports provider_missing. - Rename the locator test's missing absolute PATH entry so it no longer shadows node:path relative. Refs morluto#3403
Resolving the selected executable before launch replaced the runner's "Cannot run selected tool <path>" refusal with a message that kept the configured path only in diagnostics, so the public CLI no longer told the caller which REA_TSHARK_COMMAND or REA_CAPINFOS_COMMAND value to fix and the pcap CLI boundary test failed. Include the configured command in the capability-unavailable message and assert it there. Refs morluto#3403
… source The service fixture added on main in 719d56e predates the required tool.selection_source field, so its report failed output validation before reaching the late-cancellation path the test covers. Refs morluto#3403
Contributor
Author
|
Thanks for the careful review, @CassieFox23! I addressed it in Should fix
Nits
While merging, I also found that resolving the executable before launch had dropped the configured path from the missing-tool message, which broke |
When a tool probe returned with its owned process still awaiting cleanup, a failed post-run executable re-read or identity check threw before run() reported that cleanup. Probes have no snapshot to carry it, so callers saw an ordinary error with cleanupIncomplete false while the provider kept the process and refused the next request. After the command returns, combine such a failure with the probe's ProviderCleanupError (cancellation and other command failures keep their own cleanup outcome). Classify request failures through one analysisFailure owner shared by run() and executeAdmitted(), and let the availability probe rethrow any cleanup-incomplete failure instead of reporting the tool unavailable. Refs morluto#3403
…robe's The combined post-run failure was bound only to the probe's pending process cleanup. When the executable re-read had also failed to close its file, the classified failure owned that cleanup, but the combined error's live cleanupResources omitted the file and could clear cleanupIncomplete once the process stopped. Bind both the classified failure and the probe cleanup as dependencies. Refs morluto#3403
Contributor
|
Thanks, 555aaditya, for the quick round. At 48dd89e I read the code and ran no tests. All five items from my review are fixed:
I did not review the cleanup changes added since. 🤖 Written with Claude Code (Sonnet 5.5, verified by Opus 5.5), posted with my OK 🦊 |
10 of 11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_clientoutput 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:fastpassed. 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 andverify-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 onf2cffbdf, including Linux focused tests andverify-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: withREA_TSHARK_COMMANDorREA_CAPINFOS_COMMANDunset, 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 withtshark_missing/capinfos_missing. Migration: put the tool in an absolute PATH directory or set the absoluteREA_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 absoluteREA_TSHARK_COMMAND, so it gains PATH installs and loses nothing.Review follow-ups (merge of
mainat b8c1b45)main'sparseAnalysisViewResult(pcapCaptureView.ts) andprivateWorkspaceFailure(TsharkCaptureProvider.ts), keepingpcapEvidenceSchemas.tests/boundary/cli/pcapCli.test.tschecks.inspect_packet_capturesession availability rule over MCP: PATH-only, absolute and relativeREA_TSHARK_COMMAND, plus the no-absolute-PATH-entry case. The first three are Linux-gated.main's new late-cancellation service fixture now hastool.selection_source.npm run test:fastpassed (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, andnpm run check:fastpassed. I didn't run the Linux-gated availability cases or real TShark locally.Author readiness
Human owner: @555aaditya
Before merge
type(scope): outcome.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:fastpassed. 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.