547 lines
27 KiB
Markdown
547 lines
27 KiB
Markdown
# Milestone 19: hygiene sweep
|
|
|
|
Goal: remove the dead surface, unify the re-hardcoded constants, close the
|
|
small silent failures, and fix the frontend state hazards — the residue of
|
|
`TECH_DEBT.md` Theme 8 plus the remaining lows, after the behavioral fixes
|
|
(m16), the contract work (m17) and the duplication refactors (m18) have
|
|
landed.
|
|
|
|
**PROVISIONAL.** This spec was written before milestones 16-18 were built. It
|
|
assumes: m17 shipped BADVERS (so `edns.extendedRcode` has a production
|
|
caller), m17 built the upstream editor (so the queries.ts upstream factories
|
|
have consumers and are not deleted here), and m18 shipped the frontend `ui/`
|
|
extraction and deleted doh_server's dead `serve()`. Before starting this
|
|
milestone, re-verify every line reference and update this spec where the
|
|
earlier milestones moved things.
|
|
|
|
## Rulings (binding)
|
|
|
|
### 1. The dead ECS parse surface is deleted
|
|
|
|
`src/dns/edns.zig`: delete `parseEcs` (:111), `Ecs` (:98-105), `EcsError`
|
|
(:107), `ecs_family_ipv4`/`ecs_family_ipv6` (:95-96), and their in-file tests
|
|
(:371-:469 block, nine tests). `stripEcs` filters by option code without
|
|
decoding, forward mode keys on raw bytes — no production path decodes ECS.
|
|
|
|
Trap the audit missed: `tests/fuzz/dns_fuzz.zig:86` calls `edns.parseEcs`
|
|
inside a fuzz target that the default `test` step builds. Delete that line in
|
|
the same change or the build breaks.
|
|
|
|
`extendedRcode` (:272) is **kept**: milestone 17 gave it the BADVERS caller.
|
|
If m17 diverged and BADVERS was not built, delete it too and note the
|
|
divergence here.
|
|
|
|
Precedent: `ecsPayload` was built and removed within milestone 7 when it lost
|
|
its only caller (specs/milestone-7.md:361). Git history keeps the code.
|
|
|
|
### 2. `writeSettings` returns an error union
|
|
|
|
`src/web/handlers/settings.zig:349` returns `?db.Error`, so every exit is a
|
|
value return and the `errdefer` at :354 never fires — it implies protection
|
|
it cannot provide. The real safety is two explicit `tx.rollback()` calls
|
|
(:358, :363 — the audit said three; it is two).
|
|
|
|
Change the return type to `db.Error!void`. The errdefer becomes live
|
|
(`db.Tx.rollback` at storage/db.zig:814 is idempotent and documented safe in
|
|
an errdefer), and the two manual rollback-then-return arms collapse into
|
|
plain `try`/error returns. The one caller (`applyPut`, :327) switches from
|
|
`if (writeSettings(...)) |err|` to `catch`; its hash-free-on-failure behavior
|
|
is unchanged.
|
|
|
|
### 3. The logger's field widths become the single source
|
|
|
|
`src/storage/logger.zig:44-48` holds four file-private constants
|
|
(`max_domain_len = 253`, `max_client_len = 45`, `max_reason_len = 32`,
|
|
`max_upstream_len = 64`). Make all four `pub`. Then:
|
|
|
|
- `src/server/handler.zig:67` deletes its local `max_reason_len = 32`; the
|
|
comptime guard at :69-75 checks `logger.max_reason_len`, so the guard
|
|
finally proves what its comment claims.
|
|
- `src/server/handler.zig:79` deletes its local `max_ip_text = 45` and uses
|
|
`logger.max_client_len` (the :82 `max_resolver_text` derivation follows).
|
|
- `src/server/clients.zig:32` likewise.
|
|
- `src/web/handlers/queries.zig:27-30` — the fifth copy the audit missed —
|
|
adopts `logger.max_domain_len` for the domain width. Its `max_client_len`
|
|
is **64**, not 45; the query log's client column is written by the logger
|
|
and can never exceed 45 bytes, so it adopts `logger.max_client_len` too.
|
|
If a test proves a wider value ever reaches that field, stop and record
|
|
the divergence instead of forcing it.
|
|
|
|
Acceptance is a grep: the literals 253, 45 and 32 appear in these roles only
|
|
in logger.zig.
|
|
|
|
### 4. `track()` reports fullness; one mutex acquisition per query
|
|
|
|
`src/server/handler.zig:255-258` takes the tracker mutex twice per query:
|
|
`tracker.track(io, from)` and then `tracker.snapshotStats(io)` — a full
|
|
struct copy — solely to mirror `dropped_full` into the handler's atomic
|
|
`tracker_full` (:150-153).
|
|
|
|
Change `Tracker.track`/`trackAt` (`src/server/clients.zig:89-97`) to return
|
|
`u64`: the current `dropped_full`, read under the mutex they already hold.
|
|
The handler stores that return value. `snapshotStats` stays for /metrics.
|
|
|
|
### 5. Log truncation gets a marker and a counter
|
|
|
|
`src/platform/logging.zig:285-288`: `mw.print(format, args) catch {}` on a
|
|
fixed 2048-byte writer, then the partial buffer is used with no marker — the
|
|
module's own never-both-discarded-and-silent invariant, violated in its own
|
|
kitchen. On `error.WriteFailed`: overwrite the final three bytes of the
|
|
buffered message with `...` (the safe_url.zig marker, src/safe_url.zig:11-14)
|
|
and increment a new `state.stats.lines_truncated`, sibling to
|
|
`lines_deduped` (:295). One test: a message over 2048 bytes ends in `...`
|
|
and bumps the counter by one.
|
|
|
|
### 6. TLS errors are classified by name, not by name prefix
|
|
|
|
`src/filter/fetcher.zig:169-170` and `src/upstream/doh_client.zig:160-161`
|
|
classify TLS failures with `startsWith(@errorName(err), "Tls")` /
|
|
`"Certificate"` — a std rename silently downgrades TLS failures into generic
|
|
buckets. Verified reachable set from `std.http.Client` (0.16.0) **today**:
|
|
exactly `error.TlsInitializationFailed` and
|
|
`error.CertificateBundleLoadFailure`. But this milestone lands after
|
|
milestone 18, whose ruling 4 makes the DoH path unwrap the client's stashed
|
|
read cause — and that cause set includes the record-layer members
|
|
(`TlsAlert`, `TlsBadRecordMac`, `TlsDecodeError`, ...) from
|
|
`std.crypto.tls.Client`. A two-member switch written against today's
|
|
surface would downgrade those to generic receive failures.
|
|
|
|
In both files: replace the prefix match with an exact switch → `error.TlsFailed`
|
|
naming the two top-level members **plus**, on whichever paths m18's cause
|
|
unwrap surfaces, the `Tls*` members of the unwrapped read-cause set —
|
|
enumerated at implementation time from the pinned std sources against the
|
|
m18 code as landed, not from this spec's list. The `else` falls through to
|
|
the existing per-phase mapping. Rewrite both tests (fetcher.zig:311-313,
|
|
doh_client.zig:261-263): they currently assert on `CertificateExpired`,
|
|
which nothing can produce, and omit `CertificateBundleLoadFailure`; the new
|
|
tests use only names from the verified reachable set, including a stashed
|
|
record-layer cause on the unwrapped path.
|
|
|
|
If m18 already extracted a shared `mapError` into transport.zig, apply this
|
|
ruling to the shared copy instead and note it here.
|
|
|
|
### 7. The transcribed mbedTLS values get an init-time check
|
|
|
|
`src/platform/tls_server.zig:501-511` hand-transcribes four config enums and
|
|
six error codes from the pinned Mbed TLS 3.6.7 headers with no drift guard —
|
|
unlike the sizes and alignment, which the shim reports. A renumbered
|
|
`close_notify` after a version bump would silently invert truncation
|
|
detection (:286, :351).
|
|
|
|
`src/platform/mbedtls_shim.c` gains ten getters in the existing shape
|
|
(`int nx_const_ssl_transport_stream(void) { return MBEDTLS_SSL_TRANSPORT_STREAM; }`
|
|
etc. — the needed headers are already included). `tls_server.zig` verifies
|
|
all ten against the transcribed constants next to the existing alignment
|
|
assert at :78, and the :657 test block asserts them too. Cheap insurance,
|
|
one-time cost.
|
|
|
|
### 8. The reload lock invariant is written down
|
|
|
|
Fourteen call sites in four handler files (groups, rules, clients,
|
|
blocklists — the audit said five handlers; it is four files, fourteen sites)
|
|
call `mutations.reload` *after* releasing `config_lock`. That is safe only
|
|
because the production `reload_fn` re-reads the database under the manager's
|
|
own writer lock — a cross-module fact stated nowhere. `local.zig` is the
|
|
deliberate opposite (publish under the lock; the ordering rationale lives at
|
|
local.zig:80-85).
|
|
|
|
Extend the doc comment on `mutations.reload`
|
|
(src/web/handlers/mutations.zig:122-125): the `reload_fn` contract is that it
|
|
re-reads all state from the database itself; it must never accept pre-read
|
|
rows (contrast `swapLocalTables`, which is the pre-read shape and is why
|
|
local.zig holds the lock). A future signature change to `reload` that adds
|
|
row-passing breaks fourteen call sites' correctness — the comment must say
|
|
exactly that. Documentation only; no locking change.
|
|
|
|
### 9. `web_dev_dir` moves into WebState
|
|
|
|
`src/app.zig:605-609` holds the `--web-dev` directory in a module-level
|
|
mutable global because the fallback handler "has no closure" — but
|
|
`serveWebDev` (:613-620) receives `*WebState` and discards it. Add
|
|
`dev_dir: []const u8 = ""` to `WebState` (src/web/server.zig, beside
|
|
`version`), set it at the composition root, read it in `serveWebDev`, delete
|
|
the global and its apologia comment.
|
|
|
|
### 10. The private key PEM is wiped
|
|
|
|
`src/server/cert_store.zig:326` frees the key PEM without zeroization, and
|
|
`readPem` (:345-362) uses `readFileAllocOptions`, whose grow-as-you-read can
|
|
leave intermediate copies in freed pages — a wipe at the call site cannot
|
|
reach them. Split the key path: a `readKeyPem` that reads through a single
|
|
fixed `max_pem_bytes + 1` buffer (no reallocation), copies to an exact-size
|
|
allocation, and `std.crypto.secureZero`s the big buffer before returning; the
|
|
caller wipes the returned slice before `gpa.free`. The idiom precedent is
|
|
auth.zig:228. The certificate PEM path stays as-is — it is public material.
|
|
Runs at boot and on real renewals only (:311-312 stats gate the poll).
|
|
|
|
### 11. Header-array overflow asserts
|
|
|
|
`src/web/http_util.zig:335` maps a too-long `extra_headers` onto
|
|
`error.OutOfMemory` — a future eighth header would surface as mysterious
|
|
OOM-labeled drops. Every one of the 62 call sites passes a compile-time
|
|
count, maximum 3 (static.zig:174, +1 for content-type = 4 of 8). Replace the
|
|
check with `std.debug.assert(extra_headers.len + 1 <= headers.len);` so the
|
|
mistake fails loudly in tests.
|
|
|
|
### 12. Shipped `.gz` siblings are verified; orphans are rejected
|
|
|
|
`tools/gen_web_assets.zig:71-77` embeds a dist-shipped `.gz` sibling with
|
|
zero content checks, and an orphan `.gz` (no base file) is silently embedded
|
|
as unreachable `application/octet-stream` bytes. Exposure is latent (no
|
|
frontend compression plugin today) — close it while it is cheap:
|
|
|
|
- Sibling: decompress with `std.compress.flate.Decompress` (container
|
|
`.gzip`; the tool already uses the Compress side at :133) and byte-compare
|
|
against the base file; `std.process.fatal` on mismatch, matching the
|
|
tool's existing error style (:73-75).
|
|
- Orphan: `std.process.fatal` naming the path, instead of indexing it.
|
|
|
|
Extend the embedded-dist test (`src/web/static.zig:413-439`) to decompress
|
|
each `.gz` entry and compare with its base — it currently checks magic bytes
|
|
and size only.
|
|
|
|
### 13. The Dockerfile arch default comes from the host
|
|
|
|
`deploy/docker/Dockerfile:21`: `${TARGETARCH:-amd64}` — under the legacy
|
|
builder TARGETARCH is empty, so a plain `docker build` on the Pi 5 packages
|
|
the x86_64 binary and dies at `docker run` with exec-format, far from the
|
|
mistake. Replace the default with a `uname -m` mapping resolved before the
|
|
case:
|
|
|
|
```dockerfile
|
|
RUN arch="${TARGETARCH:-}"; \
|
|
if [ -z "$arch" ]; then case "$(uname -m)" in \
|
|
x86_64) arch=amd64 ;; aarch64) arch=arm64 ;; \
|
|
*) echo "unsupported build host $(uname -m); use buildx" >&2; exit 1 ;; \
|
|
esac; fi; \
|
|
case "$arch" in ...existing arms... esac
|
|
```
|
|
|
|
The existing unsupported-arch arm stays fail-fast.
|
|
|
|
### 14. Frontend: the settings registry is typed
|
|
|
|
`web/src/features/settings/SettingsPage.tsx` — `SectionDef` exists (:21) but
|
|
`FieldDef.key` is `string` (:16), driving `as Record<string, unknown>` casts
|
|
at :217, :226, :261. A typo'd key compiles and breaks the field. Make the
|
|
pair generic:
|
|
|
|
```ts
|
|
interface FieldDef<S extends keyof Settings> {
|
|
key: keyof Settings[S] & string;
|
|
kind: "number" | "text" | "boolean" | readonly string[];
|
|
}
|
|
interface SectionDef<S extends keyof Settings> { section: S; title: string; fields: readonly FieldDef<S>[]; }
|
|
function defineSection<S extends keyof Settings>(def: SectionDef<S>) { return def; }
|
|
```
|
|
|
|
`SECTIONS` (:35-118, 12 sections) becomes a tuple of `defineSection(...)`
|
|
calls, `TLS_FIELDS` (:27) becomes `FieldDef<"doh_server" | "dot_server">[]`
|
|
compatible, and the three `Record<string, unknown>` casts are deleted. The
|
|
per-branch value casts inside `FieldRow` (:141, :160, :174, :198) may stay —
|
|
they are narrowing on `kind`, not drift holes. Acceptance: renaming a
|
|
Settings field makes `tsc` fail on the registry.
|
|
|
|
### 15. Frontend: the settings baseline is frozen
|
|
|
|
Same file, :220 — `buildSettingsPatch(data.settings, edited, ...)` diffs the
|
|
mount-time clone against **live** query data, so a background refetch makes
|
|
out-of-band changes appear as user edits and Save silently reverts them. Add
|
|
a `baseline` state initialized with the same `structuredClone` and reset in
|
|
the same `onSuccess` (:234-241) where `edited` resets; the patch becomes
|
|
`buildSettingsPatch(baseline, edited, ...)`. `settingsDiff.ts` is unchanged.
|
|
|
|
### 16. Frontend: refresh status leaves the query cache
|
|
|
|
`web/src/features/blocklists/BlocklistsPage.tsx:33` reads the refresh
|
|
snapshot with non-subscribing `getQueryData` from a cache entry that has no
|
|
queryFn, no subscriber, and the default 5-minute gcTime — after which the
|
|
page claims no refresh ever ran. This is client UI state. New module
|
|
`web/src/features/blocklists/refreshStore.ts` in the exact shape of
|
|
`settings/restartBanner.ts` (module-level value + listener Set +
|
|
`useSyncExternalStore` hook), holding `SourceStatus[] | null`. The mutation
|
|
(`blocklistsUpdateNowMutation`, lib/queries.ts:156) writes the store instead
|
|
of `setQueryData`; the page subscribes via the hook; the
|
|
`queryKeys.blocklistSources` entry and its doc comment are deleted. Note the
|
|
prefix-invalidation side effect disappears with it (invalidateBlocklistWorld
|
|
invalidating `["blocklists"]` used to mark the entry stale — harmless, since
|
|
the entry could never refetch).
|
|
|
|
### 17. Frontend: one default-group helper
|
|
|
|
Four encodings of "group 1", two semantics (GroupsPage.tsx:14 named
|
|
constant; PrefixesEditor.tsx:21 and LookupPage.tsx:133 byte-identical
|
|
prefer-id-1 expressions; RulesPage.tsx:24 first-of-list — which, because the
|
|
API orders by name (groups_repo.zig:148), preselects the alphabetically
|
|
first group, not the default). New module `web/src/lib/defaultGroup.ts`:
|
|
|
|
```ts
|
|
export const DEFAULT_GROUP_ID = 1;
|
|
export function defaultGroupId(groups: readonly Group[]): number {
|
|
return groups.find((g) => g.id === DEFAULT_GROUP_ID)?.id ?? groups[0]?.id ?? DEFAULT_GROUP_ID;
|
|
}
|
|
```
|
|
|
|
All four sites adopt it; RulesPage's preselect bug goes away with the
|
|
adoption.
|
|
|
|
### 18. Frontend: the dashboard primes without throwing
|
|
|
|
`web/src/routes.tsx:97-102` — the dashboard loader's `Promise.all` over four
|
|
`ensureQueryData` calls means one failing endpoint blanks the whole page with
|
|
`RouteError` on cold navigation, defeating the page's own per-widget
|
|
degrade (DashboardPage.tsx:68-96, which uses `useQuery` precisely to allow
|
|
it). Change **only the dashboard loader** to `Promise.allSettled`. The five
|
|
other `Promise.all` loaders stay: their pages use `useSuspenseQuery` and have
|
|
no granular fallback to preserve.
|
|
|
|
### 19. Frontend: the query-key table is complete
|
|
|
|
`web/src/lib/queries.ts` — eight raw literals, all in this file: seven
|
|
`["lookup"]` prefix-invalidations (:99, :125, :132, :159, :167, :189, :211)
|
|
and one `["groups"]` (:133). Add `lookupAll: ["lookup"] as const` to
|
|
`queryKeys`, use it at the seven sites, and use `queryKeys.groups` at :133.
|
|
Acceptance: no `["lookup"]`/`["groups"]` literal outside the table.
|
|
|
|
### 20. Frontend: the form catch swallows only the expected class
|
|
|
|
`web/src/features/blocklists/BlocklistForm.tsx:21-33` — the bare `catch {}`
|
|
wraps both the `await onSubmit` and the reset `setState` calls; anything but
|
|
the mutation rejection dies silently with no console trace. House precedent
|
|
is auth/store.tsx:85-89 (swallow only the 401 it expects, rethrow the rest),
|
|
pinned by two tests. Restructure: `try { await onSubmit(...) } catch (error)
|
|
{ if (error instanceof ApiError) return; throw error; }`, resets after the
|
|
try. The page's `<InlineError>` keeps rendering the mutation error as today.
|
|
|
|
### Carried in from milestone 17: the stale phase-comment sweep
|
|
|
|
Milestone 17 ruling 7 rewrote the "Phase 7/8" headers in three files; its
|
|
repo-wide acceptance grep then found 24 more stale phase references in
|
|
files its sessions did not own, recorded here for this milestone:
|
|
safesearch.zig:58, forward_client.zig:7, dns_cache.zig:10/:42/:435,
|
|
cli.zig:332, filter_integration_test.zig:1391, logger.zig:178/:303,
|
|
retention.zig:7/:137/:151, disk_monitor.zig:7, querylog_schema.zig:71,
|
|
db.zig:272, manager.zig:292/:613/:1145, and the six repository files
|
|
(groups_repo.zig:7, rules_repo.zig:15, sources_repo.zig:9,
|
|
upstreams_repo.zig:7, local_repo.zig:3, clients_repo.zig:12-13). Each
|
|
comment is rewritten to state the as-built truth it gestures at (not
|
|
deleted, unless it says nothing beyond the phase number). Line numbers
|
|
are hints from m17-time HEAD; re-grep before acting. Acceptance: "Phase
|
|
7" and "Phase 8" appear in no source doc comment repo-wide; spec files
|
|
and TECH_DEBT.md are exempt. The session owning each file in the plan
|
|
below picks up its share; files owned by no session fall to S4.
|
|
|
|
Also carried from milestone 17 (ruling 4 residual): the login log line in
|
|
`src/web/handlers/auth.zig` prints the socket peer, which reads as
|
|
loopback for every login behind a trusted proxy; it should print
|
|
`request.client_addr`.
|
|
|
|
## Sessions
|
|
|
|
S1-S4 run in parallel; no two sessions write the same file.
|
|
|
|
### Session S1: backend surface
|
|
|
|
Owns `src/dns/edns.zig`, `tests/fuzz/dns_fuzz.zig`,
|
|
`src/web/handlers/settings.zig`, `src/web/handlers/mutations.zig`,
|
|
`src/web/http_util.zig`, `src/server/cert_store.zig`, `src/app.zig`,
|
|
`src/web/server.zig`. Rulings 1, 2, 8, 9, 10, 11.
|
|
|
|
### Session S2: constants, counters, classification
|
|
|
|
Owns `src/storage/logger.zig`, `src/server/handler.zig`,
|
|
`src/server/clients.zig`, `src/web/handlers/queries.zig`,
|
|
`src/platform/logging.zig`, `src/filter/fetcher.zig`,
|
|
`src/upstream/doh_client.zig`, `src/platform/tls_server.zig`,
|
|
`src/platform/mbedtls_shim.c`. Rulings 3, 4, 5, 6, 7.
|
|
|
|
### Session S3: frontend
|
|
|
|
Owns everything under `web/src/`. Rulings 14-20.
|
|
|
|
### Session S4: tools and deploy
|
|
|
|
Owns `tools/gen_web_assets.zig`, `src/web/static.zig`,
|
|
`deploy/docker/Dockerfile`. Rulings 12, 13.
|
|
|
|
### Orchestrator
|
|
|
|
Re-verifies this spec's line references against post-m18 reality before S1-S4
|
|
start; strikes the closed findings in `TECH_DEBT.md` after.
|
|
|
|
## Module layout
|
|
|
|
New files:
|
|
|
|
- `web/src/features/blocklists/refreshStore.ts` — refresh snapshot store.
|
|
- `web/src/lib/defaultGroup.ts` — default-group constant and helper.
|
|
|
|
Deleted surface: `parseEcs`/`Ecs`/`EcsError`/`ecs_family_*` (edns.zig), the
|
|
`web_dev_dir` global (app.zig), the `queryKeys.blocklistSources` entry
|
|
(queries.ts).
|
|
|
|
## Acceptance (milestone complete)
|
|
|
|
- [ ] `parseEcs` and its family are gone; `zig build test` compiles and
|
|
passes (the dns_fuzz caller was removed in the same change).
|
|
- [ ] `writeSettings` returns `db.Error!void`; the errdefer is the only
|
|
rollback path; the settings PUT integration tests still pass. A fault
|
|
test forces a failure after `Tx.begin` (a fault seam under
|
|
`builtin.is_test`, the milestone-15 rotation-seam shape) and proves
|
|
the errdefer rolled the transaction back: a subsequent `Tx.begin` on
|
|
the same connection succeeds.
|
|
- [ ] Grep proves the field widths exist once: 253/45/32 in their logger
|
|
roles appear only in logger.zig.
|
|
- [ ] One tracker-mutex acquisition per query: `track` returns
|
|
`dropped_full`; `/metrics` output for the tracker family is unchanged.
|
|
- [ ] An oversized log message ends in `...` and increments
|
|
`lines_truncated`; the new test pins both.
|
|
- [ ] Both TLS mapError switches name exactly the two reachable errors; the
|
|
rewritten tests use reachable names only.
|
|
- [ ] The ten mbedTLS constants are verified at init against shim getters;
|
|
a deliberately wrong transcription fails the :657 test block.
|
|
- [ ] `mutations.reload`'s doc comment states the re-read contract and the
|
|
fourteen-call-site consequence.
|
|
- [ ] `WebState.dev_dir` replaces the global; `--web-dev` serving still
|
|
works (manual check with a dev dir).
|
|
- [ ] The key PEM path reads through a fixed buffer and is wiped before
|
|
free; no `readFileAllocOptions` remains on the key path.
|
|
- [ ] `respondBytes` asserts instead of returning OutOfMemory.
|
|
- [ ] gen_web_assets rejects a corrupted shipped sibling and an orphan
|
|
`.gz` (both proven with a doctored dist in a temp dir); the embedded
|
|
test decompresses and compares.
|
|
- [ ] `docker build` (legacy builder, no buildx) on this host produces a
|
|
runnable image; the Dockerfile has no bare `:-amd64` default.
|
|
- [ ] `tsc` fails when a Settings field named in SECTIONS is renamed
|
|
(proven, then reverted).
|
|
- [ ] The settings baseline freezes at mount: a simulated background refetch
|
|
with out-of-band changes produces no phantom patch entries (frontend
|
|
test).
|
|
- [ ] The refresh snapshot survives past gcTime: a frontend test renders the
|
|
status 6+ minutes of fake time after the mutation.
|
|
- [ ] All four group-id sites import `defaultGroup.ts`; RulesPage preselects
|
|
the id-1 group when present (frontend test with a group sorting before
|
|
"default").
|
|
- [ ] The dashboard renders per-widget errors on cold navigation with one
|
|
endpoint failing (frontend test); the other five loaders are
|
|
unchanged.
|
|
- [ ] No `["lookup"]` or `["groups"]` literal outside `queryKeys`.
|
|
- [ ] BlocklistForm rethrows a non-ApiError (frontend test, mirroring the
|
|
logout pair).
|
|
- [ ] Full suite green: `zig build test -Dintegration`, `npm run test`,
|
|
`npm run typecheck`, `npm run lint`.
|
|
|
|
## Recorded (implementation)
|
|
|
|
Accepted deviations and findings from the built milestone.
|
|
|
|
### Ruling 1 (S1)
|
|
|
|
- The edns.zig test block held six `parseEcs` tests, not nine. A seventh
|
|
use lived inside "encodeOpt round-trips through record parse and
|
|
parseOpt", which is not a parseEcs test; its last two lines now compare
|
|
the found option's bytes directly.
|
|
- `extendedRcode` stays, with a nuance the ruling's premise missed: the
|
|
BADVERS production path uses the write-side `splitRcode`; extendedRcode
|
|
is the read-side verifier whose callers are the two BADVERS wire-format
|
|
tests (handler.zig, packet.zig). Deleting it would weaken those tests.
|
|
- The write-fault seam for the rollback test has no exported control
|
|
surface — the test lives in the same file and needs none.
|
|
- The ruling-8 doc comment names `local.zig`'s `publish` function instead
|
|
of a line number, so it cannot go stale.
|
|
|
|
### Rulings 3-7 (S2)
|
|
|
|
- Ownership amendment: `src/web/metrics.zig` (owned by no session) was
|
|
granted to S2 mid-flight. Ruling 5's new counter raises the /metrics
|
|
sample count 29 → 30 (`nxdns_log_lines_truncated_total`); the
|
|
hard-coded assertion became fully derived
|
|
(`1 + dns_stat_fields.len + @typeInfo(LoggerCounters)... +
|
|
@typeInfo(logging.Stats)...`), so a new counter in any of those
|
|
structs extends the exposition and the assertion together.
|
|
- Ruling 6 lands asymmetrically, following the ruling body over the
|
|
stale acceptance bullet: doh_client.zig names eleven members (the two
|
|
collapsed top-level ones plus the nine `Tls*` members of the unwrapped
|
|
read-cause set); fetcher.zig names only the two, because it has no
|
|
cause unwrap and no record-layer member can reach it. Both files carry
|
|
a comment pointing at the other.
|
|
- Addition beyond the ruling: an exact switch does not catch a std
|
|
rename — `error.X` in an expression names a member into existence, so
|
|
a renamed member leaves the switch compiling and matching nothing.
|
|
Comptime guards close this for real: doh_client.zig asserts every
|
|
member of `std.crypto.tls.Client.ReadError` maps to `TlsFailed`, and
|
|
both files assert the two collapsed names still exist in
|
|
`std.http.Client.RequestError`.
|
|
- The ruling's "stashed record-layer cause through the unwrap" test is
|
|
not constructible: the stub connection is `.plain` on purpose, and a
|
|
plain connection's stashed error type has no `Tls*` member. The
|
|
record-layer set is instead tested by an `inline for` over the std
|
|
error set against `mapError` — all nine members, not one sample.
|
|
- Ruling 3 behavioral note: queries.zig's client width dropped 64 → 45
|
|
as directed; a `?client=` filter of 46-64 bytes now returns 400
|
|
instead of matching nothing. `src/filter/wildcard.zig`'s
|
|
`max_pattern_len = 253` is a rule-pattern limit, not a logger role,
|
|
and stays.
|
|
|
|
### Rulings 14-20 (S3)
|
|
|
|
- Ruling 14: the three `Record<string, unknown>` consumption casts
|
|
collapse into one documented cast inside `sectionValues` rather than
|
|
zero — iterating the heterogeneous registry erases the per-section
|
|
correlation TypeScript needs, and `TlsListenerSettings` (a named
|
|
interface, no implicit index signature) blocks the assignment route
|
|
with types.ts off-limits. Four casts before, one after.
|
|
- Ruling 20: the catch body lives in an exported `swallowMutationError`
|
|
helper because jsdom never fires `unhandledrejection`, so a rethrow
|
|
from an async submit handler cannot be asserted directly; the helper
|
|
is tested directly, mirroring how auth/store.tsx's logout pair is
|
|
tested.
|
|
- The refresh store notifies synchronously inside `onSuccess`, one flush
|
|
before react-query's success dispatch; one pre-existing assertion
|
|
moved from `getByText` to `await findByText`. The sub-frame transient
|
|
was judged cheaper than batching through notifyManager.
|
|
- `lib/queries.ts` imports `features/blocklists/refreshStore.ts` (lib →
|
|
features) because the spec directs the mutation factory to write the
|
|
store; recorded as sanctioned layering.
|
|
- `SourceStatusSection`'s prop is `SourceStatus[] | null` to match the
|
|
store; BlocklistsPage tests clear the module-level store in
|
|
`beforeEach`.
|
|
- Ruling 18: post-m18 there are four other `Promise.all` loaders, not
|
|
five.
|
|
|
|
### Rulings 12, 13 and carry-ins (S4)
|
|
|
|
- The undecodable-sibling message reports `decompress.err orelse err`,
|
|
because `std.Io.Reader` collapses every decode failure into
|
|
`error.ReadFailed` and stashes the real one.
|
|
- The Dockerfile keeps its `&&` chain instead of the spec sketch's `;`
|
|
separators, so any failed step still aborts the layer; the case arms
|
|
are untouched.
|
|
- The phase-comment sweep leaves `PLAN.md:605`/`:609` alone — they are
|
|
the section headings that define Phase 7 and Phase 8, not stale
|
|
comments.
|
|
- The orchestrator added the host-fallback sentence to
|
|
docs/how-to/install-with-docker.md.
|
|
|
|
### Residual (named, not fixed)
|
|
|
|
`fetcher.zig` hands the collapsed `ReadFailed`/`WriteFailed` to
|
|
`mapError` with no cause unwrap, so a blocklist download canceled at
|
|
shutdown returns `error.ReceiveFailed` instead of `error.Canceled` — the
|
|
class m18 ruling 4 fixed for DoH. Consequence: a wrong source status and
|
|
a spurious warn line at shutdown. Fixing it needs `req`/`resp` plumbing
|
|
at four call sites; it deserves its own ruling.
|
|
|
|
## Anti-requirements
|
|
|
|
- No deletion of `extendedRcode` (m17 gave it a caller) and no deletion of
|
|
the upstream mutation factories (m17 built their consumer).
|
|
- No locking change in the reload handlers — ruling 8 is documentation.
|
|
- No new frontend dependencies; the store is hand-rolled like restartBanner.
|
|
- No compression plugin for the frontend build; ruling 12 guards the seam,
|
|
it does not start using it.
|
|
- No `useInfiniteQuery` migration and no QueryLogPage work — that was
|
|
milestone 16.
|
|
- No renaming of settings keys or API fields — typing the registry must not
|
|
change the wire format.
|