delete TECH_DEBT.md: all 71 findings closed by milestones 15-19

2026-08-08 12:39:58 +02:00
parent cdacc560b7
commit 004092a7ef
1 changed files with 0 additions and 278 deletions
@@ -1,278 +0,0 @@
 # nxdns Tech-Debt Audit — Final Report
 All findings below were adversarially verified against the code at HEAD (1ff727f). Raw finding count was 77; 6 were merged into other findings that describe the same underlying debt, leaving **71 distinct findings: 1 high, 34 medium, 36 low**.
 ## Overview
 Dominant themes, in order of threat:
 1. **The verification pipeline itself is compromised.** CI watches a branch nobody pushes to, the main test binary aborts on every green run while printing `failed command`, a stale frontend bundle has already shipped a broken page once with no guard added since, the live-network suite never runs automatically, and the test aggregator is a hand-maintained list. This is the same silent-green failure class that shipped the 35f2324 fetcher bug.
 2. **Copy-paste infrastructure with drift already underway.** The connection-server lifecycle (the trickiest concurrency code in the repo) exists in four hand-synced copies; the repo memory-safety choreography in ~18; the web CRUD shell in 7 handler files; Tailwind class constants in 12+ frontend files. Several copies have already diverged.
 3. **Silent failures and swallowed errors.** Cancellation converted to bogus statuses, transport causes discarded before classification, an oversized cookie header producing an undiagnosable login loop, a paginated log view that silently develops row gaps.
 4. **Operator-facing contract drift.** Config keys, docs, and the validator promise behavior the runtime does not deliver (per-query timeout that is per-attempt, hostname `tls://` upstreams that validate clean but can never connect, an upstream-management UI that was specced and never built).
 5. **Observability debt.** Counters written and never exported, sibling listeners exporting the same metric name with different semantics, warn-level logging for routine peer behavior in violation of the module's own ruling.
 ---
 ## Theme 1: The verification pipeline is compromised (11 findings)
 > **Status: all 11 findings closed by milestone 15 (commit 9f8a5cd,
 > 2026-08-07).** Two corrections to the findings themselves, discovered
 > during implementation: the teardown-abort diagnosis was wrong — no binary
 > aborts under `zig build`; the `failed command` label is a zig 0.16.0
 > build-runner display artifact (root cause recorded in
 > specs/milestone-15.md ruling 3) — and the http_util "decoded segment
 > never gains a `/`" invariant was false (`%2F` decodes to a literal `/`);
 > the count-based invariant that replaced it is recorded in ruling 6c.
 > The aarch64-under-qemu acceptance line is environment-blocked on this
 > host (no `qemu-aarch64`); the aarch64 suite cross-compiles clean.
 The repo's history proves this theme's stakes: commit 35f2324 fixed a process-killing bug that 1284 green hermetic tests hid. Several of the guards that should prevent a recurrence are absent or inert.
 - **[medium] .gitea/workflows/ci.yml:5 — CI triggers only on `main` while work happens on `master`.**
  `branch.master.merge` confirms day-to-day pushes go to `origin/master`, which the workflow does not watch; local master is 3 commits ahead of both remote heads, so milestone 13+ has never run through CI. Every gate (tests, aarch64/qemu, frontend, size budgets, docker smoke) is currently inert for real work. The fix is already specified in specs/milestone-14.md §7 (make master canonical, switch ci.yml, delete origin/main); implement that spec rather than re-deciding.
 - **[medium] build.zig:48 — main test binary SIGABRTs at teardown; every green run prints `failed command`.** *(merged: build.zig:68, same defect from the output-signal angle)*
  Reproduced at HEAD: `zig build test` exits 0 but the mbedTLS-linked binary aborts under `--listen=-` after all 1242 tests pass (exit 134 when run directly with `--listen`; clean in stdio mode, pointing at an interaction with zig's IPC runner rather than a production shutdown defect). Cost: the one string that should mean failure appears on every success, training humans, CI log scans, and agents to ignore failure text. Fix: root-cause the abort (debugger under `--listen`, bisect the mbedTLS threading macros); failing that, document the artifact in AGENTS.md/build.zig so nobody "fixes" it by loosening a check, and make any future non-zero test-binary exit fail the build.
 - **[medium] build.zig:24 — no freshness guard on `-Dweb-dist` despite a stale bundle having already shipped a crashing settings page.**
  docs/explanation/performance-and-testing.md:159 admits the hole is still open; CI is safe only by incidental job ordering. Milestone-14 specs a hard failure for the adjacent placeholder-embed case but not this one. Fix: a build-time staleness check (hash of web/src + lockfile recorded at frontend build time, or refuse `web/dist` older than the newest file under web/src).
 - **[medium] src/tests.zig:3 — hand-maintained 115-entry test import list with no completeness guard.**
  Zig collects tests only from the root module; a forgotten import means a file's tests silently never run (verified empirically: an unreferenced import drops its tests while the run stays green). The ~14 standalone integration/live/drift suites are reachable only through this list. The list is complete today. Fix: walk src/ in build.zig (or generate the aggregator via WriteFiles, as bench_stage already does) and fail when a file is unreachable from tests.zig.
 - **[medium] .gitea/workflows/live-tls.yml:7 — the only real-network tests run on manual dispatch only.**
  The `-Dlive` suite (real public DoT/DoH resolvers) has no schedule; both failure modes it exists to catch have already happened once here (35f2324; a live DoT test caught real upstream cert drift in milestone 4). Non-blocking on push is correct; never running automatically is the gap. Fix: add a weekly `schedule:` cron — equally non-blocking, so it does not conflict with the milestone-1 ruling.
 - **[medium] src/filter/filter_integration_test.zig:414 — the multi-read HTTP body path through the fetcher is still never driven end to end.**
  This is the exact seam where 35f2324 lived; the fix added unit tests that pre-buffer readers into the fatal state, but the loopback fixture still answers everything in one `request.respond` of a ~150-byte body. Fix: a fixture route that writes the body in several flushed chunks, or a body larger than the fixture's 8192-byte write buffer.
 - **[medium] src/dns/edns.zig:190 — stripEcs is the only attacker-facing packet-rewriting entry point absent from the fuzz targets.**
  It runs on the raw client query on the default `ecs_mode = .strip` path and does the module's most delicate work (rebuild around OPT, patch RDLENGTH in place). The fuzz file's stated exclusion covers only ResponseBuilder. Verification found no latent bug in the patch arithmetic, so this stays a coverage gap. Fix: a fuzz target asserting output re-parses, contains no ECS option, and preserves everything else — the input derivation already exists in `parseTarget`.
 - **[medium] src/docs_drift_test.zig:51 — the CLI drift guard hardcodes the subcommand list instead of deriving it.**
  The sibling guards derive their needles from the source of truth (routes.table, model.toSettings); this one keeps a parallel copy, so adding `nxdns stats` leaves the test green with the new command missing from docs — the exact drift it exists to catch. Fix: a public command table in cli.zig (or derive from `@typeInfo(cli.Command)`) that dispatcher, usage text, and the guard all iterate.
 - **[low] src/platform/logging.zig:610 — rotation failure paths are declared untested and cover the module's trickiest state machine.**
  The exactly-one-sink-error contract spans four functions and is codified in the spec but trusted, not asserted; the only failure path with a test is a failed open. Fix: an injection seam on the deleteLocked/renameLocked wrappers so the pending-rotation recovery can be asserted.
 - **[low] tests/fuzz/blocklist_fuzz.zig:122 — fuzzing stops at the line parsers; the compiler's streaming takeDelimiter/StreamTooLong/discard loop is never fuzzed.**
  The loop's own comment notes that omitting the discard loops forever, and its EndOfStream-during-discard branch has no test at all. Note: the finding's fix sketch is wrong — compiler.zig imports ../dns/, so a src/filter-rooted fuzz module fails with ImportOutsideModulePath; use a build.zig aggregator module (the bench_core.zig pattern).
 - **[low] src/web/http_util.zig:104 — the hand-rolled HTTP parsers (percent-decoder, path/cookie/query parsing, DoH base64url) are the third untrusted-byte family and the only one unfuzzed.**
  The CI half of the original finding was refuted: corpus-replay-in-`zig build test` is the documented, sanctioned CI fuzz smoke, and `--fuzz=<n>` is blocked by upstream Zig 0.16 defects on stock installs. What remains: add decodeInPlace/parsePath (and optionally dnsParam) targets in the existing harness pattern, running as corpus replay.
 ## Theme 2: Duplicated infrastructure, drift already underway (11 findings)
 > Status (m18): all 11 findings closed by milestone 18 (specs/milestone-18.md).
 - **[high — CLOSED m18] src/server/dot_server.zig:3 — connection-server lifecycle machinery duplicated across four listeners.** *(merged: tcp_server.zig:218, dot_server.zig:180, dot_server.zig:452 — four findings describing the same debt)*
  tcp_server, dot_server, doh_server, and web/server each carry a private copy of the same slot pool, claim/finish/beginShutdown protocol, accept loop with error mapping, cancel-protection dance, and select-based idle race. `race`/`expire`/`readPrefix`/`readBody`/`writeReply` are byte-identical between tcp and dot; dot_server's header says outright "This file mirrors tcp_server.zig". Drift is live: the counter is `accepted` on TCP vs `connections` on DoT/DoH; `decideClaim` is written two ways for identical semantics; doh_server ships a module-level `serve()` that nothing calls (dead divergent glue); the milestone-10 review log records a TLS-context leak fixed by hand-porting a pattern between copies — the predicted failure mode has already fired once. This is the most invariant-heavy concurrency code in the repo (mutex-ordered shutdown, cancel-protection windows), copied four times with no compiler help. The milestone-10 "mirror tcp_server's shape" ruling was a parallel-session build instruction, not an architectural decision against extraction. Fix: one shared listener core (comptime-parameterized slot pool + claim/shutdown + accept loop + race) with the per-connection serve function and TLS handshake stage as variation points; at minimum, extract race/expire/readPrefix/bump/Stop into src/server/ helpers.
 - **[medium — CLOSED m18] src/storage/repositories/groups_repo.zig:29 — list/free/errdefer scaffolding hand-rolled 18 times across seven repos.**
  Every repo re-implements prepare → ArrayList → the load-bearing errdefer ordering → per-column columnTextAlloc → append, plus a matching freeX and allocation-failure test. Decisive detail from verification: the milestone-4 spec's own reference sample declares the two errdefers in the reverse (fatal, use-after-free) order — every implementation silently corrected it, and a future repo copied from the spec reproduces the UAF unless its author also copies the test. Fix: a shared comptime helper in crud.zig (`list(RowType, sql, readRow)` plus paired free) so the memory-safety choreography exists once.
 - **[medium — CLOSED m18] src/web/handlers/groups.zig:168 — CRUD shell duplicated across seven handler files; the 4-line configDb switch appears 40 times.**
  list/get/remove handlers and the applyCreate/Update/Delete shells are identical modulo repo function and message string, and the copies have already diverged in lock scope and reload flavor (local.zig holds the lock through publish; blocklists adds pruneFiles; groups alone reads back under the lock). Fix: a comptime resource descriptor (repo fns + conflict message + reload flavor) generating the shells, keeping the genuinely different per-resource decision functions hand-written. Note the four distinct reload flavors make this less mechanical than the finding implies.
 - **[medium — CLOSED m18] web/src/features/local/RecordsTab.tsx:25 — RecordsTab/ZonesTab are structural copy-paste, and Tailwind class constants are re-declared across 12 files.** *(merged: RecordsTab.tsx:14, the class-constant half of the same debt)*
  The two tabs share identical FormState/openForm/onSubmit/onDelete plumbing, mutation trio, and byte-identical class constants; the focus-visible literal appears in 21 files, and drift has already shipped: PrefixesEditor and GroupsPage inputs omit the focus-visible outline entirely, silently violating the milestone-9 accessibility floor. The project's own precedent (InlineError was hoisted and deduplicated during milestone 9) says this is house style left unapplied. Fix: shared form-shell/table-shell components and a ui/ directory (or one classes.ts module).
 - **[low — CLOSED m18] src/local/forward_client.zig:260 — race-against-deadline and stream-error-unwrap scaffolding copied across three transports.**
  The Outcome union + expire() select race is byte-identical between pool.zig and forward_client.zig (`fn expire(` appears 14 times tree-wide); the cancel-protected close helpers and mapPhase/sendFailure/receiveFailure repeat between dot_client and forward_client. The predicted drift is already real: doh_client has none of the unwrap (see Theme 3). Fix: shared helpers in transport.zig, which already owns the framing helpers and error taxonomy.
 - **[low — CLOSED m18] src/local/records.zig:197 — normalizeName duplicated verbatim in forward_zones.zig, with a third partial copy in dns_cache.buildKey.**
  Divergence risk is not hypothetical: filter/rules.zig and filter/compiler.zig already carry near-variants with differing byte-rejection policies. Fix: move it to dns/name.zig next to fromText (both callers already import it).
 - **[low — CLOSED m18] src/filter/compiler.zig:64 — the subtle takeDelimiter/StreamTooLong/discard streaming loop exists twice (compile and manager.collectSample).**
  Both copies are correct and tested, and the cited behavioral differences are intentional per call site; the risk is a std.Io.Reader semantics change or a third copy-paste. Fix: a shared bounded-line-iterator helper. (The 35f2324 linkage in the original finding was inaccurate — that bug was reader-buffer aliasing, a different class.)
 - **[low — CLOSED m18] src/cli.zig:906 — upstream client construction duplicated between probeUpstreams and app.Upstreams.build.**
  Concrete drift hazard: cli.zig hardcodes the DoH buffer sizes as literals that app.zig defines as named constants — changing the constants leaves `check` probing different buffers than `run` uses, undermining the probe's stated purpose. Fix: a shared build-one-client-per-entry helper, or at minimum shared size constants.
 - **[low — CLOSED m18] src/cli.zig:722 — ZON parse failure rendered through two channels in `check` vs `import`.**
  check prints the multi-line zon_diag inline (embedding newlines mid-FAIL-line, contradicting the one-line-per-problem promise in docs/reference/configuration.md:332), import routes through reportParseFailure/Diagnostics. Fix: expose reportParseFailure (it is currently private) and route check through it.
 - **[low — CLOSED m18] build.zig:155 — test-suite module wiring duplicated block-for-block for host and aarch64, plus a re-spelled target triple.**
  Divergence is caught loudly (compile/link errors, blocking CI qemu job), just late. Fix: an addTestSuite helper mirroring addExecutable, triple from cross_targets.
 - **[low — CLOSED m18] tests/fuzz/blocklist_fuzz.zig:160 — Smith corpus encoders (u32-LE length-prefix convention) copy-pasted between the two fuzz files.**
  A Smith stream-format change caught in one file and missed in the other leaves that corpus silently decoding to garbage while targets stay green. Fix: a shared dependency-free tests/fuzz/smith_encode.zig — no build.zig change needed since dns_fuzz.zig already imports corpus.zig by relative path.
 ## Theme 3: Silent failures and swallowed errors (11 findings)
 - **[medium — CLOSED m16] src/filter/manager.zig:541 — loadSource swallows error.Canceled into a per-source `.load_failed` status.**
  Every other catch site in the file (roughly ten) propagates Canceled per the milestone-3 invariant "error.Canceled must never be recorded at all"; here a cancellation during a reload's file reads records a bogus "Canceled" load failure, consumes the one-shot cancellation signal, and publishes a snapshot with that source excluded. Fix: one more catch arm per readFileAlloc call (`error.Canceled => return error.Canceled`; Manager.Error already has the member).
 - **[medium — CLOSED m16] src/platform/tls_server.zig:430 — BIO callbacks discard the concrete transport error, including cancellation.**
  net_reader.err/net_writer.err (which hold Reset/Timeout/Canceled) are never read; everything collapses to MBEDTLS_ERR_NET_RECV/SEND_FAILED, so a routine idle-budget cancel surfaces as a warn-level "mbedtls_ssl_read failed" and peer resets are indistinguishable from timeouts. The client side (dot_client's concreteRead/concreteWrite) solved exactly this. Fix: capture the stashed causes in the BIO callbacks and fold them into the error surface — note the ReadError set must be widened, not just the field captured.
 - **[medium — CLOSED m16] src/web/server.zig:616 — a Cookie header over 1024 bytes silently degrades to unauthenticated.**
  copyHeader returns "" with no log; under the documented reverse-proxy-on-shared-domain deployment (foreign cookies riding along), every request 401s and the operator sees an unexplained login loop with zero diagnostics anywhere on the request path. Fix: extract only the nxdns_session pair from an oversized header (cookieValue already parses), or at minimum log the size-drop (no secret-logging conflict).
 - **[medium — CLOSED m16] web/src/features/queries/QueryLogPage.tsx:88 — load-more accumulation silently develops a mid-table row gap when the base page refetches.**
  The refocus-after-30s refetch shifts the newest-100 boundary up while `extra` starts strictly below the old cursor; the missing rows are in neither, and the stale cursorOverride means load-more never heals it. On a live DNS server the trigger is routine. Fix: useInfiniteQuery (or disable background refetch while extra is non-empty), or detect the discontinuity and reset the accumulation.
 - **[low — CLOSED m18] src/upstream/doh_client.zig:157 — DoH mapError never unwraps the stashed cause behind ReadFailed/WriteFailed.**
  Verification narrowed the blast radius: the pool's select harness means shutdown cancellation is handled correctly despite this, so the reachable impact is rare mid-exchange local-resource errors (e.g. ENOBUFS) recorded as peer faults against a healthy upstream — still a stated spec-invariant violation, fixed once for DoT and left in DoH. Fix: the same unwrap the other two transports carry.
 - **[low — CLOSED m16] src/filter/manager.zig:1237 — commitStatus silently drops the outcome of a source with no status entry.**
  Reachable today via a race between startupPass and a web-inserted source; the milestone-5 policy says every non-ok state "is recorded and surfaced, not swallowed". Fix: warn on the fall-through or have refreshSourceLocked insert the missing entry.
 - **[low — CLOSED m19] src/web/http_util.zig:335 — static header-array overflow reported as OutOfMemory.**
  A future eighth header would manifest as mysterious OOM-labelled connection drops. All call sites are static (max 3 today). Fix: std.debug.assert so the mistake fails loudly in tests.
 - **[low — CLOSED m19] web/src/features/blocklists/BlocklistForm.tsx:30 — bare `catch {}` on submit swallows everything, not just the mutation error it assumes.**
  An exception outside mutateAsync never reaches mutation state, so the form silently does nothing with no console trace. The codebase's own convention (milestone-9 review: logout swallows only 401) is swallow-only-the-expected-class. Fix: rethrow or console.error anything the inline-error prop will not display.
 - **[low — CLOSED m19] tools/gen_web_assets.zig:71 — dist-shipped .gz siblings trusted unverified; orphan .gz files embed as unreachable bytes.**
  The embedded-dist test partially covers this but never gunzips-and-compares, and CI's test run never sees the dist that ships (`cross -Dweb-dist` vs `test -Dintegration`). Exposure is latent (no compression plugin today). Fix: decompress and byte-compare shipped siblings; reject orphans.
 - **[low — CLOSED m19] deploy/docker/Dockerfile:20 — `${TARGETARCH:-amd64}` bypasses the fail-fast arm under the legacy builder.**
  A plain docker build on an arm64 host (the Pi 5 target) packages the x86_64 binary and dies at `docker run` with exec-format, far from the mistake. Fix: drop the default and error naming buildx, or default from `uname -m`.
 - **[low — CLOSED m19] src/platform/logging.zig:287 — oversized log messages truncate mid-format with no marker.**
  `mw.print(...) catch {}` then uses the partial buffer, contradicting the sink's own "never both discarded and silent" invariant and the '...'-marker pattern safe_url.zig uses. Fix: catch the fixed-writer overflow and append a marker or bump a lines_truncated stat.
 ## Theme 4: Operator-facing contract drift (11 findings)
 - **[medium — CLOSED m17] src/upstream/pool.zig:110 — no overall exchange deadline; the key named `total_timeout_ms` bounds one attempt.**
  With N upstreams all timing out, a query takes N × 5000ms; docs/reference/configuration.md calls the key a "per-query budget" while cli.md correctly says "per-attempt" — the two reference docs contradict each other, PLAN still promises a total budget, and validate.zig's total>=read cross-check relates knobs of different subsystems. The per-attempt code is intentional per milestone-3; the name, docs, and PLAN are the drift. Fix: either one race around the failover loop, or rename to attempt_timeout_ms and correct docs/PLAN/validator.
 - **[medium — CLOSED m17] src/config/validate.zig:609 — the validator accepts hostname `tls://` upstreams that the DoT client refuses by design on every dial.**
  Literal documented spec drift: milestone-3 explicitly told the orchestrator to carry the IP-literal requirement into the phase-4 validator, and it was dropped. Worse, docs/reference/configuration.md:348 presents a hostname NextDNS `tls://` config that can never complete an exchange. Fix: apply the same IP-literal check parseResolver already enforces, at the spot where checkCollections already holds the parsed Endpoint.
 - **[medium — CLOSED m17] src/config/validate.zig:391 — no upper bound on the two fields that literally size boot allocations.**
  `query_log_buffer_max = 4000000000` validates clean, `nxdns check` prints OK, `nxdns run` OOMs classified as a runtime failure. Timeouts and TTLs have maxima in the same file. Fix: sanity caps (or a byte-budget check) matching the file's established idiom; a full memory-fit guarantee is out of reach and not needed.
 - **[medium — CLOSED m17] web/src/lib/queries.ts:251 — the upstream management UI was specced in milestone 8 ("the Settings page must edit them") and never built.**
  The full client layer (API wrappers, query, three mutation factories) exists with zero consumers; no route or page edits the resolver pool, so URLs/priority/enabled/tls_name are curl-only. Same pattern in miniature: ruleUpdateMutation and five get* wrappers are unused. Fix: build the editor the m8 spec called for, or delete the vestigial factories and record the scope decision.
 - **[medium — CLOSED m17] src/web/api_limiter.zig:137 — behind the documented reverse proxy, the API limiter is bypassed and the SSE cap collapses.**
  Proxy-on-same-box makes every request loopback; default `localhost_exempt = true` then disables the only brake on repeated argon2 verifications (the spec-mandated brute-force defense), while remote users share one address's 3-stream SSE cap. Documented nowhere. Fix: an opt-in trusted-proxy setting (parse X-Forwarded-For only from configured proxies) or at minimum a documented warning to set `api_localhost_exempt=false` when proxying.
 - **[medium — CLOSED m17] web/src/lib/types.ts:1 — the REST contract lives in three hand-synced copies with a route-name-only drift guard.**
  Zig handlers, openapi.yaml, and types.ts are each maintained by hand; the only automated guard checks route paths textually. The Zig side is genuinely well contract-tested against a real server, but the server↔yaml and server↔types.ts field-level seams are unguarded, and every frontend test stubs fetch. The milestone-9 "no codegen" ruling excludes generation, not guarding. Fix: one integration layer asserting the frontend's consumed shapes against real responses per endpoint.
 - **[low — CLOSED m17] src/dns/packet.zig:354 — BADVERS is unexpressible: addOptEcho hardcodes extended_rcode = 0 and no caller checks the EDNS version.**
  A version-1 query answered locally gets NOERROR with a version-0 OPT instead of RCODE=16 (RFC 6891 MUST). Negligible operational impact (real clients send version 0; forwarded replies pass through), but the write-direction mechanism is missing entirely. Fix: an extended_rcode parameter on addOptEcho plus a version check in the handler.
 - **[low — CLOSED m17] src/server/rate_limiter.zig:5 — module docs still defer their own concurrency contract to "Phase 7", which shipped.**
  The authoritative thread-safety statement points at a future that resolved (handler.limiter_mutex, app.runMaintenance); shutdown.zig references a Phase-8 restart endpoint that never shipped and now cannot. Fix: rewrite the headers to state the as-built contract.
 - **[low — CLOSED m17] docs/how-to/install-with-systemd.md:206 — `nxdns 0.1.0-dev` hardcoded in four doc transcripts with no drift guard.**
  Correct today; goes silently wrong at the first tag. The milestone-14 spec already mandates the placeholder + guard. Caveat for implementation: milestone-13 requires every doc command block to be executed verbatim, so decide how placeholders coexist with executed transcripts.
 - **[low — CLOSED m17] src/platform/logging.zig:497 — manual file_pos tracking assumes exclusive ownership of the log path, undocumented for operators.**
  logrotate copytruncate produces a sparse NUL-prefixed file; the docs describe built-in rotation, even claiming the file is "opened for append" (it is not — positional writes), without warning off external rotation. Fix: a warning in files-and-directories.md at minimum; a stat/length re-check before write for robustness.
 ## Theme 5: Protocol robustness (5 findings)
 - **[medium — CLOSED m16] src/cache/dns_cache.zig:110 — truncated (TC=1) responses are cacheable and cached for full TTL.**
  classify checks rcode and ancount but never `flags.tc`, and validateResponse doesn't either; a misbehaving upstream returning TC=1 over DoH/DoT/forward-TCP gets its partial answer cached and served (TC bit intact, even over TCP — a protocol violation that can loop retrying clients) for up to 86400s. RFC 2181 §9 forbids this; no spec declares it out of scope. Fix: reject tc in classify, arguably also in validateResponse.
 - **[medium — CLOSED m16] src/filter/parsers.zig:66 — the format sniffer misclassifies hosts files with `##` banner comments as ABP.**
  hasAbpMarker runs before isComment and matches `##` unanchored with no comment guard (unlike the deliberately guarded `$` branch two lines later). Reproduced with the literal URLhaus banner style: the whole file parses as ABP, `0.0.0.0` enters the compiled domain set, and — worse than the original finding — hosts lines with inline reference comments containing URLs are silently dropped (real blocking loss). Fix caveat from verification: guarding the element-hiding branch with !isComment is a circular no-op; instead anchor the `##` match per the As-built note, or treat `##` at line start on a `#`-initial line as a banner.
 - **[medium — CLOSED m16] web/src/features/live/useLiveQueries.ts:29 — SSE cap/session-expiry detection relies on EventSource retry behavior that does not exist for HTTP rejections.**
  Per the WHATWG spec a non-200 response fails the connection permanently after one error event, so the 3-consecutive-errors threshold is unreachable on exactly the 429/401 paths it was built for: the UI shows "Reconnecting…" forever, the capped state and session probe are dead, and the spec's required "too many live viewers + retry" state is violated. FakeEventSource has no readyState/reconnect semantics, so tests pass — the mocked-network class again. Fix: expose readyState on EventSourceLike, treat CLOSED in the error handler as permanent failure, and run the session probe there.
 - **[low — CLOSED m19] src/filter/fetcher.zig:169 — TLS error classification by @errorName prefix string-matching.**
  Duplicated in doh_client.zig; a std rename silently downgrades TLS failures into generic buckets, and the test exercises inputs std can no longer produce (verification shows only TlsInitializationFailed and CertificateBundleLoadFailure are reachable). Contradicts transport.zig's own exhaustive-switch philosophy. Fix: name the reachable errors in the exact-match switch in both files.
 - **[low — CLOSED m19] src/platform/tls_server.zig:506 — hand-transcribed mbedtls error codes/config enums have no drift guard, unlike sizes/alignment.**
  All ten values currently match the pinned 3.6.7 headers; the exposure is the eventual version bump, where a renumbered close_notify code would silently invert truncation-detection semantics. The sizes-vs-constants asymmetry is principled (hash-pinned version), so this is cheap insurance, not an oversight to be alarmed about. Fix: export the constants from mbedtls_shim.c and verify once at init.
 ## Theme 6: Concurrency, locking and lifecycle coupling (6 findings)
 - **[medium — CLOSED m16] src/filter/manager.zig:609 — writer_lock held across every download of a refresh pass, stalling all web mutations' reloads.** *(merged: web/handlers/blocklists.zig:171, the same debt from the manual-refresh-endpoint angle)*
  refreshAll holds writer_lock uncancelably across N × 300s download budgets plus compiles; every mutation handler ends in Manager.reload on the same lock, so an unrelated rule save parks uncancelably (on a request path with deliberately no timeout, occupying one of 64 slots) until the pass ends. The spec's serialization rationale covers only readers-never-wait; writer-vs-writer latency is unaddressed emergent behavior. DNS serving is unaffected and the stalled edit is not lost. Fix: download+compile to tmp files outside the lock, take it only for publish+DB write+reload — or a 202-with-poll job model for the manual endpoint.
 - **[medium — CLOSED m16] src/storage/retention.zig:95 — VACUUM runs even when the disk monitor says critical.**
  The logger and blocklist refresher gate on `writesAllowed()`; the seventh-pass VACUUM — the most expensive write the program makes, needing roughly DB-size free space — fires unconditionally, transiently draining the exact filesystem the monitor watches, then failing SQLITE_FULL with no retry for seven daily passes. Prune and checkpoint should keep running (they free space). Fix: pass the optional monitor into runOnce the way the two sibling tasks already do, gating only the vacuum step.
 - **[medium — CLOSED m16] src/web/sse.zig:153 — Hub has no shutdown broadcast; graceful drain stalls up to 15s per idle SSE subscriber.**
  Socket shutdown cannot wake a task parked on the per-slot Event; only the heartbeat write fails, up to 15s later — measured: the SSE integration teardown sits exactly 15s in group.await. Production is masked by the cancel path, but the documented "shutdown unblocks every connection" contract is violated. Fix: Hub.close(io) setting a shutdown flag and signaling every active slot's event, called from beginShutdown.
 - **[low — CLOSED m19] src/web/handlers/groups.zig:70 — reload runs outside config_lock in five handlers but inside it in local.zig; the unlocked variant is safe only via an undocumented Manager invariant.**
  Safety depends on Manager.reload re-reading the database under its own writer_lock — a cross-module fact stated nowhere at the call sites; changing reload to accept pre-read rows (the shape swapLocalTables already has) would silently regress five handlers with the exact ordering bug the milestone-8 review already fixed once in local.zig. Fix: document the re-read requirement at mutations.reload, or hold config_lock uniformly and document the cost.
 - **[low — CLOSED m16] src/web/handlers/settings.zig:312 — config_lock held across argon2id hashing (19 MiB, t=2) in the settings PUT.**
  Stalls every mutation handler and the settings GET for the hash duration on the Pi 5, contradicting the lock discipline auth.zig documents and the login path follows. Hashing depends only on the parsed patch; the LiveHash generation check already handles the concurrent-install race. Fix: hash before acquiring the lock.
 - **[low — CLOSED m16] src/server/doh_server.zig:66 — idle_timeout only bounds the handshake; idle keep-alive clients pin DoH slots until restart.**
  The web-listener precedent the spec cites assumes clients eventually close; DoH stubs hold keep-alives by design, and with no TCP keepalive a vanished peer pins a slot permanently — while DoT on the same LAN reclaims after 10s. Fix: extend the race to receiveHead the way DoT races readPrefix, or at least rename the misleading option.
 ## Theme 7: Observability debt (6 findings)
 - **[medium — CLOSED m16] src/server/udp_server.zig:38 — UDP/53 and TCP/53 listener stats are written and never read anywhere.**
  dropped_no_slot/dropped_oversize increment with no metric, API field, or log line (errors log only at debug below the default level), while the DoT/DoH siblings export equivalent counters to /metrics. The module doc sells "dropped and counted" as the failure surface; the surface is unobservable. Fix: give both the same snapshotStats() + metric families the TLS listeners have.
 - **[medium — CLOSED m16] src/local/forward_client.zig:45 — ForwardClient.Stats are instrumented but discarded in production.**
  The doc claims the foreign-datagram counter prevents "an unrecorded failure mode"; the only production caller builds a stack-local client per query and drops it, so the spoofing signal does not exist and truncation/failure counters never reach /metrics. Fix: aggregate into handler-owned atomics like the rest of the pipeline, or delete the Stats struct.
 - **[medium — CLOSED m16] src/server/dot_server.zig:313 — a timed-out handshake counts as tls_handshake_failures on DoT but idle_timeouts on DoH.**
  The DoT classification is the recorded spec ruling; DoH drifted in the copy, and since DoH requests have no timeout, its idle_timeouts metric can only ever mean handshake stalls — the same exported name carries disjoint semantics per listener, corrupting cross-listener dashboards. Fix: align DoH's .timed_out to tls_handshake_failures per the ruling.
 - **[medium — CLOSED m16] src/platform/tls_server.zig:311 — warn-level logging for routine peer misbehavior contradicts the module's own debug ruling.**
  A peer TCP drop is deliberately debug on the read path, then the close_notify against the dead socket warns; probes that send bytes or reject the cert warn per event, and .tls_server is not in the dedup scope set, so peer-driven warns can evict genuine warnings from the rotating log. Fix: apply the read path's peer-vs-local triage to close/handshake reporting, or add the scope to the dedup set.
 - **[medium — CLOSED m16] src/web/api_limiter.zig:210 — ApiLimiter.sweep is never called in production.**
  Designed for background reclamation (preallocated stale_keys so sweep never allocates), but only the DNS-side limiter got scheduled in runMaintenance. Once 4096 distinct addresses are seen, the table stays full forever and every unknown-address request pays an O(4096) scan under the limiter mutex. Fix: add it to the maintenance loop, or delete sweep and own inline-eviction-only.
 - **[low — CLOSED m19] src/server/handler.zig:255 — two tracker-mutex acquisitions per query (scan + full stats copy) purely to mirror one counter.**
  Fix: track() returns fullness, or dropped_full becomes an atomic readable without the mutex.
 ## Theme 8: Dead code, hardcoded values, frontend state hygiene (11 findings)
 - **[medium — CLOSED m19] web/src/features/settings/SettingsPage.tsx:16 — settings field registry is stringly typed and cast-driven.**
  A typo'd key compiles, renders undefined, and breaks the field (verification: an edited typo'd key is actually sent and rejected at save — still broken, tsc still green). Fix: `key: keyof Settings[S]` via a generic SectionDef, which removes every cast and makes drift a compile error.
 - **[medium — CLOSED m19] web/src/features/blocklists/BlocklistsPage.tsx:33 — blocklist refresh status lives in an observer-less query-cache entry.**
  Read with non-subscribing getQueryData (renders only via the mutation's coincidental re-render) and garbage-collected after 5 minutes, after which the page claims no snapshot ever existed. This is client UI state, not server-cache state. Fix: a small store (the restartBanner useSyncExternalStore pattern already in-repo) or a real subscriber with gcTime: Infinity.
 - **[low — CLOSED m19] src/dns/edns.zig:111 — parseEcs, Ecs, ecs_family_*, extendedRcode have zero production callers.**
  stripEcs filters by option code without decoding; forward mode keys on raw bytes; no path composes an extended RCODE (see BADVERS). The project's own precedent deleted ecsPayload when it lost its caller. Fix: wire extendedRcode into a BADVERS path (giving it a caller) or delete both and let git history resurrect them.
 - **[low — CLOSED m19] src/web/handlers/settings.zig:354 — dead errdefer in writeSettings implies protection it cannot provide.**
  The function returns `?db.Error`, so every exit is a value return and the errdefer never fires; the real safety is three explicit rollbacks below it. Fix: return `db.Error!void` so the spec's canonical errdefer pattern becomes live and the manual rollbacks collapse.
 - **[low — CLOSED m19] web/src/features/settings/SettingsPage.tsx:220 — the settings diff compares live query data against a mount-time clone.**
  A background refetch makes out-of-band changes appear as user edits; Save silently reverts them. Fix: track dirty keys and rebase untouched fields, or freeze the baseline.
 - **[low — CLOSED m19] web/src/features/rules/RulesPage.tsx:24 — default group id 1 encoded four ways with two fallback semantics.**
  RulesPage preselects the alphabetically first group (the API orders by name), not the default group. Fix: one exported DEFAULT_GROUP_ID and a defaultGroupId(groups) helper.
 - **[low — CLOSED m19] web/src/routes.tsx:97 — the dashboard loader's Promise.all makes the page's per-widget error handling unreachable on first load.**
  One failing endpoint blanks the whole dashboard on cold navigation; the graceful per-section degrade only runs for background refetches. Fix: prime without throwing (allSettled / fire-and-forget) so the granular UI owns failures.
 - **[low — CLOSED m19] web/src/lib/queries.ts:99 — raw `["lookup"]` / `["groups"]` literals bypass the queryKeys table at eight sites.**
  A key restructure updates typed usages and leaves the literals silently invalidating nothing. Fix: a `lookupAll` entry and using queryKeys.groups.
 - **[low — CLOSED m19] src/storage/logger.zig:44 — Entry buffer sizes are file-private and re-hardcoded by callers whose comptime guards check their own copies.**
  handler.zig's reason-length guard proves fit against its local 32, not logger's constant; shrinking logger's buffer passes the guard and silently truncates. Fix: make the constants pub and point the guards at them. (The two max_ip_text=45 copies derive from RFC 5952 independently; that portion is marginal.)
 - **[low — CLOSED m19] src/app.zig:609 — dev-mode asset directory in a module-level mutable global.**
  Verification corrected the root cause: the fallback pointer already receives *WebState and discards it; the fix is a `dev_dir` field on WebState set at the composition root — not the signature-widening the finding proposed.
 - **[low — CLOSED m19] src/server/cert_store.zig:326 — private key PEM freed without zeroization.**
  Occurs at boot and on actual cert renewals (not every poll); the parsed key stays resident in mbedTLS regardless, so this closes only the freed-page-reuse exposure. Fix must wipe inside readPem (readFileAllocOptions may reallocate, leaving intermediate copies), not just a defer at the call site.
 ---
 ## Suggested cleanup order
 **Phase 1 — make green mean green (Theme 1, first).**
 Nothing else can be validated until the pipeline is trustworthy. Start with ci.yml → master (already specced in milestone-14 §7), then the test-binary abort (root-cause or document), the web/dist freshness guard, the tests.zig completeness guard, and a cron on live-tls.yml. These are independent of each other and of everything below, and every later phase relies on them to verify itself. The remaining Theme-1 items (fuzz targets, multi-read fixture, docs_drift derivation) can trail in the same phase.
 **Phase 2 — contained behavioral fixes with real failure modes (Themes 3, 5, 6, 7 mediums).**
 Do these before any large refactor so the refactors don't carry known bugs forward: the loadSource Canceled arm, the TC=1 check in classify, the sniffer banner rule (use the anchoring fix, not the circular guard), the VACUUM disk gate, the cookie-header extraction/log, the EventSource readyState handling, the DoH/DoT metric alignment, Hub.close, the ApiLimiter sweep wiring, and exporting the UDP/TCP listener stats. Each is localized, each has a verifier-checked fix shape, and several (sniffer, TC=1, EventSource) fix behavior users can hit today.
 **Phase 3 — contract decisions (Theme 4).**
 These need a decision more than code: total_timeout rename vs. real deadline; upstream UI build vs. delete; the trusted-proxy setting; validator IP-literal and allocation bounds; the types.ts guard strategy. Decide once, then the edits are mechanical. Doing this after Phase 1 means the doc-drift guards you add here actually run.
 **Phase 4 — the duplication refactors (Theme 2).**
 Largest payoff, largest risk, so they go after the pipeline is trustworthy and the known bugs are out (a refactor that must simultaneously fix behavior is how mirrored copies diverge further). Order within the theme: the listener core first (the only high, four copies, drift underway, and it subsumes several per-listener findings); then the repo list helper (the spec itself contains the fatal ordering, so future repos are landmines until this exists); then the web CRUD descriptor, transport race/close helpers, and the frontend ui/ extraction. Each collapse means every Phase-2-style fix in that family lands once from then on.
 **Phase 5 — hygiene sweep (Theme 8 and remaining lows).**
 Dead code deletions (parseEcs, dead errdefer, unused mutation factories per the Phase-3 decision), pub constants, the query-key table, DEFAULT_GROUP_ID, the WebState dev_dir field, key zeroization. Low risk, no dependencies; several ride along naturally with Phase 4 files they touch.