Skip to content

Commit d8d8dea

Browse files
levylevy
committed
docs: add INET requirements, naming conventions, and enforcement tooling
Add under doc/architecture: - requirements.md and architectural-requirements.md: user-facing R-* and dev-facing AR-* requirements with a quality-attribute lens and enforcement map - naming-conventions.md covering every name category, with known deviations tracked in the naming-exceptions.md ledger - sealing.md and sealing-status.md: file-sealing mechanism (rules + status) - architecture-exceptions.md ledger seeded with allowlisted foundational types - enforcement/: check-architecture.sh (with subtree-scope arg), .clang-tidy config, and T4 agent-review checklist; audit report for common/packet in reports/common-packet.md
1 parent b52bc21 commit d8d8dea

11 files changed

Lines changed: 1841 additions & 0 deletions

‎doc/architecture/architectural-requirements.md‎

Lines changed: 670 additions & 0 deletions
Large diffs are not rendered by default.

‎doc/architecture/architecture-exceptions.md‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# Architecture Exceptions and Violations
2+
3+
Known places where the INET code base departs from the dependency-direction requirements in
4+
[architectural-requirements.md](architectural-requirements.md) — chiefly **AR-ORG-DOMAINS**
5+
(dependencies point protocols → infrastructure, never the reverse) and **AR-ORG-VIS-SPLIT** (model
6+
code must not depend on the visualizer). It is the architecture counterpart of
7+
[naming-exceptions.md](naming-exceptions.md), and it is seeded automatically from
8+
[`check-architecture.sh`](enforcement/check-architecture.sh) — re-running that script *is* the audit.
9+
10+
Each entry has a **disposition**:
11+
12+
- **Sanctioned** (`AS-*`) — a deliberate, accepted coupling. Sanctioned entries are added to the
13+
allowlist in `check-architecture.sh` so the check stays quiet about them.
14+
- **Violation** (`AV-*`) — a real coupling to fix; the check keeps flagging it until it is inverted
15+
or the offending type is relocated. Each has a suggested fix and a status (`Open` / `Done`).
16+
17+
The underlying tension is that `common/` legitimately holds framework-wide *value types* and *node
18+
abstractions*, but some of those historically live under a protocol layer, and some genuinely
19+
protocol-specific code has drifted into `common/`. The two need to be told apart.
20+
21+
---
22+
23+
## Sanctioned exceptions
24+
25+
| Id | Coupling | Rule | Why it stays |
26+
|---|---|---|---|
27+
| AS-01 | `common/` → address & protocol-id value types: `MacAddress`, `Ipv4Address`, `Ipv6Address`, `L3Address`, `L3AddressResolver`, `EtherType`, `IpProtocolId` | AR-ORG-DOMAINS | Foundational value types used framework-wide; they *should* live in `common/`, but relocating them is a large, mechanical, high-churn change. Allowlisted in `check-architecture.sh`. **Real fix:** move them under `common/`. |
28+
29+
---
30+
31+
## Open violations
32+
33+
Grouped by cluster (the check reports the individual `file:line` hits). Counts are from the
34+
2026-07-20 audit.
35+
36+
### Node-structure coupling — *decide*
37+
38+
| Id | Coupling | Where (examples) | Suggested resolution | Status |
39+
|---|---|---|---|---|
40+
| AV-ORG-01 | `common/` infra → `NetworkInterface` / `IInterfaceTable` / `InterfaceTable` / `InterfaceTag` (~11) | `MessageDispatcher`, `LifecycleController`, `InterfaceOperations`, `IInterfaceRegistrationListener`, `packet/recorder/*` | These are foundational *node-structure* abstractions, like AS-01 — either **sanction + allowlist** them (and ideally move them to `common/`), or invert via a `common/`-side interface. Pick one and record it. | Open (decide) |
41+
42+
### Observation/recording infra → physical layer — *decide*
43+
44+
| Id | Coupling | Where | Suggested resolution | Status |
45+
|---|---|---|---|---|
46+
| AV-ORG-02 | `common/` observation code → `physicallayer` `Signal` / `ReceptionBase` / `IReception` / `ITransmission` / `INarrowbandSignalAnalogModel` / `SignalTag` (~9) | `ResultFilters.cc`, `packet/printer/PacketPrinter.h`, `packet/recorder/PcapRecorder.cc` | Result filters and packet recorders inherently observe every layer. Either accept as observation infrastructure, or relocate the recorders/result-filters out of `common/` into a dedicated observation package that is *allowed* to depend downward. | Open (decide) |
47+
48+
### Genuine violations — *fix*
49+
50+
| Id | Coupling | Where | Suggested resolution | Status |
51+
|---|---|---|---|---|
52+
| AV-ORG-03 | `common/socket/SocketMap` → concrete transport sockets `TcpSocket`, `UdpSocket` | `common/socket/SocketMap.cc` | `SocketMap` should be generic over `ISocket`/`INetworkSocket`, not concrete transport types (AR-ORG-CONTRACTS). | Open |
53+
| AV-ORG-04 | `common/clock` → `applications/base/ApplicationBase` | `common/clock/ClockUserModuleMixinImpl.cc` | Clock infrastructure must not know about an application base class; remove the include / depend on a neutral abstraction. | Open |
54+
| AV-ORG-05 | `common/ResultFilters` → `applications/base/ApplicationPacket_m` | `common/ResultFilters.cc` | Move the application-specific result filter out of `common/`, or depend on a neutral packet tag. | Open |
55+
| AV-VIS-01 | model code → `visualizer/` (AR-ORG-VIS-SPLIT) | `mobility/base/MobilityBase.cc`, `environment/ground/OsgEarthGround.cc` | Invert: the visualizer must subscribe to mobility/environment from outside, not the reverse; move any OSG rendering hooks into the visualizer package. | Open |
56+
57+
---
58+
59+
## Auditing
60+
61+
Re-running [`check-architecture.sh`](enforcement/check-architecture.sh) from the INET repo root reproduces the
62+
violation list. When it reports something new:
63+
64+
1. If the coupling is a deliberate, accepted framework-wide dependency, add an `AS-*` row **and** add
65+
the header to the allowlist in `check-architecture.sh`.
66+
2. Otherwise add an `AV-*` row with a suggested fix and `Status = Open`; when the coupling is inverted
67+
or the type relocated, set `Status = Done` (keep the id).
68+
69+
### Audit coverage
70+
71+
| Area | Date | Findings |
72+
|---|---|---|
73+
| `check-architecture.sh` over `src/inet` (AR-ORG-DOMAINS, AR-ORG-VIS-SPLIT) | 2026-07-20 | AS-01 (allowlisted); AV-ORG-01…05 + AV-VIS-01. Include-graph only — behavioral violations of these ARs (e.g. vis *logic* not reached via an include) are not covered by this check and need T4 agent review. |

