Skip to content

feat(protocols)!: move OSPF and RARP to the application layer (#719) - #1031

Merged
JarryShaw merged 1 commit into
mainfrom
feat/719-ospf-rarp-application-layer
Oct 5, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
feat/719-ospf-rarp-application-layer

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort)
  • Test cases cover the change — three new/moved files under tests/protocols/application/
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf / refactor / test / docs / ci / chore

Description of your pull request and other information

Implements the layer-placement ruling on #719, and depends on the Application loosening that merged as #1030. The IETF is the single source of truth (RFC 1122 §1.1.3, RFC 1812 §7), so OSPF moves to application/ with Application as its only base and RARP as class RARP(Application, ARP) — base order load-bearing, layer base first. The data/ and schema/ modules and the .rst pages move with them. A new conventions page states the rule with its citations.

Dispatch entries do not move. RARP is still reached from Link.__proto__ by EtherType and OSPF from Internet.__proto__ at TransType.OSPFIGP; only the module they point at changes. That decoupling of dispatch tier from subpackage is the thing most likely to look wrong to a reader, so the page says it explicitly.

One correction carried in, because I got it wrong on the issue thread. I had written that Link and Application define "precisely the same three names beyond ProtocolBase", so OSPF would inherit only layer from Link. That is false. Measured on this tree, excluding dunder boilerplate, Link overrides __proto__, _read_protos and register where Application does not — but all three also exist on ProtocolBase, so the strict difference Link − Application − ProtocolBase is empty and each still resolves. What changes is which registry: OSPF.__proto__ becomes ProtocolBase.__proto__, so OSPF no longer sees Link's EtherType entries, while register and _read_protos resolve on ProtocolBase. RARP keeps Link's through ARP.

That turns out to be behaviourally nil, and the page and a new test now say why rather than asserting it: OSPF falls back to the root ProtocolBase.__proto__, a -1 lookup is absent from both registries and resolves to Raw either way, and _lookup_next_layer inserts nothing on a miss, so neither registry grows.

A second correction, already posted on the issue: there is no extraction-boundary change. layer='internet' gives IPv4:OSPFIGP before and after — IPv4 terminates the chain on its own. The only observable movement is _sigterm, on RARP as well as OSPF, inverting between layer='link' and layer='application' while protochains stay identical at every layer= value including unset.

Verified locally, in eleven separate narrow runs, because CI cannot tell us. The Plain unittest ordering (protocols) leg currently cancels at its 45-minute cap on every run including main's (#1029), so local coverage is the only real evidence here: application 152 passed, link 35, internet 279, transport 152, schema 38, misc 116, dispatch 24, protocol-base/registry 49, option-roundtrip 6, tests/project 268, tests/foundation 270. No chunk skipped; every summary line read.

