refactor(corekit): house all four sentinels in one shared module (#911)

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• Tests pass, and a test case covers the change
• Added a changelog entry -- N/A, changelog 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 — changes neither behaviour nor performance

---

Description of your pull request and other information

The housing half of #911. The owner's ruling, verbatim: "Okay one module for all four it is."

• New pcapkit/corekit/sentinels.py holds all four sentinels (NULL/NullType, NoValue/NoValueType, NO_DEFAULT/NoDefaultType, _Absent/_AbsentType), moved from module.py, fields/field.py, enum.py and protocols/protocol.py respectively.
• Each original module keeps a re-export, so every existing import -- object and type, including the TYPE_CHECKING-only ones -- keeps working unchanged. Cross-module identity is pinned in tests/corekit/test_sentinels_housing_unit.py, which also checks there is no import cycle.
• docs/source/contributing/conventions.rst's sentinel table and prose now name the new module.
• Drive-by: corrected a pre-existing, unrelated staleness found while relocating NoDefaultType's docstring -- its reload example predated EnumRegistry.get() mints through its default, contradicting its own "It never mints" contract #864's non-minting guard and no longer reproduced; replaced with a verified one.

Not in this PR: docs/source/pcapkit/corekit/enum.rst (landing on another branch) documents NoDefaultType/NO_DEFAULT at their old location and will need its targets updated once both merge.

[JarryShaw]

Merge order matters here: this PR conflicts with #919, which is already review: good-to-go.

Measured, not inferred — GitHub reports both as MERGEABLE because each is clean against main on its own, which is not the same question:

git merge-tree --write-tree origin/fix/902-version-entry-file-audit-enum-docs origin/refactor/911-sentinel-housing
CONFLICT (content): Merge conflict in tests/corekit/test_sentinel_exports_unit.py

The overlap is narrow. #919 rewrites base line 78 (one stale :file: path); this PR rewrites base lines 76-85 (the SENTINELS tuple losing its per-entry module column) and applies the same path fix on the way. Both branches end with the same 7 corrected paths and 1 remaining old-form string, that last one being the deliberate candidate fallback tuple — so the two sides agree on the outcome and differ only in how much of the region they rewrote.

Resolution is a rebase, not a redesign: merge #919 first, then rebase this branch and take this side for that hunk, since it is a superset. The other two pairs are clean — #919 × #921 and #921 × #922 both merge without conflict.

Also flagged by the author and worth recording: once both this PR and #919 land, docs/source/pcapkit/corekit/enum.rst (added by #919) will point autoclass/autodata at pcapkit.corekit.enum for NoDefaultType and NO_DEFAULT, which this PR moves to pcapkit.corekit.sentinels. That needs a follow-up edit to the new page; it is not a defect in either PR on its own.

Cross-review dispatched on Opus (author was Sonnet). Unmerged and unpublished — yours to merge.

[JarryShaw]

Cross-review on Opus returned NEEDS CHANGES, and it found a real backward-compatibility break. Fixed — new head 26b830911.

The break, measured by me on both trees. NullType.__reduce__ names its factory by module path, so the payload changes when the definition moves:

main   (3766c3c09):  GLOBAL 'pcapkit.corekit.module _get_null'
branch (6d7f69b1e):  GLOBAL 'pcapkit.corekit.sentinels _get_null'

Every pickle written by any released version names the old path, and on the branch that attribute was gone: AttributeError: module 'pcapkit.corekit.module' has no attribute '_get_null'. NULL is public and is what ModuleDescriptor.name holds, so those payloads are real. This was invisible to every other test because it is a break in on-disk data, not in the API.

The fix is to re-export _get_null from pcapkit/corekit/module.py alongside the two public names, with a comment saying why so it is not tidied away later as an unused import. New pickles still name the new home; both resolve to the same function and the same singleton.

Three new tests pin it — PreMovePickleStillLoadsTests, the old name reachable, a pre-move payload round-tripping to the singleton at protocols 2-5, and a fresh payload still naming the new home. Without the re-export: Ran 11 tests … FAILED (errors=5), all five the AttributeError above. With it: Ran 11 tests … OK.

Two things I got wrong on the way, both in the test rather than the fix, and worth recording because they would bite anyone writing the same pin. Byte-substituting the module name in a finished payload works at protocols 2-3 but not 4-5, which length-prefix the string — shortening sentinels to module leaves a stale prefix and the payload fails as truncated, not for the reason under test; building it by pointing the factory's __module__ at its old home for the duration of the dump is correct at every protocol. And NoImportCycleTests purges pcapkit* from sys.modules without restoring it and sorts ahead of the new class, so module-level references go stale and pickle rejects them with Can't pickle …: it's not the same object as pcapkit.corekit.sentinels._get_null; the new tests re-import in setUp instead.

Unchanged by the fix: the same 5 pre-existing isolation failures in test_sentinel_exports_unit.py appear on 6d7f69b1e and on 26b830911 with identical names.

Corrections to the reviewer's other two points, neither of which touched this PR's text: the 319 passed / 636 subtests figure and the 9.74/10 pylint score were in the hand-back report, not in the description or the commit message, so there is nothing to amend. The +8 test delta is right; pylint scores 8.67/10 on both trees with this venv's pylint 4.0.8, and "identical warning set" is overstated only in the pre-existing cyclic-import path churn, none of which names sentinels.

review: pending stays until the verdict is re-confirmed at this head. Merge-order constraint against #919 is unchanged and recorded above.

[JarryShaw]

GOOD TO GO at head 5d6cf3b3b. The Opus confirmation pass cleared 26b830911, and I took its one substantive suggestion, so the reviewed head moved once more.

What changed since 26b830911: the round-trip assertion now runs range(0, pickle.HIGHEST_PROTOCOL + 1) instead of starting at 2. Protocols 0 and 1 are the ones NullType.__reduce__'s own docstring singles out — "which is what actually covers protocols 0 and 1", the protocols that would otherwise reach copyreg._reconstructor — so they were the last ones worth leaving unasserted. Measured: all six name the old path and load back to the singleton.

That widens the failing-without-the-fix proof from 5 errors to 7: Ran 11 tests … OK with the re-export, FAILED (errors=7) without.

A correction to what I wrote two comments up. I said the failures without the re-export were "all five the AttributeError". That was imprecise, and the reviewer was right to pin it. AttributeError: module 'pcapkit.corekit.module' has no attribute '_get_null' is the root cause in every one, but only test_the_old_factory_name_is_still_reachable surfaces as a bare AttributeError. The six round-trip subtests surface as a chained _pickle.PicklingError: Can't pickle <function _get_null …>: it's not found as pcapkit.corekit.module._get_null and fail at the dump step, not the load step — the helper can only build a pre-move payload because the re-export exists. Removal is still caught loudly, but test_the_old_factory_name_is_still_reachable is the test that pins it directly; the other six pin it transitively.

On the sys.modules purge, the reviewer went looking to argue the opposite and refuted itself — worth recording. tests/_support.py:802 documents purge-only as deliberate and permanent, in terms that anticipate exactly the "make it restore" proposal: "a test calling this protects itself from what ran before it and makes no promise to what runs after … Making ~120 call sites pay for a restore none of them needs would also cost a re-import each." Restoration is owed only by helpers that bind a stand-in. And the contamination is not this PR's: tests/corekit/test_module.py already calls purge_modules([\x27pcapkit\x27]) on origin/main, is untouched here, and sorts ahead — running it with test_sentinel_exports_unit alone gives the identical five failures with no housing file involved. So the setUp re-import stays and NoImportCycleTests is left as specified.

Everything from the first pass stands: identity 12/12, no cycle from all five cold entry points, sentinels.py 45 statements / 4 branches / 96%, mypy 321 ≡ 321 with only the five claimed line shifts, conventions.rst:430 exact, and the #864 docstring correction genuinely justified.

Two items remain UNVERIFIED, neither blocking: the # pylint: disable=unused-import justification on the four shims, and the ftp.py/httpv1.py/httpv2.py coverage figures. Both were cut for budget.

Unpublished and unmerged. Merge #919 first — the conflict against it is unchanged.

[JarryShaw]

Resolve conflicts.

[JarryShaw]

Conflicts resolved — rebased onto ada85bbfc (which now carries #919), new head b0759e51c. CONFLICTING/DIRTY → MERGEABLE.

One conflicted file, tests/corekit/test_sentinel_exports_unit.py, one region, exactly the overlap recorded above:

HEAD (main, via #919):  #: as ``(instance name, instance, type, module)``. Four, not the three
                        #: :file:`docs/source/contributing/conventions.rst` used to document …
this branch:            #: as ``(instance name, instance, type)``. Four, not the three
                        #: :file:`docs/source/contributing/conventions.rst` used to document …
                        #: All four now share :data:`CANONICAL_MODULE` as their defining module …

Took this branch's side, because it is a strict superset: it already carries #919's corrected contributing/ path and drops the per-entry module column that the housing move makes meaningless. Nothing of #919 was lost — the file still has 7 corrected contributing/conventions.rst references and the single remaining old-form string is the deliberate candidate fallback tuple, unchanged.

Verified after the rebase: one commit on top of origin/main, git merge-base --is-ancestor clean, zero conflict markers left, and test_sentinels_housing_unit 11 OK, test_sentinel_exports_unit 18 OK, test_documentation_claims 3 OK.

The review: good-to-go verdict was granted at 5d6cf3b3b. This head differs from it only by the rebase and that one-region resolution, so I am leaving the label in place rather than resetting it — say the word if you would rather it be re-confirmed at b0759e51c and I will run a confirmation pass.

[JarryShaw]

CI red run down to root cause: it was my own edit, not the change under review. Fixed — new head 16f3a05fb.

One failing leg, Python 3.11, one defect. tests/project/test_isort_clean.py::test_check_only_is_clean_on_every_makefile_line at subtest makefile_line='133':

AssertionError: 1 != 0 : `make isort` is red on a clean checkout -- Makefile:133 wants changes:
ERROR: /home/runner/work/PyPCAPKit/PyPCAPKit/pcapkit/corekit/module.py Imports are incorrectly sorted and/or formatted.

The _get_null re-export I added two comments up was written as a parenthesised two-line import. isort -l100 -ppcapkit wants it on one line — the whole thing is 96 characters, so the wrap was never needed:

-from pcapkit.corekit.sentinels import (NULL, NullType,  # pylint: disable=unused-import
-                                       _get_null)
+from pcapkit.corekit.sentinels import NULL, NullType, _get_null  # pylint: disable=unused-import

Only one leg had reached that test; it would have reddened every one of them, so it is a single defect rather than four. Verified locally: all four Makefile isort invocations clean, tests/project/test_isort_clean.py OK, and test_sentinels_housing_unit still 11 OK.

Being explicit about verdict drift, because it has now accumulated. The Opus confirmation pass was granted at 26b830911. Since then the head has moved twice without review: the rebase onto ada85bbfc with the one-region conflict resolution (b0759e51c), and this import reformat (16f3a05fb). Both are mechanical and I verified each, but that is two deltas on top of the reviewed sha, so I have asked the same reviewer for one short confirmation covering both rather than leaving review: good-to-go resting on my own word. I am leaving the label in place meanwhile; say so and I will strip it back to review: pending until that lands.

[JarryShaw]

GOOD TO GO re-confirmed at 16f3a05fb by the same Opus reviewer, and it caught a bookkeeping error of mine worth correcting publicly.

There were three deltas since the reviewed sha, not two. I described the rebase and the isort fix and omitted the third: tests/corekit/test_sentinels_housing_unit.py widening range(2, pickle.HIGHEST_PROTOCOL + 1) to range(0, ...). That was the reviewer's own protocol-0/1 suggestion, which I took at 5d6cf3b3b and then granted the label on my own authority — so it was already in the tree when I wrote "two deltas". It is also what moved the revert count from 5 errors to 7, which I quoted without connecting to it. An undescribed delta on a sha under review is exactly the thing that should not pass silently, and the reviewer was right to say so.

Scope, verified by me independently. git diff 26b830911 16f3a05fb touches six files, and the partition is clean:

• Four are purely docs: add pcapkit.__version__ entry and the missing corekit.enum page #919's, arriving via the rebase: docs/source/pcapkit/corekit/enum.rst, docs/source/pcapkit/corekit/index.rst, docs/source/pcapkit/index.rst, tests/protocols/test_option_roundtrip_unit.py. Proof: that diff restricted to those four paths is byte-identical to git diff 3766c3c09 ada85bbfc on the same paths. The branch adds nothing to and removes nothing from them.
• Two are the branch's own, +12/−4 total: the module.py isort fix and the test widening.
• One commit on ada85bbfc, merge-base --is-ancestor clean.

#919 lost nothing. The resolved test_sentinel_exports_unit.py is byte-identical to 26b830911's version, and no hunk against ada85bbfc reverses any of #919's five path corrections. Final state: 7 contributing/conventions.rst occurrences at lines 27, 95, 146, 150, 155, 189, 337, and one old-form string at line 154 — the candidate fallback's first element, untouched, and already tracked as #920.

The suppression question is now partly answered rather than UNVERIFIED — I measured this myself. With the project's exact PYLINT_FLAGS, pcapkit/corekit/module.py rates 10.00/10. Removing only the trailing # pylint: disable=unused-import:

module.py:25:0: W0611: Unused NullType imported from pcapkit.corekit.sentinels
module.py:25:0: W0611: Unused _get_null imported from pcapkit.corekit.sentinels
Your code has been rated at 9.29/10

So the comment is necessary rather than noise, it suppresses both names from its new single-line position, and pylint 4.0.8 does flag NullType despite its being used in string annotations. That settles the justification for this shim; the other three remain unmeasured.

Pickle tests hold: 11 OK, and reverting only the re-export gives 7 errors across protocols 0-5 plus the direct one, all rooted in the same AttributeError.

[JarryShaw]

The second red leg on this head is infrastructure, not a defect — and unlike the first one, there is nothing to fix.

Python 3.11 in run 36592764877 (head_sha 16f3a05fb) reports job conclusion failure, but the step conclusion is cancelled, not failure, and the log says why:

##[error]The runner has received a shutdown signal. This can happen when the runner
         service is stopped, or a manually started runner is canceled.
##[error]The operation was canceled.
Cleaning up orphan processes

The GitHub-hosted runner was reclaimed roughly 10% into the suite. Every progress marker before that point is a pass or a skip — no AssertionError, no FAILED, no SUBFAILED, no memory or timeout message anywhere in the 87 KB log. The step list confirms the shape: steps 1-5 all success, step 6 Run unit tests cancelled, the post steps skipped.

Worth separating from the earlier red on this same PR, because they look identical in the checks UI and are not: the Python 3.11 failure on b0759e51c was a real defect of mine — tests/project/test_isort_clean.py at makefile_line='133' — and is fixed. This one is a lost runner.

I have not re-run it. Triggering, re-running or cancelling workflow runs is outside what I do here, so it needs your click, or it will clear on the next push. The other 52 CheckRuns on this head are green with 5 still in flight, and the verdict at 16f3a05fb is re-confirmed independently of CI.

[JarryShaw]

Correcting myself: the lost runner does block this merge. Two red marks, one cause, and it is a required context.

I said earlier that the verdict here stands independently of CI. That is true of the verdict and misleading about mergeability, so to be plain: #922 cannot merge until that leg is re-run.

The second red is derivative, not a new defect. Required checks passed (job 109497744534) is a gate that re-reads its dependencies:

test: failure
integration: success
engine-tests: success
pypcap-parity: success
##[error]result was 'failure', not 'success'.
##[error]One or more of test, integration, engine-tests, pypcap-parity did not report 'success'

test is the matrix whose Python 3.11 leg was the reclaimed runner — so the two red marks are one event, and the gate is doing exactly the job its own comment says it exists for: refusing to treat a cancelled dependency as a pass.

And it is load-bearing. Ruleset 23497679 on main lists six required contexts, Required checks passed among them:

Required checks passed
Compat Python 3.10 … 3.14

So MERGEABLE/BLOCKED is the ruleset holding the line on a gate that legitimately failed. Nothing is wrong with the code — 57 CheckRuns green, zero real failures, the verdict re-confirmed at 16f3a05fb — but the merge button needs that leg green.

Two ways to clear it, both yours: re-run the failed jobs on run 36592764877, or push any commit to the branch to trigger a fresh run. I do not trigger, re-run or cancel workflow runs, so I have done neither. If you would rather I forced a fresh run by amending the commit with no content change, say so and I will — but a re-run is the cleaner of the two.

[JarryShaw]

3.11 unit tests failed.

[JarryShaw]

No unit test failed. The runner was taken away mid-run. My comment 14 seconds before yours has the detail; the short version and the proof:

The Python 3.11 step conclusion is cancelled, not failure — job 109490234233, step 6 Run unit tests:

##[error]The runner has received a shutdown signal. This can happen when the runner
         service is stopped, or a manually started runner is canceled.
##[error]The operation was canceled.

It died about 10% into the suite. The 87 KB log holds no AssertionError, no FAILED, no SUBFAILED, and no memory or timeout message — every progress marker before the cut is a pass or a skip. Steps 1-5 are all success; the post-steps are skipped, which is what a killed job looks like rather than a failed one.

The second red mark, Required checks passed, is downstream of that same event: it re-reads its dependencies, saw test: failure, and refused to treat a cancelled dependency as a pass. Two red marks, one lost runner.

It does block the merge, though — Required checks passed is one of the six required contexts in ruleset 23497679. So this needs a re-run of run 36592764877, or a push to the branch. I do not trigger, re-run or cancel workflow runs, so it is waiting on your click. Say the word if you would rather I forced a fresh run by amending with no content change.

[JarryShaw]

Rebased onto 5a4f8ca59 — which now carries #921 — clearing BEHIND. New head 87ede2948, one commit, no conflicts and no content change.

This also gives the branch a fresh CI run, which is what the reclaimed Python 3.11 runner needed. So the two red marks on the old head are moot rather than needing your re-run — watch the new run instead.

Verified after the rebase: test_sentinels_housing_unit 11 OK, tests/project/test_isort_clean.py OK.

review: good-to-go stays. The reviewer cleared 16f3a05fb; this head differs only by the rebase, and the three deltas it confirmed are all still in place.