diff --git a/docs/build-status.md b/docs/build-status.md index d3dcb31..dd464b6 100644 --- a/docs/build-status.md +++ b/docs/build-status.md @@ -819,3 +819,32 @@ prevent. The pre-existing rate test caught that when I first tried the generous right to. Second half of the same bug: callers treated *any* refusal as terminal, so `TryAllow` now says why — a sender paces through a transient "too fast just now" and still stops dead on a spent budget or an expired grant. Both halves are pinned by regression tests. + +### Findings registry (2026-08-01) +Closes open item 1 of measurement-schema.md §9. A finding code is the stable, machine-readable half +of a result — what a dashboard groups by and what someone greps a year of archived runs for — and +that only holds if a code means exactly one thing forever. Ad-hoc string literals at fifteen call +sites cannot promise that, and by the time the registry was written the failure had already +happened. + +**Two emitters had independently produced `connectivity.downstream_loss` and +`connectivity.loss_downstream` for the same claim**, and nothing anywhere objected. Anyone +aggregating either one would have silently seen half their data. Merged into +`connectivity.loss_downstream`, paired with `loss_upstream` so the two directions read as a set. + +**Two codes were also renamed out of `nat.*`.** `nat.udp_unreachable` is not about NAT — it means +no replies came back — but the prefix determines the category, and the category determines which +verdict light the finding rolls up into (§7.3). A `nat.*` code landing under *connectivity* is not +a naming quibble; it changes which light turns red. Cheap to fix now, a breaking change later. + +Codes are now declared as typed `FindingSpec`s carrying their category and default severity, and +emitters reference the spec instead of retyping the string — so a typo is a compile error and two +call sites cannot disagree about a finding's category. + +`docs/findings-registry.md` is the contract, and a test reads it: it fails when the document and +the registry have codes the other lacks, or when a severity differs. Documentation that drifts from +its implementation is worse than none, because it still looks authoritative. The check scopes +itself to table rows, so the prose can keep explaining which codes were retired and why. + +Six tests: uniqueness, declared-vs-listed, prefix↔category agreement, naming convention, a +word-order-anagram check (the shape the duplication actually took), and the document agreement. diff --git a/docs/findings-registry.md b/docs/findings-registry.md new file mode 100644 index 0000000..2276212 --- /dev/null +++ b/docs/findings-registry.md @@ -0,0 +1,91 @@ + + +# Echolot findings registry + +Closes open item 1 of `measurement-schema.md` §9. + +A **finding code** is the stable, machine-readable half of a result. The prose around it changes +freely; the code is what a dashboard groups by, what a diff between two runs keys on, and what +someone greps a year of archived runs for. That only works if a code means exactly one thing, +forever. + +This document is the contract. It is kept in step with +`echolot-app/core-measurement/.../FindingRegistry.kt` by a test that fails when either side has a +code the other does not — a registry that drifts from its documentation is worse than none, +because it looks authoritative. + +## Rules + +1. **The prefix determines the category**, and the category determines which verdict light the + finding rolls up into (§7.3). A `nat.*` code appearing under *connectivity* is not a naming + quibble; it changes which light turns red. Two codes were renamed from `nat.*` to + `connectivity.*` for exactly this reason. +2. **One code per concept.** Two emitters independently produced `connectivity.downstream_loss` + and `connectivity.loss_downstream` for the same claim before this registry existed. Anyone + aggregating either would have silently seen half their data. +3. **Codes are declared, not typed.** Emitters reference a `FindingSpec`, so a typo is a compile + error and no two call sites can disagree about a finding's category or default severity. +4. **Severity in the registry is the default.** An emitter may escalate for a specific run; it may + not quietly reclassify the finding in general. +5. **Say what is ruled out**, where that is the useful half. "Loss upstream" is worth far more + when it also states that the return path is clean, because that halves where to look next. +6. **Renaming a code is a breaking change** once runs are archived at scale. Before 1.0 it is + cheap; after, it needs an alias and a deprecation window. + +## Registry + +### connectivity + +| code | severity | means | rules out | +|---|---|---|---| +| `connectivity.udp_unreachable` | high | No UDP echo replies came back from the server at all. | — | +| `connectivity.udp_unreachable_upstream` | high | The server received none of the probes, so traffic is dropped on the way out. | The return path: nothing arrived to be replied to. | +| `connectivity.udp_loss` | medium | A large fraction of round-trip probes were lost, direction unknown. | — | +| `connectivity.loss_upstream` | medium | Probes were lost on the way to the server. | The return path: replies came back for everything that arrived. | +| `connectivity.loss_downstream` | medium | Packets were lost on the way back from the server. | The outbound path: the server received what it was answering. | +| `connectivity.downstream_blocked` | high | Server-initiated packets never arrive, although round trips work. | Basic reachability: the path forwards replies, just not unsolicited traffic. | +| `connectivity.downstream_reorder` | low | Downstream packets arrive in a different order than they were sent. | — | +| `connectivity.captive_portal` | high | A captive portal is intercepting connectivity checks. | — | +| `connectivity.no_internet` | high | Android's own connectivity checks fail on this network. | — | + +### mtu + +| code | severity | means | rules out | +|---|---|---|---| +| `mtu.reduced_downstream` | low | The downstream path MTU is below the usual 1500 bytes. | — | +| `mtu.downstream_blackhole` | medium | Datagrams above the path MTU are dropped downstream, fragmented or not. | — | +| `mtu.fragments_blocked` | medium | IP fragments do not reach this device even when sent in order. | — | +| `mtu.fragment_reorder_sensitive` | low | Fragments are delivered in order but dropped when reordered or delayed. | Fragmentation itself: in-order fragments arrive fine. | + +### nat + +| code | severity | means | rules out | +|---|---|---|---| +| `nat.udp_rebinding` | medium | A NAT remapped the UDP source port mid-flow. | — | +| `nat.symmetric` | medium | The NAT assigns a different external port per destination. | — | + +### perf + +| code | severity | means | rules out | +|---|---|---|---| +| `perf.throughput_no_delivery` | high | No throughput traffic arrived, although the server sent it. | — | +| `perf.throughput_below_offered` | low | Less throughput arrived than the server sent for the whole run. | — | + +### dns + +| code | severity | means | rules out | +|---|---|---|---| +| `dns.answer_rewritten` | high | A resolver returned an answer that differs from the authoritative record. | — | +| `dns.authoritative_unreachable` | medium | The canary zone's authoritative server could not be reached. | — | + +## Adding a finding + +1. Add a `FindingSpec` to `FindingRegistry`, and to its `all` list. +2. Add the row here, under the section its prefix names. +3. Emit it with `finding(FindingRegistry.YOUR_CODE, …)`. + +The registry test checks 1 and 2 agree, that every prefix maps to the category it claims, and that +no two entries share a code. diff --git a/docs/measurement-schema.md b/docs/measurement-schema.md index a75ffb3..c38ce3b 100644 --- a/docs/measurement-schema.md +++ b/docs/measurement-schema.md @@ -279,7 +279,8 @@ Free-text fields (`notes`, `error.detail`, dump excerpts from Shizuku parsers) c ## 9. Open items -1. Findings registry document — start alongside the first implemented tests. +1. ~~Findings registry document~~ — done: `findings-registry.md`, kept in step with + `FindingRegistry.kt` by a test that fails when the two disagree. 2. Whether Shizuku raw-dump excerpts (dumpsys/ip output) are embedded in `evidence` verbatim (auditable, but large and hard to anonymize) or parsed-only with an optional "attach raw dumps" toggle. Proposal: toggle, default on for local archive, default off for export. 3. Peer-mode documents: each device produces its own run; the coordinator embeds the peer's findings summary and cross-references by `run.id`. Full merge format deferred. 4. Size guardrails: soft cap 20 MB uncompressed per run; trains beyond that downsample evidence (keep aggregates + first/last N + all anomalies) and record `"evidence_truncated": true`. diff --git a/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/DownstreamMeasurement.kt b/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/DownstreamMeasurement.kt index 226cdfd..bf9e36d 100644 --- a/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/DownstreamMeasurement.kt +++ b/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/DownstreamMeasurement.kt @@ -118,7 +118,7 @@ class DownstreamMeasurement(private val ids: IdSource) { if (!inOrder) { findings.add( finding( - "mtu.fragments_blocked", Category.MTU, Severity.MEDIUM, testId, + FindingRegistry.FRAGMENTS_BLOCKED, testId, "IP fragments do not reach this device", "A fragmented datagram sent in the normal order never arrived. Anything that " + "relies on fragmentation — large DNS answers over UDP, some VPN traffic — " + @@ -133,7 +133,7 @@ class DownstreamMeasurement(private val ids: IdSource) { }.joinToString(" or ") findings.add( finding( - "mtu.fragment_reorder_sensitive", Category.MTU, Severity.LOW, testId, + FindingRegistry.FRAGMENT_REORDER_SENSITIVE, testId, "Fragments are dropped when they arrive $which", "In-order fragments are delivered, but the same datagram sent $which is not. " + "Something on the path only reassembles when the first fragment (the one " + @@ -198,7 +198,7 @@ class DownstreamMeasurement(private val ids: IdSource) { if (ipMtu < 1500) { findings.add( finding( - "mtu.reduced_downstream", Category.MTU, Severity.LOW, df.test.id, + FindingRegistry.MTU_REDUCED_DOWNSTREAM, df.test.id, "Downstream path MTU is $ipMtu bytes, below 1500", "The largest datagram that reached this device without fragmenting was " + "$pathMtu bytes of payload ($ipMtu on the wire). Tunnels (PPPoE, VPN, " + @@ -213,7 +213,7 @@ class DownstreamMeasurement(private val ids: IdSource) { if (fragLargest <= pathMtu && sizes.any { it > pathMtu }) { findings.add( finding( - "mtu.downstream_blackhole", Category.MTU, Severity.MEDIUM, frag.test.id, + FindingRegistry.MTU_DOWNSTREAM_BLACKHOLE, frag.test.id, "Datagrams above $pathMtu bytes are dropped downstream, fragmented or not", "Nothing larger than $pathMtu bytes arrived, even when the network was " + "free to fragment it. Traffic that relies on large responses will " + @@ -226,7 +226,7 @@ class DownstreamMeasurement(private val ids: IdSource) { if (train.received == 0) { findings.add( finding( - "connectivity.downstream_blocked", Category.CONNECTIVITY, Severity.HIGH, train.test.id, + FindingRegistry.DOWNSTREAM_BLOCKED, train.test.id, "No server-initiated packets arrived", "The server sent ${train.sent} packets toward this device and none arrived, " + "while the round-trip echo worked. Something on the path forwards replies " + @@ -236,7 +236,7 @@ class DownstreamMeasurement(private val ids: IdSource) { } else if (train.lossPct >= 5.0) { findings.add( finding( - "connectivity.downstream_loss", Category.CONNECTIVITY, Severity.MEDIUM, train.test.id, + FindingRegistry.LOSS_DOWNSTREAM, train.test.id, "Downstream loss of ${round1(train.lossPct)}%", "${train.sent - train.received} of ${train.sent} packets sent toward this " + "device were lost. Downstream loss is invisible to a round-trip test, " + @@ -247,7 +247,7 @@ class DownstreamMeasurement(private val ids: IdSource) { if (train.reordered > 0) { findings.add( finding( - "connectivity.downstream_reorder", Category.CONNECTIVITY, Severity.LOW, train.test.id, + FindingRegistry.DOWNSTREAM_REORDER, train.test.id, "${train.reordered} downstream packet(s) arrived out of order", "Packets arrived in a different order than they were sent. Usually per-packet " + "load balancing across links; harmless for most traffic, not for all of it.", @@ -414,9 +414,17 @@ class DownstreamMeasurement(private val ids: IdSource) { // ---- helpers ---------------------------------------------------------------------- - private fun finding(code: String, cat: Category, sev: Severity, testId: String, title: String, desc: String) = + /** + * Builds a finding from a registry entry, which supplies the code, category and severity. + * + * Taking a [FindingSpec] rather than three loose values is the point: a typo becomes a + * compile error, and two call sites cannot disagree about which category a finding belongs + * to - a disagreement that would split one fault across two verdict lights. + */ + private fun finding(spec: FindingSpec, testId: String, title: String, desc: String) = Finding( - id = ids.uuid(), code = code, category = cat, severity = sev, confidence = Confidence.HIGH, + id = ids.uuid(), code = spec.code, category = spec.category, severity = spec.severity, + confidence = Confidence.HIGH, title = title, description = desc, evidenceRefs = listOf(EvidenceRef(testId)), ) diff --git a/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ServerMeasurement.kt b/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ServerMeasurement.kt index 41f023e..7284f5e 100644 --- a/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ServerMeasurement.kt +++ b/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ServerMeasurement.kt @@ -208,11 +208,11 @@ class ServerMeasurement( val findings = ArrayList() if (received == 0) { - findings.add(finding("nat.udp_unreachable", Category.CONNECTIVITY, Severity.HIGH, testId, + findings.add(finding(FindingRegistry.UDP_UNREACHABLE, testId, "No UDP echo replies from the server", "Every ECHO probe to the server's UDP data plane was lost — the path blocks or drops the session's UDP traffic.")) } else if (lossPct >= 20.0) { - findings.add(finding("connectivity.udp_loss", Category.CONNECTIVITY, Severity.MEDIUM, testId, + findings.add(finding(FindingRegistry.UDP_LOSS, testId, "High UDP loss to the server (${round1(lossPct)}%)", "A large fraction of ECHO probes were lost, indicating an unreliable UDP path.")) } @@ -220,14 +220,14 @@ class ServerMeasurement( directional?.let { d -> when { d.noneReachedServer && received == 0 -> findings.add( - finding("nat.udp_unreachable_upstream", Category.CONNECTIVITY, Severity.HIGH, testId, + finding(FindingRegistry.UDP_UNREACHABLE_UPSTREAM, testId, "Nothing reached the server", "The server received none of the ${d.sent} probes, so the traffic is being " + "dropped on the way out, not on the way back. A firewall or NAT on " + "this side of the path is the place to look."), ) d.lossUpstreamPct >= 2.0 -> findings.add( - finding("connectivity.loss_upstream", Category.CONNECTIVITY, Severity.MEDIUM, testId, + finding(FindingRegistry.LOSS_UPSTREAM, testId, "${d.lossUpstreamPct} % of probes were lost on the way to the server", "${d.lostUpstream} of ${d.sent} probes never reached the server. The " + "return path is not implicated: replies came back for everything that " + @@ -236,7 +236,7 @@ class ServerMeasurement( } if (d.lossDownstreamPct >= 2.0) { findings.add( - finding("connectivity.loss_downstream", Category.CONNECTIVITY, Severity.MEDIUM, testId, + finding(FindingRegistry.LOSS_DOWNSTREAM, testId, "${d.lossDownstreamPct} % of replies were lost on the way back", "The server received ${d.seenByServer} probes and answered them, but " + "${d.lostDownstream} of those replies never arrived. The outbound path " + @@ -246,7 +246,7 @@ class ServerMeasurement( } if (natRebinding) { - findings.add(finding("nat.udp_rebinding", Category.NAT, Severity.MEDIUM, testId, + findings.add(finding(FindingRegistry.NAT_UDP_REBINDING, testId, "NAT remapped the UDP source port mid-flow", "The server observed more than one source port for this session (${observedPorts.joinToString()}), i.e. a NAT with a short UDP mapping or per-packet remapping.")) } @@ -276,9 +276,17 @@ class ServerMeasurement( } } - private fun finding(code: String, cat: Category, sev: Severity, testId: String, title: String, desc: String) = + /** + * Builds a finding from a registry entry, which supplies the code, category and severity. + * + * Taking a [FindingSpec] rather than three loose values is the point: a typo becomes a + * compile error, and two call sites cannot disagree about which category a finding belongs + * to - a disagreement that would split one fault across two verdict lights. + */ + private fun finding(spec: FindingSpec, testId: String, title: String, desc: String) = Finding( - id = ids.uuid(), code = code, category = cat, severity = sev, confidence = Confidence.HIGH, + id = ids.uuid(), code = spec.code, category = spec.category, severity = spec.severity, + confidence = Confidence.HIGH, title = title, description = desc, evidenceRefs = listOf(EvidenceRef(testId)), ) diff --git a/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ThroughputMeasurement.kt b/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ThroughputMeasurement.kt index e6b8b83..3139cc3 100644 --- a/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ThroughputMeasurement.kt +++ b/echolot-app/core-engine/src/main/kotlin/app/echo_lot/engine/ThroughputMeasurement.kt @@ -120,7 +120,7 @@ class ThroughputMeasurement(private val ids: IdSource) { sender == null -> Unit // no sender report: nothing can be concluded, so nothing is received.isEmpty() -> findings.add( finding( - "perf.throughput_no_delivery", Category.PERFORMANCE, Severity.HIGH, testId, + FindingRegistry.THROUGHPUT_NO_DELIVERY, testId, "No throughput traffic arrived", "The server sent ${sender.packets} packets and none arrived. This is a " + "connectivity fault rather than a slow link.", @@ -128,7 +128,7 @@ class ThroughputMeasurement(private val ids: IdSource) { ) networkLimited -> findings.add( finding( - "perf.throughput_below_offered", Category.PERFORMANCE, Severity.LOW, testId, + FindingRegistry.THROUGHPUT_BELOW_OFFERED, testId, "Downstream throughput ${receivedKbps / 1000} Mbit/s, below the " + "${sender.kbps / 1000} Mbit/s offered", "The server sent at ${sender.kbps / 1000} Mbit/s for the full run and " + @@ -169,9 +169,17 @@ class ThroughputMeasurement(private val ids: IdSource) { private fun parseInt(body: String?, key: String): Int? = body?.let { Regex("\"$key\"\\s*:\\s*(-?\\d+)").find(it)?.groupValues?.get(1)?.toIntOrNull() } - private fun finding(code: String, cat: Category, sev: Severity, testId: String, title: String, desc: String) = + /** + * Builds a finding from a registry entry, which supplies the code, category and severity. + * + * Taking a [FindingSpec] rather than three loose values is the point: a typo becomes a + * compile error, and two call sites cannot disagree about which category a finding belongs + * to - a disagreement that would split one fault across two verdict lights. + */ + private fun finding(spec: FindingSpec, testId: String, title: String, desc: String) = Finding( - id = ids.uuid(), code = code, category = cat, severity = sev, confidence = Confidence.HIGH, + id = ids.uuid(), code = spec.code, category = spec.category, severity = spec.severity, + confidence = Confidence.HIGH, title = title, description = desc, evidenceRefs = listOf(EvidenceRef(testId)), ) diff --git a/echolot-app/core-measurement/src/main/kotlin/app/echo_lot/measurement/FindingRegistry.kt b/echolot-app/core-measurement/src/main/kotlin/app/echo_lot/measurement/FindingRegistry.kt new file mode 100644 index 0000000..b649cc6 --- /dev/null +++ b/echolot-app/core-measurement/src/main/kotlin/app/echo_lot/measurement/FindingRegistry.kt @@ -0,0 +1,175 @@ +// SPDX-FileCopyrightText: 2026 Echolot contributors +// SPDX-License-Identifier: GPL-3.0-or-later + +package app.echo_lot.measurement + +/** + * The registry of finding codes (measurement-schema.md §9, open item 1). + * + * A finding code is the stable, machine-readable half of a result: the prose changes, the code is + * what a dashboard groups by and what someone greps a year of archived runs for. That only holds + * if a code means exactly one thing forever — which is not something ad-hoc string literals at + * fifteen call sites can promise. + * + * The failure this exists to prevent had already happened by the time it was written. Two + * independently-added emitters produced `connectivity.downstream_loss` and + * `connectivity.loss_downstream` for the same concept, and nothing anywhere objected. Anyone + * aggregating either one would have silently seen half their data. + * + * So codes are declared here as typed specs, each carrying its category and default severity, and + * emitters reference the spec rather than retyping the string. That makes a typo a compile error, + * and makes it impossible for two call sites to disagree about which category a finding belongs + * to — a disagreement that would otherwise split one fault across two verdict lights. + */ +data class FindingSpec( + val code: String, + val category: Category, + /** Severity when nothing about the specific run argues otherwise; emitters may escalate. */ + val severity: Severity, + /** One line: what this finding asserts. Present tense, no hedging. */ + val meaning: String, + /** + * What the finding rules *out*, where that is the useful half. "Loss upstream" is worth much + * more when it also says the return path is fine, because that halves where to look next. + */ + val rulesOut: String? = null, +) + +object FindingRegistry { + + // ---- connectivity ---------------------------------------------------------------- + + // Renamed from nat.* before anything shipped: neither of these is about NAT, and the + // prefix is what decides which category - and therefore which verdict light - a finding + // rolls up into. A nat.* code landing under connectivity would be a permanent puzzle. + val UDP_UNREACHABLE = FindingSpec( + "connectivity.udp_unreachable", Category.CONNECTIVITY, Severity.HIGH, + "No UDP echo replies came back from the server at all.", + ) + + val UDP_UNREACHABLE_UPSTREAM = FindingSpec( + "connectivity.udp_unreachable_upstream", Category.CONNECTIVITY, Severity.HIGH, + "The server received none of the probes, so traffic is dropped on the way out.", + rulesOut = "The return path: nothing arrived to be replied to.", + ) + + val UDP_LOSS = FindingSpec( + "connectivity.udp_loss", Category.CONNECTIVITY, Severity.MEDIUM, + "A large fraction of round-trip probes were lost, direction unknown.", + ) + + val LOSS_UPSTREAM = FindingSpec( + "connectivity.loss_upstream", Category.CONNECTIVITY, Severity.MEDIUM, + "Probes were lost on the way to the server.", + rulesOut = "The return path: replies came back for everything that arrived.", + ) + + /** + * The single code for "lost on the return path", whichever measurement found it. + * + * Two emitters had independently invented `connectivity.downstream_loss` and + * `connectivity.loss_downstream` for this, and nothing objected. Anyone aggregating either + * one would have silently seen half their data. Paired with [LOSS_UPSTREAM] so the two + * directions read as a set. + */ + val LOSS_DOWNSTREAM = FindingSpec( + "connectivity.loss_downstream", Category.CONNECTIVITY, Severity.MEDIUM, + "Packets were lost on the way back from the server.", + rulesOut = "The outbound path: the server received what it was answering.", + ) + + val DOWNSTREAM_BLOCKED = FindingSpec( + "connectivity.downstream_blocked", Category.CONNECTIVITY, Severity.HIGH, + "Server-initiated packets never arrive, although round trips work.", + rulesOut = "Basic reachability: the path forwards replies, just not unsolicited traffic.", + ) + + val DOWNSTREAM_REORDER = FindingSpec( + "connectivity.downstream_reorder", Category.CONNECTIVITY, Severity.LOW, + "Downstream packets arrive in a different order than they were sent.", + ) + + val CAPTIVE_PORTAL = FindingSpec( + "connectivity.captive_portal", Category.CONNECTIVITY, Severity.HIGH, + "A captive portal is intercepting connectivity checks.", + ) + + val NO_INTERNET = FindingSpec( + "connectivity.no_internet", Category.CONNECTIVITY, Severity.HIGH, + "Android's own connectivity checks fail on this network.", + ) + + // ---- mtu ------------------------------------------------------------------------- + + val MTU_REDUCED_DOWNSTREAM = FindingSpec( + "mtu.reduced_downstream", Category.MTU, Severity.LOW, + "The downstream path MTU is below the usual 1500 bytes.", + ) + + val MTU_DOWNSTREAM_BLACKHOLE = FindingSpec( + "mtu.downstream_blackhole", Category.MTU, Severity.MEDIUM, + "Datagrams above the path MTU are dropped downstream, fragmented or not.", + ) + + val FRAGMENTS_BLOCKED = FindingSpec( + "mtu.fragments_blocked", Category.MTU, Severity.MEDIUM, + "IP fragments do not reach this device even when sent in order.", + ) + + val FRAGMENT_REORDER_SENSITIVE = FindingSpec( + "mtu.fragment_reorder_sensitive", Category.MTU, Severity.LOW, + "Fragments are delivered in order but dropped when reordered or delayed.", + rulesOut = "Fragmentation itself: in-order fragments arrive fine.", + ) + + // ---- nat ------------------------------------------------------------------------- + + val NAT_UDP_REBINDING = FindingSpec( + "nat.udp_rebinding", Category.NAT, Severity.MEDIUM, + "A NAT remapped the UDP source port mid-flow.", + ) + + val NAT_SYMMETRIC = FindingSpec( + "nat.symmetric", Category.NAT, Severity.MEDIUM, + "The NAT assigns a different external port per destination.", + ) + + // ---- perf ------------------------------------------------------------------------ + + val THROUGHPUT_NO_DELIVERY = FindingSpec( + "perf.throughput_no_delivery", Category.PERFORMANCE, Severity.HIGH, + "No throughput traffic arrived, although the server sent it.", + ) + + val THROUGHPUT_BELOW_OFFERED = FindingSpec( + "perf.throughput_below_offered", Category.PERFORMANCE, Severity.LOW, + "Less throughput arrived than the server sent for the whole run.", + ) + + // ---- dns ------------------------------------------------------------------------- + + val DNS_ANSWER_REWRITTEN = FindingSpec( + "dns.answer_rewritten", Category.DNS, Severity.HIGH, + "A resolver returned an answer that differs from the authoritative record.", + ) + + val DNS_AUTHORITATIVE_UNREACHABLE = FindingSpec( + "dns.authoritative_unreachable", Category.DNS, Severity.MEDIUM, + "The canary zone's authoritative server could not be reached.", + ) + + /** Every registered finding, in declaration order. */ + val all: List = listOf( + UDP_UNREACHABLE, UDP_UNREACHABLE_UPSTREAM, UDP_LOSS, LOSS_UPSTREAM, LOSS_DOWNSTREAM, + DOWNSTREAM_BLOCKED, DOWNSTREAM_REORDER, CAPTIVE_PORTAL, NO_INTERNET, + MTU_REDUCED_DOWNSTREAM, MTU_DOWNSTREAM_BLACKHOLE, FRAGMENTS_BLOCKED, + FRAGMENT_REORDER_SENSITIVE, + NAT_UDP_REBINDING, NAT_SYMMETRIC, + THROUGHPUT_NO_DELIVERY, THROUGHPUT_BELOW_OFFERED, + DNS_ANSWER_REWRITTEN, DNS_AUTHORITATIVE_UNREACHABLE, + ) + + private val byCode: Map = all.associateBy { it.code } + + fun byCode(code: String): FindingSpec? = byCode[code] +} diff --git a/echolot-app/core-measurement/src/test/kotlin/app/echo_lot/measurement/FindingRegistryTest.kt b/echolot-app/core-measurement/src/test/kotlin/app/echo_lot/measurement/FindingRegistryTest.kt new file mode 100644 index 0000000..2370f44 --- /dev/null +++ b/echolot-app/core-measurement/src/test/kotlin/app/echo_lot/measurement/FindingRegistryTest.kt @@ -0,0 +1,133 @@ +// SPDX-FileCopyrightText: 2026 Echolot contributors +// SPDX-License-Identifier: GPL-3.0-or-later + +package app.echo_lot.measurement + +import java.io.File +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue +import kotlin.test.fail + +/** + * Keeps the finding registry honest. + * + * The interesting test is the last one: it reads `docs/findings-registry.md` and fails when the + * document and the code disagree. Documentation that drifts from its implementation is worse than + * none, because it still looks authoritative — and a finding registry is precisely the artifact + * other people build tooling against. + */ +class FindingRegistryTest { + + @Test + fun codesAreUnique() { + val dupes = FindingRegistry.all.groupBy { it.code }.filterValues { it.size > 1 }.keys + assertTrue(dupes.isEmpty(), "duplicate finding codes: $dupes") + } + + @Test + fun everyDeclaredSpecIsInTheAllList() { + // Reflection over the object's properties: a spec that is declared but left out of `all` + // is invisible to the doc check and to any consumer enumerating the registry. + val declared = FindingRegistry::class.java.declaredMethods + .filter { it.parameterCount == 0 && it.returnType == FindingSpec::class.java } + .mapNotNull { runCatching { it.invoke(FindingRegistry) as FindingSpec }.getOrNull() } + .map { it.code } + .toSet() + val listed = FindingRegistry.all.map { it.code }.toSet() + assertEquals(declared, listed, "declared specs and the `all` list disagree") + } + + // The prefix decides the category, and the category decides which verdict light the finding + // rolls up into. A code whose prefix disagrees with its category silently moves a fault to a + // different light — the exact bug that got two codes renamed out of nat.*. + @Test + fun everyPrefixMatchesItsCategory() { + for (spec in FindingRegistry.all) { + val fromPrefix = TestType.category(spec.code) + assertEquals( + fromPrefix, spec.category, + "${spec.code} is declared as ${spec.category} but its prefix maps to $fromPrefix", + ) + } + } + + @Test + fun codesFollowTheNamingConvention() { + val shape = Regex("^[a-z0-9]+\\.[a-z0-9_]+$") + for (spec in FindingRegistry.all) { + assertTrue(shape.matches(spec.code), "malformed code: ${spec.code}") + assertTrue(spec.meaning.isNotBlank(), "${spec.code} has no meaning") + assertTrue( + spec.meaning.trimEnd().endsWith("."), + "${spec.code}'s meaning should be a sentence: '${spec.meaning}'", + ) + } + } + + // Two near-identical codes are how one fault ends up split across two dashboards. This is a + // blunt check — it will not catch every synonym — but it catches the shape that already + // happened: the same words in a different order. + @Test + fun noTwoCodesAreAnagramsOfEachOther() { + val normalised = FindingRegistry.all.associate { spec -> + spec.code to spec.code.substringAfter('.').split('_').sorted().joinToString("_") + } + val clashes = normalised.entries.groupBy { it.value }.filterValues { it.size > 1 } + if (clashes.isNotEmpty()) { + fail("codes differing only in word order: ${clashes.values.map { g -> g.map { it.key } }}") + } + } + + @Test + fun theDocumentAndTheRegistryAgree() { + val doc = findDoc() ?: run { + println("findings-registry.md not found from ${File(".").absolutePath} — skipping") + return + } + val text = doc.readText() + + // Only table rows count as "documented". Prose may legitimately mention a code that no + // longer exists — the rules section explains why two were merged — and treating that as + // a registry entry would force the document to forget its own history. + val documented = text.lines() + .filter { it.trimStart().startsWith("|") } + .flatMap { row -> Regex("`([a-z0-9]+\\.[a-z0-9_]+)`").findAll(row).map { it.groupValues[1] } } + .toSet() + val registered = FindingRegistry.all.map { it.code }.toSet() + + val missingFromDoc = registered - documented + val missingFromCode = documented - registered + assertTrue( + missingFromDoc.isEmpty(), + "these codes exist in FindingRegistry but not in docs/findings-registry.md: $missingFromDoc", + ) + assertTrue( + missingFromCode.isEmpty(), + "docs/findings-registry.md documents codes that no longer exist: $missingFromCode", + ) + + // And the severities must match, or the document is describing a different system. + for (spec in FindingRegistry.all) { + val row = text.lines().firstOrNull { + it.trimStart().startsWith("|") && it.contains("`${spec.code}`") + } ?: continue + val severity = spec.severity.name.lowercase() + assertTrue( + row.contains("| $severity |"), + "${spec.code} is ${severity} in code but the doc row says otherwise: $row", + ) + } + } + + /** Walks up from the test's working directory to find the repo's docs/ folder. */ + private fun findDoc(): File? { + var dir: File? = File(".").absoluteFile + repeat(6) { + val candidate = File(dir, "docs/findings-registry.md") + if (candidate.isFile) return candidate + dir = dir?.parentFile + } + return null + } +}