[JarryShaw]

• Searched for similar pull requests
• Followed the coding style
• make test passes, and a test case covers the change
• Changelog entry: N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• refactor — code refactoring

---

Description

Closes #2318.

• Problem: two guards could never run, because something earlier had already raised — Header.read's FileError on an unknown magic number, which the schema's magic_number_callback beats to it; and _read_opt_calipso's two length guards, which calipso_pad_len beats to it while the option is unpacked. Beside them sat commented-out raise lines next to the warn that replaced them, a repeated pylint: disable=unused-argument, and three names nothing read: dispatch.py's _internet_enum, a FAKE_GH query no caller sends, and GATING.
• A third candidate, _read_opt_qs's unknown QS function arm, is NOT dead — the unassigned-option dispatch it depends on is by schema class, decided from the func a parse's own registry resolves it against, not from the func held in a directly-constructed schema's flags. A QuickStartRequestOption or QuickStartReportOption built directly with an unassigned func reaches this method's own check regardless, and a parse reaches it too once a caller registers a schema against that func, so it stays, with a Note on each of the three readers explaining why.
• Fix: the two genuinely unreachable guards are deleted, with the prose that described them corrected — Header.read documents ProtocolError, and _read_opt_calipso documents what checked the length instead. The three fabricated-schema stanzas that reached the deleted guards by calling a private reader directly are replaced by the real parse paths, which raise as before.
• Test: tests/protocols/test_dead_branches_2318_unit.py parses what was supposed to reach each deleted branch and pins what happens instead, pins the restored QS guard against both schema kinds across all three protocols through the public construction path and through a parse once a schema is registered against the func, and pins the source as holding no such deleted branch, no commented-out raise beside a warn, and no repeated pragma; tests/project/test_dead_helpers_2318_unit.py pins the rule that made each helper dead. 8 of the 17 test methods across the two files fail on the parent commit; the other 9 pin unchanged behaviour by design.

[github-actions]

Coverage: 91.58% (unit tier, Python 3.15, ab96134d8, Unit Tests run success)

Package	Statements	Missed	Branches	Partial	Cover
pcapkit (top level)	104	4	20	2	95.16%
pcapkit/const	19601	881	2620	867	92.12%
pcapkit/corekit	2596	74	818	37	96.51%
pcapkit/dumpkit	282	3	102	3	98.44%
pcapkit/foundation	3029	23	1050	25	98.82%
pcapkit/interface	138	4	52	3	96.32%
pcapkit/protocols	17444	218	4588	185	98.15%
pcapkit/toolkit	914	36	334	10	95.51%
pcapkit/utilities	502	7	144	5	98.14%
pcapkit/vendor	4432	1988	1018	167	51.87%

Per-file detail: the coverage-html artifact of this run.

[JarryShaw]

Review: NEEDS CHANGES at 962827881 (Sonnet). The Header and CALIPSO unreachability claims hold (114,877-case differential fuzz, 0 outcome differences). But the unknown QS function arm is reachable via the public construction path — HOPOPT(options=[QuickStartReportOption(..., flags={'func': 1, ...})]) and the IPv6_Opts/IPv4 equivalents now silently succeed and write wire bytes for an unassigned function, where main correctly raised ProtocolError. A Request-class schema with the same func now raises an unrelated AttributeError instead. This contradicts the removal's core premise — it wasn't dead code, just unreachable from the one path the earlier #2318 investigation checked (calling the private reader directly).

Also: the two new test modules violate the #1103 house convention (change-relative "are gone"/"Three guards"/counted-number prose instead of timeless statements), and the PR body's "every case fails on the parent commit" is false for 6 of 14 methods (by design — they pin unchanged behavior).

Restoring the guard per the reviewer's recommendation, plus the prose/body corrections. Fix in progress.

[JarryShaw]

Amended 962827881 → 993f4a244 (squashed cleanly to one commit — caught my own mistake mid-amend: pushed a second commit on top instead of replacing the first, fixed with git reset --soft origin/main before it went further).

