From 44a7738dd4463a7ef3a0329c6fb77e9051682ba1 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 15 Oct 2026 19:49:56 -0400 Subject: [PATCH] fix(cli): make every help string match what the CLI does (#1796) The CLI help described behaviour the code does not have. -o read "The name of input pcap file"; the input, -o and -a help said extensions are always appended, but -a is off by default; -j, -p and -t each claimed to override all other options, while main() tests -f, then -j, then -p, then -t; -f claimed every dictdumper format, but the formats are those Extractor registers, PCAP included. The rest were vague or ungrammatical ("a extraction", "if ... omits", "Indicate if store buffer", `stdin'). - pcapkit/__main__.py: every help string states the real behaviour, including the format precedence and default, the engine fallback, where -P/-L stop and that third-party engines ignore them, and when -B/-O write a buffer file. No behaviour change. - docs/source/pcapkit/index.rst: the CLI block regenerated from `python -m pcapkit --help` at COLUMNS=80. - tests/cli/test_main.py: pin each corrected help, checking the named precedence against what main() forwards; every new assertion fails on origin/main. Closes #1796 --- docs/source/pcapkit/index.rst | 75 +++++++++------ pcapkit/__main__.py | 74 +++++++++------ tests/cli/test_main.py | 169 ++++++++++++++++++++++++++++++++++ 3 files changed, 259 insertions(+), 59 deletions(-) diff --git a/docs/source/pcapkit/index.rst b/docs/source/pcapkit/index.rst index d316cb1f92..ddec18adca 100644 --- a/docs/source/pcapkit/index.rst +++ b/docs/source/pcapkit/index.rst @@ -82,43 +82,58 @@ Command Line Tool PCAP file extractor and formatted dumper positional arguments: - input-file-name The name of input pcap file. If ".pcap" omits, it will - be automatically appended. Use "-" to indicate reading - from `stdin'. + input-file-name The name of the input PCAP or PCAP-NG file. With + `--auto-extension`, ".pcap" is appended unless its + extension is already ".pcap", ".cap" or ".pcapng". Use + "-" to read from standard input. options: -h, --help show this help message and exit -V, --version show program's version number and exit -o, --output file-name - The name of input pcap file. If format extension - omits, it will be automatically appended. - -f, --format format Print a extraction report in the specified output - format. Available are all formats supported by - dictdumper, e.g.: json, plist, and tree. - -j, --json Display extraction report as json. This will yield - "raw" output that may be used by external tools. This - option overrides all other options. - -p, --plist Display extraction report as macOS Property List - (plist). This will yield "raw" output that may be used - by external tools. This option overrides all other - options. - -t, --tree Display extraction report as tree view text. This will - yield "raw" output that may be used by external tools. - This option overrides all other options. - -a, --auto-extension If output file extension omits, append automatically. - -v, --verbose Show more information. - -F, --files Split each frame into different files. - -E, --engine PKG Indicate extraction engine. Note that except default - or pcapkit engine, all other engines need support of - corresponding packages. + The name of the output report file. If it is omitted, + "out" is used. With `--auto-extension`, the extension + of the output format (e.g. ".json") is appended if + absent. With `--files`, the directory each frame's + report is written to. + -f, --format format The format of the report: "json", "plist" (or "xml"), + "tree" (or "text", "txt") or "pcap" (or "cap"). Takes + precedence over `--json`, `--plist` and `--tree`. With + no format option, "tree" is used. + -j, --json Write the report as JSON, as `--format json` does. + Ignored if `--format` is given. + -p, --plist Write the report as a macOS property list, as + `--format plist` does. Ignored if `--format` or + `--json` is given. + -t, --tree Write the report as tree view text, as `--format tree` + does; this is the default. Ignored if `--format`, + `--json` or `--plist` is given. + -a, --auto-extension Append missing file extensions: ".pcap" to the input + file name, and the extension of the output format + (e.g. ".json") to the output file name unless + `--files` is set. + -v, --verbose Print the input name, the protocol chain of each + frame, and where the report is stored. + -F, --files Write a report file per frame, in the directory named + by `--output`. + -E, --engine PKG The extraction engine, case-insensitive: "default" (or + "pcapkit"), or a third-party one such as "dpkt" or + "scapy", which needs its package installed. An unknown + or unavailable engine falls back to "default". -P, --protocol PROTOCOL - Indicate extraction stops after which protocol. - -L, --layer LAYER Indicate extract frames until which layer. - -B, --buffer-save Indicate if store buffer to file when reading from - stdin. + Stop dissecting each frame after this protocol, e.g. + "IPv4" (case-insensitive); the rest of the frame is + kept as raw data. Third-party engines ignore it. + -L, --layer LAYER Stop dissecting each frame after its protocol in this + layer: "link", "internet", "transport" or + "application" (case-insensitive); "none", the default, + sets no limit. Third-party engines ignore it. + -B, --buffer-save Save a copy of the input to a file as it is read. Only + an input that is not seekable, such as a pipe on + standard input, is saved. -O, --buffer-path file-name - The name of buffer storage file. If `--buffer-save` is - set and this omits, it will be automatically assigned. + The name of the file `--buffer-save` writes. If it is + omitted, a temporary file is used and then deleted. .. versionchanged:: 1.5.0 diff --git a/pcapkit/__main__.py b/pcapkit/__main__.py index 30b9f18d44..1da1699c76 100644 --- a/pcapkit/__main__.py +++ b/pcapkit/__main__.py @@ -46,41 +46,52 @@ def get_parser() -> 'ArgumentParser': description='PCAP file extractor and formatted dumper') parser.add_argument('-V', '--version', action='version', version=__version__) parser.add_argument('fin', metavar='input-file-name', - help=('The name of input pcap file. If ".pcap" omits, ' - 'it will be automatically appended. Use "-" to indicate ' - 'reading from `stdin\'.')) + help=('The name of the input PCAP or PCAP-NG file. With ' + '`--auto-extension`, ".pcap" is appended unless its ' + 'extension is already ".pcap", ".cap" or ".pcapng". ' + 'Use "-" to read from standard input.')) parser.add_argument('-o', '--output', action='store', metavar='file-name', dest='fout', - help=('The name of input pcap file. If format extension ' - 'omits, it will be automatically appended.')) + help=('The name of the output report file. If it is omitted, ' + '"out" is used. With `--auto-extension`, the extension ' + 'of the output format (e.g. ".json") is appended if ' + 'absent. With `--files`, the directory each frame\'s ' + 'report is written to.')) parser.add_argument('-f', '--format', action='store', metavar='format', dest='format', - help=('Print a extraction report in the specified output ' - 'format. Available are all formats supported by ' - 'dictdumper, e.g.: json, plist, and tree.')) + help=('The format of the report: "json", "plist" (or "xml"), ' + '"tree" (or "text", "txt") or "pcap" (or "cap"). Takes ' + 'precedence over `--json`, `--plist` and `--tree`. With ' + 'no format option, "tree" is used.')) parser.add_argument('-j', '--json', action='store_true', default=False, - help=('Display extraction report as json. This will yield ' - '"raw" output that may be used by external tools. ' - 'This option overrides all other options.')) + help=('Write the report as JSON, as `--format json` does. ' + 'Ignored if `--format` is given.')) parser.add_argument('-p', '--plist', action='store_true', default=False, - help=('Display extraction report as macOS Property List ' - '(plist). This will yield "raw" output that may be ' - 'used by external tools. This option overrides all ' - 'other options.')) + help=('Write the report as a macOS property list, as ' + '`--format plist` does. Ignored if `--format` or ' + '`--json` is given.')) parser.add_argument('-t', '--tree', action='store_true', default=False, - help=('Display extraction report as tree view text. This ' - 'will yield "raw" output that may be used by external ' - 'tools. This option overrides all other options.')) + help=('Write the report as tree view text, as `--format tree` ' + 'does; this is the default. Ignored if `--format`, ' + '`--json` or `--plist` is given.')) parser.add_argument('-a', '--auto-extension', action='store_true', default=False, - help='If output file extension omits, append automatically.') + help=('Append missing file extensions: ".pcap" to the input ' + 'file name, and the extension of the output format ' + '(e.g. ".json") to the output file name unless `--files` ' + 'is set.')) parser.add_argument('-v', '--verbose', action='store_true', default=False, - help='Show more information.') + help=('Print the input name, the protocol chain of each ' + 'frame, and where the report is stored.')) parser.add_argument('-F', '--files', action='store_true', default=False, - help='Split each frame into different files.') + help=('Write a report file per frame, in the directory named ' + 'by `--output`.')) parser.add_argument('-E', '--engine', action='store', dest='engine', default='default', metavar='PKG', - help=('Indicate extraction engine. Note that except ' - 'default or pcapkit engine, all other engines ' - 'need support of corresponding packages.')) + help=('The extraction engine, case-insensitive: "default" (or ' + '"pcapkit"), or a third-party one such as "dpkt" or ' + '"scapy", which needs its package installed. An unknown ' + 'or unavailable engine falls back to "default".')) parser.add_argument('-P', '--protocol', action='store', dest='protocol', default=None, metavar='PROTOCOL', - help='Indicate extraction stops after which protocol.') + help=('Stop dissecting each frame after this protocol, e.g. ' + '"IPv4" (case-insensitive); the rest of the frame is ' + 'kept as raw data. Third-party engines ignore it.')) # NOTE: ``choices`` rather than a free-form string, because the layer names # are a closed set (``pcapkit.foundation.extraction.Layers``) and a name # outside it is not rejected anywhere downstream -- it simply never matches a @@ -95,12 +106,17 @@ def get_parser() -> 'ArgumentParser': # lowercase ``'None'`` into the sentinel it wants. parser.add_argument('-L', '--layer', action='store', dest='layer', default=None, metavar='LAYER', type=str.lower, choices=['link', 'internet', 'transport', 'application', 'none'], - help='Indicate extract frames until which layer.') + help=('Stop dissecting each frame after its protocol in this ' + 'layer: "link", "internet", "transport" or "application" ' + '(case-insensitive); "none", the default, sets no limit. ' + 'Third-party engines ignore it.')) parser.add_argument('-B', '--buffer-save', action='store_true', default=False, - help='Indicate if store buffer to file when reading from stdin.') + help=('Save a copy of the input to a file as it is read. Only ' + 'an input that is not seekable, such as a pipe on ' + 'standard input, is saved.')) parser.add_argument('-O', '--buffer-path', action='store', metavar='file-name', dest='buffer_path', - help=('The name of buffer storage file. If `--buffer-save` is set and this ' - 'omits, it will be automatically assigned.')) + help=('The name of the file `--buffer-save` writes. If it is ' + 'omitted, a temporary file is used and then deleted.')) return parser diff --git a/tests/cli/test_main.py b/tests/cli/test_main.py index 11a0d585d8..ea21088109 100644 --- a/tests/cli/test_main.py +++ b/tests/cli/test_main.py @@ -40,6 +40,7 @@ import importlib.util import io import pathlib +import re import sys import types import unittest @@ -211,6 +212,174 @@ def test_get_parser_parses_expected_arguments(self) -> None: self.assertTrue(args.json) self.assertTrue(args.files) + def test_output_help_describes_the_output_file(self) -> None: + """``-o``'s help names the output report, not the input capture. + + Issue #1796: the help was copied from the positional ``input-file-name`` + and read "The name of input pcap file", so ``--help`` said ``-o`` named + the file being *read*. It also said the extension is always appended, + but :func:`~pcapkit.__main__.main` forwards ``-a``/``--auto-extension`` + as ``Extractor(extension=...)``, which is off by default. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, _, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + action = next(action for action in parser._actions # pylint: disable=protected-access + if action.dest == 'fout') + self.assertEqual(action.option_strings, ['-o', '--output']) + + text = action.help.lower() + self.assertIn('output', text) + self.assertNotIn('input', text) + # ``Extractor.__init__`` substitutes "out" for an omitted ``fout``. + self.assertIn('"out"', text) + # The extension depends on ``-a``, and ``-F`` makes ``fout`` a directory. + self.assertIn('--auto-extension', text) + self.assertIn('--files', text) + + def test_input_help_ties_the_pcap_extension_to_auto_extension(self) -> None: + """``input-file-name``'s help appends ``.pcap`` only under ``-a``. + + Issue #1796: the help said ".pcap" is always appended when omitted, but + :meth:`~pcapkit.foundation.extraction.Extractor.make_name` appends it + only with ``extension=True`` -- ``-a`` -- and then unless the name + already ends in one of ``Extractor.PCAP_EXT``. Without ``-a``, + ``pcapkit-cli in`` fails with ``FileNotFound`` for ``in``. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, _, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + action = next(action for action in parser._actions # pylint: disable=protected-access + if action.dest == 'fin') + + text = action.help.lower() + self.assertIn('--auto-extension', text) + self.assertNotIn('automatically appended', text) + # ``Extractor.PCAP_EXT``: a name already ending in one is left alone. + self.assertIn('".cap"', text) + self.assertIn('".pcapng"', text) + + def test_auto_extension_help_covers_input_and_output(self) -> None: + """``-a``'s help names the input extension as well as the output one. + + Issue #1796: the help mentioned only the output file, but ``-a`` is + forwarded as ``Extractor(extension=...)``, which + :meth:`~pcapkit.foundation.extraction.Extractor.make_name` applies to + the input name too -- and not to ``fout`` under ``-F``, where it is a + directory. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, _, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + action = next(action for action in parser._actions # pylint: disable=protected-access + if action.dest == 'auto_extension') + + text = action.help.lower() + self.assertIn('input', text) + self.assertIn('".pcap"', text) + self.assertIn('--files', text) + + @staticmethod + def _named_options(pattern: str, text: str) -> 'set[str]': + """The backquoted options in the clause of ``text`` that ``pattern`` finds. + + Args: + pattern: A regular expression whose first group is the clause. + text: A help string. + + Returns: + The options named in the clause, such as ``'--json'``, or an empty + set when ``text`` has no such clause. + + """ + match = re.search(pattern, text) + return set(re.findall(r'`(--[a-z-]+)`', match.group(1))) if match else set() + + def _forwarded_format(self, module, extractor_cls, *argv: str): + """The ``format=`` that :func:`main` hands ``Extractor`` for ``argv``.""" + with mock.patch.object(sys, 'argv', ['pcapkit-cli', 'capture.pcap', *argv]): + self.assertEqual(module.main(), 0) + return extractor_cls.created[-1].kwargs['format'] + + def test_format_switch_help_names_what_overrides_it(self) -> None: + """``-j``, ``-p`` and ``-t`` each name exactly the options that beat them. + + Issue #1796: each of them said "This option overrides all other + options", which cannot hold for all of them at once, and holds for none: + :func:`~pcapkit.__main__.main` tests ``--format`` first, then + ``--json``, then ``--plist``, then ``--tree``. So each help's "Ignored + if ... is given" clause is checked against what ``main`` forwards when + the switch is given together with each other format option. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, extractor_cls, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + helps = {action.dest: action.help for action in parser._actions} # pylint: disable=protected-access + # ``pcap`` is no stand-in constant, so a ``--format`` win is visible. + options = {'--format': ('--format', 'pcap'), '--json': ('--json',), + '--plist': ('--plist',), '--tree': ('--tree',)} + + for switch in ('--json', '--plist', '--tree'): + with self.subTest(switch=switch): + text = helps[switch[2:]] + alone = self._forwarded_format(module, extractor_cls, switch) + winners = {other for other, argv in options.items() if other != switch + and self._forwarded_format(module, extractor_cls, switch, *argv) != alone} + self.assertNotIn('overrides all other options', text) + self.assertEqual(self._named_options(r'Ignored if (.+?) (?:is|are) given', text), + winners) + + def test_format_help_names_the_switches_it_overrides(self) -> None: + """``-f``'s help lists the real formats and the switches it beats. + + Issue #1796: it said "Available are all formats supported by + dictdumper", but the formats are the keys of ``Extractor.__output__``, + which include ``pcap``, a :mod:`pcapkit.dumpkit` writer, and it said + nothing of ``-j``/``-p``/``-t``, which :func:`~pcapkit.__main__.main` + consults only when ``--format`` is not given. With no format option at + all ``main`` forwards :data:`None`, and ``Extractor.__init__`` + substitutes ``tree``. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, extractor_cls, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + text = next(action.help for action in parser._actions # pylint: disable=protected-access + if action.dest == 'format') + beaten = {switch for switch in ('--json', '--plist', '--tree') + if self._forwarded_format(module, extractor_cls, '--format', 'pcap', switch) == 'pcap'} + + self.assertNotIn('dictdumper', text) + self.assertIn('"pcap"', text) + self.assertIn('no format option, "tree" is used', text) + self.assertEqual(self._named_options(r'precedence over (.+?)\.', text), beaten) + + def test_help_strings_have_no_known_slips(self) -> None: + """No help string keeps a slip the rendered-docs proofread found. + + Issue #1796, from the #1103 proofread: "a extraction", "if ... omits" + for "is omitted", "Indicate if store buffer", and the unbalanced + quotes of ``stdin'``. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, _, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + slips = ('a extraction', 'omits', 'Indicate if', "`stdin'") + found = [(action.dest, slip) for action in parser._actions # pylint: disable=protected-access + for slip in slips if slip in (action.help or '')] + self.assertEqual(found, []) + def test_layer_and_protocol_reach_the_extractor(self) -> None: """``-L``/``-P`` are forwarded under the names ``Extractor`` reads.