fix(dumpkit): a nameless flag member no longer renders as Type::None [0] (#648)

[JarryShaw]

Fixes #648.

The defect

make_dumper's object_hook renders every enumeration member as Type::name [value], interpolating o.name with no guard. A Flag value composed entirely of undeclared bits has name is None rather than a string, so the literal four characters None landed in the name half:

JSON:  "connection": "Flags::None [0]",
Tree:  |-- connection -> Flags::None [0]
PLIST: <string>Flags::None [0]</string>

The three sites

All three carried character-for-character the same interpolation, and all three are in pcapkit/dumpkit/common.py:

pre-change line	branch	before	after
:204	MultiDict/OrderedMultiDict key path	{'Flags::None [0]': [...]}	{'Flags::0 [0]': [...]}
:213	the addon branch's 'enum' key	{'enum': 'Sub::None [0]', ...}	{'enum': 'Sub::0 [0]', ...}
:216	the scalar return	'Flags::None [0]'	'Flags::0 [0]'

They now share one render_enum helper, so the guard cannot be applied to two of three, and a fourth site cannot be added without it. render_enum is documented under Internal Definitions in docs/source/pcapkit/dumpkit/common.rst, alongside DumperBase — it is deliberately not added to __all__.

What a nameless member renders as, and why

The value's own decimal spelling. Flags(0) → 'Flags::0 [0]', Flags(8) → 'Flags::8 [8]'.

Three reasons, each measured on this tree under CPython 3.14.7:

1. It is what the enumeration libraries already do for an undeclared residue. Flags(2049).name is 'ACK|1' and Flags(2057).name is 'ACK|9' — the declared bit is named and the leftovers are given as one decimal number. A wholly-undeclared value is that same rendering with no declared bit in front, so the fallback continues an existing convention instead of inventing one.
2. It cannot be mistaken for a member name. A Python identifier may not begin with a digit, so a bare decimal is structurally impossible as a declared name — 0 of the 1867 identifiers declared under pcapkit/const is a pure decimal. 'None' had no such protection, and NONE is a real declared name elsewhere in the library (Integrity(0).name == 'NONE'), so a consumer splitting the rendering on :: genuinely could not tell "no flags set" from a member so named.
3. It needs no special case for zero, which matters because the defect never was about zero.

Alternatives considered and rejected: repr(o) gives 'Flags::<Flags: 0> [0]', which is redundant and nests brackets inside the bracketed half; an explicit token (NONE, UNNAMED, <unnamed>) reintroduces exactly the collision problem in (2); and dropping the ::name half to give Flags [0] changes the shape every consumer parses, which is a larger break than changing one field's content.

Two corrections the issue established, both reproduced

Not limited to value 0. The guard is on name is None, not on the value, because any all-undeclared value is nameless:

Flags(0)      name=None -> 'Flags::0 [0]'
Flags(1)      name=None -> 'Flags::1 [1]'
Flags(8)      name=None -> 'Flags::8 [8]'
Flags(9)      name=None -> 'Flags::9 [9]'
Flags(65536)  name=None -> 'Flags::65536 [65536]'

A fix keyed on zero would have left four of those five still emitting None. The test covers all five, so it cannot pass a zero-only fix.

Not an aenum quirk. A stdlib enum.IntFlag built from the same members answers name is None identically on 3.14.7, so the guard belongs at the interpolation rather than in a choice of enumeration library. The test asserts both libraries side by side.

One thing the issue undercounted

Five of the seven flag registries are nameless at zero, not one — pcapkit.const.tcp.flags.Flags plus the four Mobility Header flag registries (BindingACKFlag, BindingUpdateFlag, HandoverACKFlag, HandoverInitiateFlag). The remaining two (TransportProtocol, CommandType) declare an explicit undefined = 0 and were always fine. The new test discovers the registries rather than naming them, so an eighth cannot slip past, and it asserts the nameless count is at least five so it cannot silently stop exercising the fix.

Interaction with #634

#634 has merged, so a flagless TCP segment now reaches this path and connection became a string where it had been the JSON number 0. #634's reviewer judged that an improvement because the field is now consistently a string instead of switching type with the flag bits — that judgement is unaffected here. What changes is only the name half of that string: "Flags::None [0]" becomes "Flags::0 [0]". The field stays a string, so the consistency #634 bought is kept; what it gains is that the string no longer claims a member name of None. #634's own test comment said the None was "left to its own change" — this is that change, and its assertion and prose are updated here.

Blame puts all three lines in dfac5d1767 (2023-04-28), three years before #634: pre-existing, not introduced by it.

The committed fixtures do not move

examples/captures/out.json, out.plist and out.txt are unchanged, and this was measured rather than assumed. All three were regenerated from in.pcap with and without the change and the two sets are byte-identical — render_enum is the identity on every member that has a name, and no committed fixture contains a nameless one (grep '::None \[' returns nothing in all three).

A side-finding, reported and deliberately not fixed here: the committed fixtures have already drifted from what the current tree produces, independently of this change. That drift is substantial, not cosmetic — an earlier draft of this description called it "a +00:00 timezone suffix and different packet-byte offsets", which undersold it, and the cross-review was right to push back. It also includes whole frames whose hex payloads differ in content rather than offset, protocol -> NIL becoming protocol -> 5001, and packet -> NIL becoming real WireGuard-secrets bytes.

None of it is caused by this PR — the A/B regeneration above isolates that question and answers it byte-identically — but it means examples/captures/out.* no longer reflect what the library emits, which is worth its own change and is not one I have made here.

Failing, then passing

Both runs used the same harness: __editable__* stripped from sys.meta_path, the worktree at sys.path[0], pcapkit.__file__ asserted and printed before any other import, and pytest's exit code read from a file rather than a pipeline — the wrapper reported rc=0 while pytest exited 1, which is exactly the trap.

MEASURED pcapkit.__file__ = …/.claude/worktrees/agent-a3c59f390d4ed0e74/pcapkit/__init__.py

On main (fix reverted, tests present):

tests/dumpkit/test_nameless_enum_rendering_unit.py
  AssertionError: 'StdFlags::None [65536]' != 'StdFlags::65536 [65536]'
  18 failed, 2 passed, 2 subtests passed
  exit code (from file): 1

test_tcp_udp_unit.py::…::test_a_flagless_segment_seeds_its_connection_flags_as_an_enum
  AssertionError: 'Flags::None [0]' != 'Flags::0 [0]'
  1 failed, 4 subtests passed
  exit code (from file): 1

With the fix:

tests/dumpkit/ + test_tcp_udp_unit.py::…::test_a_flagless_segment_…
  15 passed, 25 subtests passed
  exit code (from file): 0

That 15 is the whole of tests/dumpkit/ — the pre-existing test_common_unit.py (9 tests) plus the new module (5) — and the one amended tcp test. Spelled out because it is not the new module's own figure: alone the new module is 5 passed / 17 subtests, and with the amended tcp test 6 passed / 21 subtests. The cross-review could not reconcile 15 against a module-only run, which is the ambiguity this paragraph removes.

Wider scoped run — tests/dumpkit/ tests/foundation/ tests/protocols/transport/test_tcp_udp_unit.py:

1 failed, 256 passed, 11 skipped, 397 subtests passed

That one failure is test_tcp_runtime.py::TCPReassemblyRuntimeTests::test_sample_capture_reassembles_every_stream_byte_exactly, which raises FileNotFoundError for a generated fixture in a tree where make samples has not been run. It fails identically with the fix reverted, so it is pre-existing and unrelated — the fixture-dependent tier, exactly as tests/_tiers.py documents.

The suite was never run whole; every run was scoped.

Coverage

pcapkit/dumpkit/common.py, statement and branch, measured with coverage directly:

	statements	branches	cover	tests	subtests
before	71	32	100%	9	4
after	76	34	100%	14	21

Coverage does not go backwards: it holds at 100% while the file grows by 5 statements and 2 branches, and the new name is None guard is covered on both arms.

Labels — fix + breaking

fix is uncontested. On breaking, arguing it both ways as asked:

For. This changes user-visible output in all six format names (json, tree, text, txt, plist, xml) out of both Extractor and TraceFlow. Since #634 merged it is reachable from a real capture — any flagless segment, an nmap NULL scan being the canonical example — so it is not hypothetical. A consumer that string-matched Flags::None [0] breaks. .github/release.yml reads PR labels only, so omitting the label is the only way this becomes a silent change to the library's principal output, and the repo's own precedent (#635, fix(pcap) + a BREAKING CHANGE note for a public-attribute meaning change) points the same way.

Against. The label reads "Alters public API or wire output", and the wire bytes are untouched — o.value and the [value] half are unchanged, only the name half moves. Read narrowly, a dump-rendering change is neither the public API nor the wire. And the set of affected inputs is small: no registry other than the five nameless-at-zero flag enumerations can produce it.

Applied, because breaking is additive and the asymmetry is decisive: a false positive costs one honest line in the release notes, while a false negative ships a changed output format with nothing telling anyone. Happy to drop it if the label is meant strictly as "wire".

One more thing found and deliberately not fixed

pcapkit.const.pcapng.option_type.OptionType(0) has a string-typed .value ('opt_endofopt [0]'), so it already renders with nested brackets — 'OptionType::opt_endofopt [opt_endofopt [0]]'. That member is named, so render_enum treats it identically to the old code and this PR neither causes nor worsens it. Surfaced by the cross-review; a latent oddity in the rendering format, left to its own change.

Cross-review

Dispatched on a different model, briefed to falsify rather than bless; verdict posted as a comment below.

[JarryShaw]

Cross-review verdict: GOOD TO GO

Independent cross-review, run on a different model (Sonnet) from the one that authored the change, in its own worktree, briefed to falsify rather than bless and to treat the issue's own measurements as claims rather than ground truth. It was read-only with respect to this PR: no amend, no push, no commit to the branch.

Tree it measured: …/.claude/worktrees/agent-a61980e1fa8991520/pcapkit/__init__.py, at commit 1db6232d, with every __editable__-named finder stripped from sys.meta_path, cached pcapkit* modules purged, and the worktree pinned at sys.path[0] on every run. Python 3.14.7.

Per-claim verdicts

claim	verdict	evidence it obtained
A three sites, all fixed, none missed	TRUE	render_enum at common.py:168 and its three call sites :258, :267, :270 are the only Type::name [value] rendering in pcapkit/ or tests/. Other .name interpolations (esp.py, extraction.py, pcapng.py key-log writers, fields/*.py) are unrelated messages or a different format.
B nameless renders as its decimal value	TRUE	Ran the real hook: Flags(0)→'Flags::0 [0]', (1)→'Flags::1 [1]', (8), (9), (65536) likewise.
C Flags(2049).name == 'ACK|1', Flags(2057).name == 'ACK|9'	TRUE	Executed directly. The PR's central justification holds.
D no declared identifier is a pure decimal	TRUE	Reproduced 1867 distinct names from 2068 declaration lines, and swept 10,367 canonical members at runtime. Zero pure decimals by both countings.
E not an aenum quirk	TRUE	Stdlib enum.IntFlag gives name is None at 0, 8, 65536 identically.
F five registries nameless at zero, not one	TRUE — "the PR is right where the issue was wrong"	Flags, BindingACKFlag, BindingUpdateFlag, HandoverACKFlag, HandoverInitiateFlag; TransportProtocol and CommandType name their zero.
G fixtures do not move	Core claim TRUE; my drift description was an understatement	Controlled A/B with only common.py toggled: diff empty for all three files. But the committed-vs-current drift is larger than I said — see below.
H fails on main, passes with the fix	TRUE, exact match	Pre-fix: 18 failed / 2 passed, exit file 1, including the quoted 'StdFlags::None [65536]' != 'StdFlags::65536 [65536]'. Post-fix: exit file 0.
I the test discriminates a wrong fix	TRUE — the important one	A zero-only guard ('0' if o.value == 0 else None) → 10 failed, exit 1. An unconditional 'NONE' → 18 failed / 3 passed, exit 1. Both wrong fixes are caught.
J the edit to test_tcp_udp_unit.py is honest and minimal	TRUE	Only adds an assertion (assertNotIn('None', rendered)) beside the changed pinned value; nothing weakened or deleted. It independently traced the four MH registries through mh.py and confirmed they are never constructed as IntFlag instances on the read path (MH flags are decomposed into named bits via BitField), so the rewritten claim that a flagless TCP segment is the library's only wire-reachable producer of a nameless member is true.
K the breaking label	"reasonable, not dishonest"	Wire bytes are untouched, but #634 — the closely analogous predecessor that changed the same connection field's dumped type — also carries fix+breaking, so applying it here is consistent with the repo's own precedent.
L correctness risks not mentioned	No new bug	str(o.value) is safe: all five currently-reachable nameless members have int values. Exhaustively checked all 10,391 named members — zero differ between the old rendering and render_enum. render_enum outside __all__ under "Internal Definitions" matches 15 other doc pages.

Structural checks also passed: one commit, author Jarry Shaw <jarryshaw@icloud.com>, Fixes #648, only a .rst doc touched (no .md), no GH-nnn.

What it disputed, and what I changed

Two findings, both acted on rather than argued away:

1. My side-finding on fixture drift undersold it. I had written "a +00:00 timezone suffix and different packet-byte offsets". The reviewer found whole frames whose hex payloads differ in content, protocol -> NIL becoming protocol -> 5001, and packet -> NIL becoming real WireGuard-secrets bytes. I verified this myself before accepting it. The description is corrected in the PR body. The core claim is unaffected — the A/B regeneration isolates this PR's contribution and it is byte-identical — but the drift is substantial and examples/captures/out.* no longer reflect what the library emits. Still deliberately not fixed here; it wants its own change.

2. A number it could not reconcile. It could not locate or reproduce "15 passed / 25 subtests" against a module-only run, getting 5/17 (module alone) and 6/21 (module plus the amended tcp test). Both figures are right: my 15 was the whole of tests/dumpkit/ — the pre-existing test_common_unit.py plus the new module — and the tcp test. The PR body now spells out the selection so the number is checkable.

3. It also surfaced a latent, pre-existing oddity: OptionType(0) has a string-typed .value, so it already renders with nested brackets as 'OptionType::opt_endofopt [opt_endofopt [0]]'. That member is named, so render_enum is identical to the old code there. Recorded in the PR body, deliberately not fixed.

Nothing in A–L was left unverified.