Against the pre-move tree, 12 of the 17 cases in test_layer_placement_unit.py fail, plus 7 subtests of test_the_names_left_the_link_subpackage. Five are regression guards that pass either way: ARP and InARP staying Link, layer='internet' stopping at IPv4, Link owning __layer__ where Raw does not, RARP over Ethernet under every layer limit, and unlimited extraction. The four failures in the relocated test_ospf_unit.py / test_rarp_unit.py are import failures on the old tree, so they are moves rather than new coverage.

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) feat Pull requests that add a new capability (feat: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 5, 2026
Layer is decided by designed function, with the IETF as the single source of
truth: RFC 1812 §7 places routing protocols in the application layer and
RFC 1122 §1.1.3 lists RARP there. Ruled on #719.

* `OSPF` moves to `application/` with `Application` as its only base. `RARP` and
  `DRARP` move as `class RARP(Application, ARP)`: layer base first, because
  `ARP`'s chain reaches `Link`, which owns `__layer__`. `layer` is now
  `'Application'` for all three; `ARP` and `InARP` stay `'Link'`.
* The matching `data/` and `schema/` modules and the `.rst` pages move too. No
  re-export is left at the old paths. `pcapkit/const` and `pcapkit/vendor` do not
  move.
* Dispatch keys are unchanged: OSPF stays in `Internet.__proto__` at
  `TransType.OSPFIGP`, RARP in `Link.__proto__` by EtherType. Only the module each
  descriptor names changes.
* Neither class overrides `__post_init__`, `_decode_next_layer` or
  `_import_next_layer`; both rely on the `-1` sentinel `Application` now accepts.
* `OSPF.__proto__` becomes `ProtocolBase.__proto__`, so OSPF no longer sees Link's
  EtherType entries. `register` and `_read_protos` still resolve, on `ProtocolBase`
  rather than Link -- the strict difference Link minus Application minus ProtocolBase
  is empty. Inert either way: a `-1` lookup misses in both registries, resolves to
  `Raw`, and inserts nothing on the miss.
* New conventions page `protocol-layer-placement`, and a 1.5.0 breaking-change
  entry listing the before/after import paths.

Protochains are unchanged at every `layer=` value, `'internet'` included; only
`_sigterm` moves, and on RARP as well as OSPF. Every tests/protocols subtree, tests/project
and tests/foundation pass locally, run in separate chunks.
@JarryShaw
JarryShaw force-pushed the feat/719-ospf-rarp-application-layer branch from b05ddc1 to 0d382c3 Compare October 5, 2026 15:36
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on b05ddc15b: NEEDS CHANGES (ran on Opus; authored on Sonnet). Every behavioural claim was confirmed — the code is sound. All four findings were prose, and all four are fixed in 0d382c302, now pushed.

1. My test-count split was wrong in both numbers. I wrote "11 of the 17 cases fail" and "Six are regression guards". Measured on an extracted main tree: 12 distinct methods not passing, 5 passing, with 7 subtest failures in test_the_names_left_the_link_subpackage as claimed. The review's sharpest catch is that my own enumerated guard list contains exactly five items while the prose says "Six" — the list was right, the count wrong. Corrected to 12 and five.

2. A stale cross-reference the sweep missed. examples/generators/dispatch.py:34 still read pcapkit.protocols.link.arp.RARP. This PR fixed five sibling refs in that same file and missed this one. It was already wrong on main — link/arp.py has __all__ = ['ARP', 'InARP'], so RARP was never there — but it is now doubly wrong, and it was the only stale hit in the tree. Repointed at application.rarp.RARP.

3. My own correction overstated, which makes this my third pass at the same claim. I had said OSPF "loses the EtherType registry, register and _read_protos". Measured: all three are also in ProtocolBase.__dict__, so the strict difference Link − Application − ProtocolBase is empty and hasattr(OSPF, 'register') is True. What actually changes is which registry it resolves to — OSPF.__proto__ is now ProtocolBase.__proto__ rather than Link.__proto__, so OSPF no longer sees Link's EtherType entries, while register and _read_protos still resolve on ProtocolBase. The conventions page, the changelog and the commit message all say that now.

4. _sigterm moves on RARP too, not only OSPF. Measured on the moved tree: RARP's flag is True at layer='application' and False at unset, 'link' and 'internet' — the same inversion. My "only _sigterm on the OSPF instance" was too narrow.

Also fixed, from its unprompted findings: documentation.rst:234 had been rewritten to application/rarp.rst inside a sentence that cites commit 68fbccd90, where the file was still at link/rarp.rst. On a page about not overstating claims, a historical finding should keep its historical path — it now names the path at the time and notes the move.

What it confirmed by independent derivation, all against a real extracted main tree rather than my figures: dispatch still resolving from the unchanged tiers by real parse (Link.__proto__[RARP] → application.rarp.RARP, Internet.__proto__[OSPFIGP] → application.ospf.OSPF); both RARP base orders, with the inversion shown to be an MRO property rather than an artefact of the move; no per-class re-pointing anywhere (9/9 absent); the pin at 162; and protochains byte-identical at all six layer= values, with layer='internet' giving IPv4:OSPFIGP on both trees. On the registry it ran the clause I most wanted checked, with a control: -1 is absent from both registries, resolves to Raw in both, neither grew, while a bare defaultdict does insert on lookup.

One thing I got wrong while verifying its findings, worth stating because I have been warning every agent about exactly it: my first re-run of tests/protocols/application showed 5 failures, and they were entirely my own missing fixture captures — 2 tracked against 23 generated. After make_samples.py: 420 passed, 1 skipped, 1349 subtests. The agent's figures were right and mine were the artefact.

Resetting review: since the head moved.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: running A cross-review is in flight against the current head - no verdict yet review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 5, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 0d382c302: GOOD TO GO (ran on Opus; authored on Sonnet). All four findings fixed, and the fix diff is prose-only — which is what let the first round's behavioural verification carry forward untouched.

Scope, verified independently rather than taken on my word: git diff --numstat b05ddc15b 0d382c302 is five files — CHANGELOG.md, 1.5.0.rst, documentation.rst, protocol-layer-placement.rst and one docstring line in examples/generators/dispatch.py. Zero files under pcapkit/ or tests/, still one commit on 02a4e7e82.

  • The counts now read 12 and five, and the enumerated guard list matches the five methods its own comm produced, summing to 17.
  • The stale-path sweep at the new head returns zero hits outside the four known-legitimate files, so the link.arp.RARP reference is gone and nothing new appeared.
  • The overstatement is corrected in all three documents plus the commit message, each stating the measured facts: strict difference empty, __proto__ repointed at ProtocolBase.__proto__, register and _read_protos still resolving there, miss inserting nothing. It also re-checked the page's surrounding arithmetic — seven / eight / four shared — against its own measured sets.
  • It checked something I had not flagged: the changelog gained net lines, which could have broken the pin. It did not — the additions are continuation prose inside existing bullets, so bullets stay at 162 against process.rst:104's 162, nine sections, changelog_md.py --check in step.

One residual imprecision it named, which I am recording here rather than pushing a fourth revision for. "The same inversion applies to RARP" is only half-observable: RARP's flag does go False → True at layer='application', but at layer='link' RARP is never instantiated in an Ethernet-framed capture on either tree — my own probe returned NoPayload there with the chain reading Ethernet:Reverse_Address_Resolution_Protocol. So for RARP the 'link' half is inferred from the shared mechanism rather than measured. Directionally correct and hedged, but the precise statement is: RARP's _sigterm becomes True at 'application' where it was False; the 'link' half of the inversion is observable for OSPF only. Not worth another CI cycle; worth being on the record.

Two gaps I am carrying rather than closing, both UNVERIFIED on its side: make pylint / make mypy / make isort, and a Sphinx build. The review makes a fair point that the Sphinx gap is now only a process gap for this change — with the sweep clean there is no dead cross-reference left for it to catch — but it is the check that would have found the link.arp.RARP reference in the first place, so the gap is mine rather than the change's.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 5, 2026
@JarryShaw
JarryShaw merged commit 30714cd into main Oct 5, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the feat/719-ospf-rarp-application-layer branch October 5, 2026 16:19
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 5, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
@JarryShaw JarryShaw moved this to Done in PyPCAPKit Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) feat Pull requests that add a new capability (feat: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant