Conversation
|
GOOD TO MERGE Independent cross-review on a different model from the author (this review ran on Claude Sonnet 5; authorship model unstated). Reviewed head Per-claim verdicts1. The fix actually fixes it — CONFIRMED. Ran the new file against the PR tree: 2. Fixes the cause, not the symptom — CONFIRMED. 3. Scope — CONFIRMED. 4. Sweep exhaustiveness / registry count — CONFIRMED, 7 registries. Verified two independent ways: (a) grepping the source for 5. Does not mask a real defect — CONFIRMED (no masking found). For every one of the 7 registries, 6. New assertions are load-bearing — CONFIRMED. Read subtest counts, not just top-line status, per this repo's own gotcha: 7. Coverage does not regress — CONFIRMED (no regression; marginally more). 8. Nothing else silently changed — one REFUTED claim inside the new docstring, otherwise CONFIRMED. Read the full diff. The new helper functions ( However, the closing paragraph of
This is factually wrong, and I can show it directly. I ran To be clear about what this does not affect: it's a narrative overclaim in a comment, not a functional problem. The new test is still a legitimate, non-vacuous addition — it pins Could not verify
SummaryRegistry count: 7 flag registries, 4 distinct guard widths (3/4/8/16 bits). Fix verified to close #702 without touching |
…idth (#702) `tests/dumpkit/test_nameless_enum_rendering_unit.py` swept the literal `NAMELESS_VALUES = (0, 1, 8, 9, 65536)` against `pcapkit.const.tcp.flags.Flags`, and `65536` is `0x10000` -- one bit past the sixteen-bit field that registry's `_missing_` bounds itself to. The guard is right, so the probe was what had to move: a value a registry is correct to refuse cannot also be a value the dumper is expected to render. `main` has been failing on the `library='aenum'` subtest since #670 introduced the file, and every PR whose Unit Tests job reached it failed for reasons that were not its own. Swept all seven flag registries under `pcapkit/const/` before choosing a direction, since aborting at the first failure hid the rest: * All seven bound `_missing_` to their own field, at four distinct widths -- three bits for `CommandType`, four for `TransportProtocol`, eight for `BindingACKFlag`, `HandoverACKFlag` and `HandoverInitiateFlag`, sixteen for `BindingUpdateFlag` and `Flags`. Exempting the width-guarded registries would therefore have exempted every one of them and left this half of the sweep with nothing in it, which is why the probe is derived per registry instead. * `65536` is out of range for all seven, not only for `Flags`. And `8` and `9` are *named* in `BindingACKFlag` and `TransportProtocol` rather than nameless, so one fixed literal was never right for more than one registry. * For all seven the bound is exactly the smallest all-ones mask covering every declared bit, which `TransportProtocol` already spells for itself as `max(cls.__members__.values()) * 2 - 1`. `_field_mask` derives that, so it needs no table to maintain and cannot drift from the guard. * `_nameless_values` returns the values a registry admits that no member names: no bits at all, each undeclared bit alone, and every undeclared bit at once. The last takes over `9`'s multi-bit role and is the largest nameless value the field holds, which is what the out-of-range literal was reaching for. * `test_no_flag_registry_renders_the_literal_none` sweeps those values rather than `registry(0)` alone. It was broad across registries and one value deep in each, so the four Mobility Header registries only ever saw a single value. * `test_a_value_past_the_field_is_refused_rather_than_rendered` asserts that rejection instead of tripping over it, and pins the derivation itself: the widest in-field value is accepted and the next one up is not, which holds only if the derived mask is the bound each registry declared. No `pcapkit` line changed. The file goes from `1 failed, 5 passed, 16 subtests passed` to `6 passed, 64 subtests passed`, and `tests/dumpkit/` is 15 passed. Coverage rises rather than falls, which deleting the literal on its own would not have managed: `tcp/flags.py:91` was reached only by this file failing on it, and the same `raise` in the other six registries was reached by nothing at all. All seven are now covered -- `ftp/command.py:76`, `mh/binding_ack_flag.py:72`, `mh/binding_update_flag.py:87`, `mh/handover_ack_flag.py:60`, `mh/handover_initiate_flag.py:63`, `reg/apptype.py:65` and `tcp/flags.py:91` -- for six fewer missed statements and six fewer partially-covered branches across those modules. Against the #648 guard reverted the sweep fails 27 subtests and the scalar test 21; against `Flags`' width widened to `0xFFFFFFFF` the new test fails on `value=65536`, which is the trade #702 warns against. Fixes #702
e47c092 to
eb16442
Compare
|
Cross-review verdict: GOOD TO GO — independent agent, different model (Sonnet), briefed to falsify rather than confirm. One dispute, fixed in Everything else it re-derived independently and matched: seven flag registries under It also confirmed the three |
Fixes #702.
mainhas been red on this file, and every PR whose Unit Tests job reached it failed for reasons that were not its own. Test-only change; nopcapkitline is touched.The defect
tests/dumpkit/test_nameless_enum_rendering_unit.py:18swept a fixed tuple:65536is0x10000, one bit past the sixteen-bit fieldpcapkit.const.tcp.flags.Flagsbounds its_missing_to atpcapkit/const/tcp/flags.py:90-91. The test asked the registry to mint a nameless pseudo-member for a value it is correct to refuse. The guard is not the defect — the TCP flags field really is 16 bits — so the probe is what moved.The registry sweep, which decided the direction
I swept every flag enumeration under
pcapkit/const/**before choosing, because the failure aborts at the first one and hides the rest. There are seven, and all seven carry a width-bounding_missing_, at four distinct widths:ftp.command.CommandType0x070reg.apptype.TransportProtocolmax(members) * 2 - 1=0x0F0mh.binding_ack_flag.BindingACKFlag0xFF0, 1mh.handover_ack_flag.HandoverACKFlag0xFF0, 1, 2, 4, 8, 16, 31mh.handover_initiate_flag.HandoverInitiateFlag0xFF0, 1, 2, 4, 8, 15mh.binding_update_flag.BindingUpdateFlag0xFFFF0, 1, 2, 4, 8, 15tcp.flags.Flags0xFFFF0, 1, 2, 4, 8, 15That result is what picked the fix, and it rules the alternative out rather than merely disfavouring it:
library='aenum'half of the sweep with nothing in it and only the file's local stdlib replica — not library code — still tested. So direction 2 from the issue is not viable.65536is out of range for every one of the seven, not just forFlags.8and9are named inBindingACKFlag('B','B|1') and inTransportProtocol('dccp','tcp|dccp'), not nameless. So the fixed literal was never correct for more than one registry, in its in-range part either.What changed
Per-registry derivation, with no table to maintain and nothing parsed out of anyone's source:
_field_mask— for all seven, the declared bound is exactly the smallest all-ones mask covering every declared bit, whichTransportProtocolalready spells for itself asmax(cls.__members__.values()) * 2 - 1because it extends itself at runtime. So the width is an invariant of the declared members._nameless_values— the values a registry admits that no member names: no bits at all, each undeclared bit alone, and every undeclared bit at once. The last takes over9's multi-bit role and is the largest nameless value the field holds, which is what the out-of-range literal was reaching for. Never returns a value the registry would refuse, which is the fix.test_no_flag_registry_renders_the_literal_nonenow sweeps those values instead ofregistry(0)alone. It was broad across registries and one value deep in each, so the four Mobility Header registries only ever saw the single value they share.test_a_value_past_the_field_is_refused_rather_than_rendered(new) asserts the rejection rather than tripping over it, and pins the derivation itself: the widest in-field value is accepted and the next one up is not, which holds only if the derived mask is the bound each registry declared.65536survives here as the bound of the two sixteen-bit registries — asserted as the refusal it always was.Evidence
Measured on the repo venv (CPython 3.14.7, one of the failing versions),
PYTHONSAFEPATH=1withpcapkit.__file__asserted into the branch checkout before trusting any number.test_nameless_enum_rendering_unit.py1 failed, 5 passed, 16 subtests passed, exit 16 passed, 64 subtests passed, exit 0tests/dumpkit/(whole package)15 passed, 68 subtests passed, exit 0, zero SUBFAILsThe failing subtest was
SUBFAILED(library='aenum', value=65536)→ValueError: 65536 is not a valid Flagsatpcapkit/const/tcp/flags.py:91, exactly as filed.Both changed/new tests are shown to fail without their fix:
pcapkit/dumpkit/common.py(droppingif name is None: name = str(o.value)): the sweep fails 27 subtests and the scalar test 21 — where the oldregistry(0)-only sweep could only ever have caught 5.Flags' guard to0xFFFFFFFF— the trade tests: the nameless-enum sweep probes 65536 against a 16-bit flag registry, so main fails without showing red #702 explicitly warns against: the new test fails withSUBFAILED(registry='pcapkit.const.tcp.flags.Flags', value=65536),AssertionError: ValueError not raised.Both mutations were reverted;
git diff origin/main..HEADis the one test file.Coverage
Coverage rises, which deleting the literal on its own would not have managed —
tcp/flags.py:91was covered only by this file failing on it, so the naive fix would have lost it. The sameraisein the other six registries was reached by nothing at all. All seven guardraises are now covered:ftp/command.py:76,mh/binding_ack_flag.py:72,mh/binding_update_flag.py:87,mh/handover_ack_flag.py:60,mh/handover_initiate_flag.py:63,reg/apptype.py:65,tcp/flags.py:91.Across the eight affected modules (
coverage run -m pytest, nopytest-cov): missed statements 1655 → 1649, partial branches 16 → 10.const/ftp/command.pyconst/mh/binding_ack_flag.pyconst/mh/binding_update_flag.pyconst/mh/handover_ack_flag.pyconst/mh/handover_initiate_flag.pyconst/reg/apptype.pyAppTypedominates the percentage)const/tcp/flags.pydumpkit/common.pyNotes
ValueErrorthe new test asserts is the bare built-in, deliberately:enumrequires it from a refused lookup, and fix(const): give the six unguarded registries the same bare-ValueError guard #677 brought the last six divergent registries onto the bare built-in the rest ofpcapkit/const/already raised. Nopcapkit/const/**file is modified, so that convention is left exactly as it is.Cross-review
Reviewed by an independent agent on a different model (Sonnet), briefed to falsify rather than confirm. Verdict: GOOD TO GO. It re-derived all seven registries, the mask/guard equivalence, the four distinct widths, the nameless-value sets, the two registries with none, and coverage of all seven
raiselines — and reproduced both mutation results (27/21 subtest failures, and the65536rejection) independently in a throwaway copy of the tree.It disputed one thing, which is fixed in this branch: a code comment read "#677 settled on the bare built-in for all 113 generated registries", but 113 was #677's pre-fix conforming count, not "all" — that commit went on to fix six more. The comment now says #677 brought the last six divergent registries onto the bare built-in the rest of
pcapkit/const/already raised. No other disagreement, and it confirmed the threeassertGreaterEqualbounds are tight (exactly 7, 4 and 5) rather than slack, so none of them can pass vacuously.