Restored the unknown QS function guard in all three readers (now confirmed reachable via the public construction path, not just the private-reader-direct-call the original analysis checked), added a test pinning it through that real path (30 subtests: 5 unassigned func values × 3 protocols × 2 schema classes), fixed the #1103 prose violations, and added a Note to each reader's docstring explaining why the guard stays — the dispatch to _read_opt_none/_read_opt_unassigned is by schema class decided from file-read octets, not by function value, so a directly-constructed schema with an unassigned func still reaches this check. Title/commit/body corrected: 2 guards removed (not 3), scope and fail-without-fix counts restated accurately. 16/16 tests pass; Header and CALIPSO removals untouched and still pass their existing tests.

[JarryShaw]

Review: NEEDS CHANGES at 993f4a244 (Sonnet, round 2). Restored guard confirmed byte-identical to main (AST comparison), round-1 repro fully reproduced and fixed (96-case sweep), mechanism verified by instrumenting the dispatch directly, Header/CALIPSO removals confirmed untouched (45,002-case fuzz). Three issues, all test/prose, no pcapkit/ behavior:

1. Two stale, now-false assertions never reverted. Round 1 replaced test_ipv4_unit.py/test_ipv6_extension_unit.py's private-reader qs_unknown assertions with a comment claiming the reader "keeps no branch" for an unassigned function — exactly the premise that caused the original defect, now false again since the guard is restored. These need reverting back to asserting the behavior, not just commenting about it, or a future sweep could cite the stale comment to delete the guard a third time.
2. Test-count claim never recomputed. "8 of 14... other 6" is round 1's stale count. Reviewer's actual count: 8 of 16 fail across the two files (12+4 methods), 8 pin unchanged behavior by design.
3. New overclaim in the restored test's own prose: "a parse cannot reach it" / "reachable only through construction" — false once a caller registers a further QS function code (confirmed: parsing ordinary bytes through a registered subclass for func 3 hits the same guard). Needs qualifying to "the stock registry." Also a counted-number violation ("Two here are not reachable" next to "the two length guards").

Fix in progress.

[JarryShaw]

Amended 993f4a244 → 0d7b200b0: reverted the two test files' stale "reader keeps no branch" comments back to real assertions (byte-identical to origin/main on test_ipv4_unit.py, verified load-bearing — patching out the restored guard breaks exactly the 3 expected tests), fixed the stale test-count claim (now accurately 8 of 17 across both files — the +1 from a new test closing the overclaim in item 3 below), and corrected the overclaim that a parse can never reach the guard — confirmed false by registering a QuickStartOption subclass against an unassigned func and parsing plain octets through it, which hits the same ProtocolError on all three readers. Added a test pinning that parse-reachable case so the corrected prose has coverage behind it. Dropped the remaining counted-number violation. 104/104 affected tests pass.

All findings across all 3 rounds now addressed.

[JarryShaw]

Review: GOOD TO GO at 0d7b200b0 (Sonnet, round 3). Exhaustive re-verification given this PR's history: reverted test files confirmed blob-identical/AST-identical to origin/main where appropriate, restored guard load-bearing (same 3 tests fail when stripped), parse-reachability independently reproduced with different func values/layout than the test (output identical on main and head), counts correct (8 of 17), Header/CALIPSO removals confirmed untouched this round via a per-function AST diff plus a fresh 4524+2560+35420-case sweep through BOTH construction and parse routes (the exact gap that caused round 0's original mistake). Full regression clean.

Two before-merge follow-ups, both reasonable — will do: (1) note the QS deviation on #2318 itself, since closing it via this PR would otherwise leave the issue's "dead" claim unaddressed where the owner ruled on it; (2) add a CALIPSO construction-route test as a backstop, since that route is correctly unreachable today but has no test pinning it.

[JarryShaw]

Amended 0d7b200b0 → ab96134d8: added the recommended CALIPSO construction-route backstop test (same 4 length/cmpt_len cases as the existing parse-path test, mutation-verified — temporarily disabling calipso_pad_len's guard fails all 8 new subtests, restored). 14/14 pass.

Both follow-ups from round 3 now done: the #2318 clarifying comment and this test.

[JarryShaw]

Ready to merge.

Review: GOOD TO GO (Sonnet, round 3) at ab96134d8.
CI: Required checks passed, 0 failures (only the non-blocking experimental GraalPy leg still pending).
Mergeable, 0 unresolved threads.