‎doc/architecture/enforcement/.clang-tidy‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# Starter clang-tidy config that enforces the C++ side of naming-conventions.md
2+
# (which concretizes AR-QUAL-NAMING). This is a STARTER staged under doc/architecture/ — to
3+
# activate it, move it to the repository root so it applies to all of src/inet.
4+
#
5+
# Scope: this only sees C++ identifiers. The NED-, .msg-, .ini-, and .oppfeatures-side
6+
# rules in naming-conventions.md need a separate small linter.
7+
#
8+
# Expect first runs to report the known deviations catalogued in naming-exceptions.md
9+
# (rtp leading-underscore members NV-08, PascalCase static constants NV-10, etc.).
10+
# Feed anything new into that ledger; don't weaken the rules here to match the code.
11+
12+
Checks: '-*,readability-identifier-naming'
13+
WarningsAsErrors: ''
14+
HeaderFilterRegex: 'inet/.*'
15+
16+
CheckOptions:
17+
# Types are PascalCase (clang-tidy's "CamelCase" == UpperCamel)
18+
- { key: readability-identifier-naming.ClassCase, value: CamelCase }
19+
- { key: readability-identifier-naming.StructCase, value: CamelCase }
20+
- { key: readability-identifier-naming.UnionCase, value: CamelCase }
21+
- { key: readability-identifier-naming.EnumCase, value: CamelCase }
22+
- { key: readability-identifier-naming.TypeAliasCase, value: CamelCase }
23+
- { key: readability-identifier-naming.TypedefCase, value: CamelCase }
24+
- { key: readability-identifier-naming.TypeTemplateParameterCase, value: CamelCase }
25+
26+
# Functions / methods / variables / members / parameters are camelCase ("camelBack")
27+
- { key: readability-identifier-naming.FunctionCase, value: camelBack }
28+
- { key: readability-identifier-naming.MethodCase, value: camelBack }
29+
- { key: readability-identifier-naming.MemberCase, value: camelBack }
30+
- { key: readability-identifier-naming.PublicMemberCase, value: camelBack }
31+
- { key: readability-identifier-naming.ProtectedMemberCase, value: camelBack }
32+
- { key: readability-identifier-naming.PrivateMemberCase, value: camelBack }
33+
- { key: readability-identifier-naming.ParameterCase, value: camelBack }
34+
- { key: readability-identifier-naming.VariableCase, value: camelBack }
35+
- { key: readability-identifier-naming.LocalVariableCase, value: camelBack }
36+
37+
# Enum values, macros, and named constants are ALL_CAPS_WITH_UNDERSCORE.
38+
# (Note: the "named-instance constant" exception — e.g. Protocol::ipv4 — is camelCase
39+
# and clang-tidy cannot express that carve-out; such hits go to naming-exceptions.md.)
40+
- { key: readability-identifier-naming.EnumConstantCase, value: UPPER_CASE }
41+
- { key: readability-identifier-naming.MacroDefinitionCase, value: UPPER_CASE }
42+
- { key: readability-identifier-naming.GlobalConstantCase, value: UPPER_CASE }
43+
- { key: readability-identifier-naming.ConstexprVariableCase, value: UPPER_CASE }
44+
45+
# Namespaces are lowercase
46+
- { key: readability-identifier-naming.NamespaceCase, value: lower_case }

‎doc/architecture/enforcement/agent-review-checklist.md‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
# Agent-Review Checklist (T4 enforcement)
2+
3+
The tier-4 gate from [architectural-requirements.md](../architectural-requirements.md) §*Quality
4+
attributes and enforcement*. It enforces the **semantic** architectural requirements — the ones no
5+
compiler or linter can express — by having an LLM reviewer judge a diff against each item. Run it as
6+
a CI step on every change (and locally before pushing).
7+
8+
## How to run
9+
10+
Input: the change under review (a diff / PR / working tree). For **each** checklist item, output one
11+
of:
12+
13+
- `PASS` — the change plainly complies (or the item doesn't apply).
14+
- `FLAG — <file>:<line> — <one-line reason>` — a clear violation.
15+
- `QUESTION — <file>:<line> — <what to check>` — plausibly a violation but genuinely a judgment call;
16+
escalate to human review (T5), don't block on it.
17+
18+
Ground rules:
19+
20+
1. **Precision over recall.** Only `FLAG` on a clear violation; when unsure, `QUESTION`. A noisy gate
21+
gets ignored.
22+
2. **Judge only what static checks miss.** The compiler, `clang-tidy`, and `check-architecture.sh`
23+
already cover the mechanical rules; you cover *semantics* (intent, logic, duplication).
24+
3. **Respect the ledgers.** Couplings already recorded in
25+
[architecture-exceptions.md](../architecture-exceptions.md) or names in
26+
[naming-exceptions.md](../naming-exceptions.md) are known — don't re-flag them; flag only *new*
27+
deviations, and propose them as new ledger rows.
28+
4. **Scope to the diff.** Review what the change adds or moves, not the whole pre-existing tree.
29+
30+
## Checklist
31+
32+
**[AR-ORG-VIS-SPLIT] Does protocol/model code contain visualization or instrumentation logic?**
33+
FLAG if a protocol/mobility/physical module draws on a canvas, builds a figure, or references a
34+
visualizer beyond emitting a signal. *Not a violation:* emitting a `@signal` that a visualizer
35+
consumes from outside.
36+
37+
**[AR-ORG-KERNEL] Does the change reimplement or patch an OMNeT++ kernel facility inside INET?**
38+
FLAG a private reimplementation of event scheduling, RNG, suspend/resume, breakpoints, or a hand-patch
39+
of kernel internals. *Not a violation:* consuming a kernel API, or a documented shim with a linked
40+
upstream issue.
41+
42+
**[AR-MOD-COMPOSITION] Is new behavior added by composition, or by inheritance / a growing god-module?**
43+
FLAG a new deep inheritance chain, or a simple module/class that accretes several unrelated
44+
responsibilities. *Not a violation:* extending a `*Base` for genuine shared machinery.
45+
46+
**[AR-COM-SOCKETS] Does a new application talk to a transport protocol via raw messages?**
47+
FLAG an app that hand-rolls command/indication messages instead of using `UdpSocket`/`TcpSocket`/peer.
48+
*Not a violation:* a new protocol implementing the socket-facing side.
49+
50+
**[AR-COM-DIRECT] Is a zero-time message standing in for a direct call?**
51+
FLAG `scheduleAt(simTime(), …)` or a zero-delay `send()` used for same-instant, same-node coordination
52+
between sibling submodules. *Not a violation:* a message that advances simulation time or crosses the
53+
medium.
54+
55+
**[AR-OBS-NED-TRUTH] Does prose/code duplicate what a NED declaration owns?**
56+
FLAG doc text that restates parameters/gates/signals/statistics already in NED, or C++ that hardcodes a
57+
value that should be a NED parameter. *Not a violation:* referencing the NED declaration.
58+
59+
**[AR-OBS-INTROSPECTION] Does a new protocol ship its introspection support?**
60+
FLAG a new protocol header/chunk added without a registered serializer, dissector, and printer.
61+
(Partly covered by a completeness test; you catch the "registered but empty/incorrect" case.)
62+
63+
**[AR-CFG-INFER / DRY] Is a derivable fact restated instead of inferred?**
64+
FLAG a manually configured value that the model could infer (e.g. interface counts), or the same
65+
constant/parameter duplicated across sites instead of set once and propagated.
66+
67+
**[AR-CFG-PARAMS] Are new parameters/fields well-formed?**
68+
FLAG a physical-quantity parameter without `@unit`, a parameter without a `default()`, or one field
69+
that means both "user override" and "computed value." *Not a violation:* a dimensionless count.
70+
71+
**[AR-EXT-NOCORE] Does adding a protocol require editing core code?**
72+
FLAG a change that adds a protocol by modifying `common/` or a dispatcher/registry switch, rather than
73+
registering through existing contract/registration points.
74+
75+
**[AR-BUILD-DECLARATIVE] Are build values hardcoded?**
76+
FLAG absolute machine paths, `-march=native`, or per-machine flags baked into build scripts instead of
77+
declared in the build descriptors.
78+
79+
**[AR-QUAL-NAMING] Do new NED/`.msg`/semantic names follow the conventions?**
80+
FLAG names that break [naming-conventions.md](../naming-conventions.md) on the NED/message side that
81+
`clang-tidy` can't see (wrong role suffix, `Msg`/`Message` packet, abbreviated field). Propose new
82+
findings as `naming-exceptions.md` rows.
83+
84+
**[AR-QUAL-LOGGING] Is a programming error logged instead of thrown?**
85+
FLAG a violated invariant / impossible state that is written to the log and execution continues, where
86+
it should `throw`/`ASSERT`/`check_and_cast`. *Not a violation:* informational logging.
87+
88+
**[AR-QUAL-TESTS] Does the change ship with tests matching its nature?**
89+
FLAG new behavior with no accompanying unit/module/statistical/validation test (fingerprints alone
90+
detect *that* behavior changed, not *whether it is correct*).
91+
92+
**[AR-QUAL-DISPLAY] Does a new module have a distinguishing icon?**
93+
FLAG a new module type with no `@display("i=…")`, or one reusing a generic catch-all icon for a
94+
semantically distinct role.
95+
96+
## Output footer
97+
98+
End with a one-line verdict: `REVIEW: n PASS, n FLAG, n QUESTION` and, for any `FLAG`, a suggested
99+
ledger row (`AV-*` or `NV-*`) so the finding lands in the backlog rather than being lost.

‎doc/architecture/enforcement/check-architecture.sh‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
#!/usr/bin/env bash
2+
#
3+
# Starter architecture check for INET — a T3 fitness function (see AR-QUAL-ENFORCED).
4+
# Enforces two dependency-direction rules from architectural-requirements.md over the
5+
# C++ #include graph:
6+
#
7+
# AR-ORG-DOMAINS — the shared 'common' package must not depend on any protocol layer
8+
# (dependencies point protocols -> infrastructure, never the reverse)
9+
# AR-ORG-VIS-SPLIT — model/protocol code must not depend on the visualizer package
10+
#
11+
# Usage (from the INET repository root):
12+
# doc/architecture/enforcement/check-architecture.sh # full check
13+
# doc/architecture/enforcement/check-architecture.sh <SUBTREE> # scope both checks to a subset,
14+
# # e.g. src/inet/common/packet
15+
#
16+
# With no argument, AR-ORG-DOMAINS covers src/inet/common and AR-ORG-VIS-SPLIT covers all of
17+
# src/inet. A SUBTREE argument restricts both checks to that directory — useful for a
18+
# focused, per-package audit report.
19+
#
20+
# Exit status 0 = clean, 1 = violations found. Wire it into CI to make the rule a gate.
21+
# This is intentionally a grep-level starter; a robust version would parse the full
22+
# include graph (e.g. dependency-cruiser / a small Python tool) and check for cycles.
23+
24+
set -uo pipefail
25+
SCOPE="${1:-}"
26+
if [ -n "$SCOPE" ]; then
27+
DOMAIN_SCOPE="$SCOPE"; VIS_SCOPE="$SCOPE"
28+
else
29+
DOMAIN_SCOPE="src/inet/common"; VIS_SCOPE="src/inet"
30+
fi
31+
status=0
32+
33+
for d in "$DOMAIN_SCOPE" "$VIS_SCOPE"; do
34+
if [ ! -d "$d" ]; then
35+
echo "error: '$d' not found (run from the INET repo root)" >&2
36+
exit 2
37+
fi
38+
done
39+
40+
LAYERS='physicallayer|linklayer|networklayer|transportlayer|routing|applications'
41+
42+
# Foundational value types that are depended on framework-wide. These are sanctioned
43+
# exceptions (AS-* in architecture-exceptions.md) — ideally they would live in common/,
44+
# but until they are moved, coupling to them is accepted rather than flagged.
45+
ALLOW='networklayer/contract/ipv4/Ipv4Address\.h'
46+
ALLOW+='|networklayer/contract/ipv6/Ipv6Address\.h'
47+
ALLOW+='|networklayer/common/L3Address(Resolver)?\.h'
48+
ALLOW+='|linklayer/common/MacAddress\.h'
49+
ALLOW+='|linklayer/common/EtherType_m\.h'
50+
ALLOW+='|networklayer/common/IpProtocolId_m\.h'
51+
52+
echo "== AR-ORG-DOMAINS: $DOMAIN_SCOPE must not #include a protocol layer (foundational value types allowlisted) =="
53+
hits=$(grep -rEn "#include \"inet/(${LAYERS})/" "$DOMAIN_SCOPE" 2>/dev/null | grep -vE "$ALLOW")
54+
if [ -n "$hits" ]; then
55+
echo "$hits" | sed 's/^/ VIOLATION: /'
56+
echo " ^ common/ reaches up into a protocol layer — invert the dependency (AR-EXT-ATTACH),"
57+
echo " or record a sanctioned exception in architecture-exceptions.md."
58+
status=1
59+
else
60+
echo " ok"
61+
fi
62+
63+
echo
64+
echo "== AR-ORG-VIS-SPLIT: non-visualizer code must not #include visualizer/ =="
65+
if hits=$(grep -rEln "#include \"inet/visualizer/" "$VIS_SCOPE" 2>/dev/null | grep -v "/visualizer/"); then
66+
echo "$hits" | sed 's/^/ VIOLATION: /'
67+
echo " ^ model/protocol code depends on the visualizer — visualizers must subscribe from outside."
68+
status=1
69+
else
70+
echo " ok"
71+
fi
72+
73+
echo
74+
if [ "$status" -eq 0 ]; then
75+
echo "PASS: architecture checks clean."
76+
else
77+
echo "FAIL: architecture violations found (record permanent exceptions, fix the rest)."
78+
fi
79+
exit "$status"

0 commit comments

Comments
 (0)