[dborup]

Relates to #122

Summary

Extends the Compare page with a reference-to-many observer comparison, built on top of the existing two-observer workflow rather than a new page. It is descriptive only: no SNR/RSSI comparison and no health score/classification, matching the issue's explicit MVP scope.

• Server: GET /api/analytics/observer-comparison?ref=&hours=&observers= computes exact packet overlap between one reference observer and up to 40 compared observers, aggregated server-side from a single indexed scan over observations.timestamp — not a 10,000-packet browser sample, and not transmissions.first_seen (so a transmission re-observed inside the window counts even if first seen earlier).
• Frontend: a "One vs. many" mode toggle next to "Two observers" (unchanged). Pick a reference, pick a 1h/6h/24h window, get a sortable table of every other eligible observer with shared/exclusive counts and asymmetric coverage percentages. Clicking a row drills into the existing pairwise detail view with the same window. State is reflected in the URL hash for deep links; the chosen reference is remembered in localStorage.

Plan / what changed

1. Server endpoint (cmd/server/observer_comparison.go)

   • Named response structs only (ObserverComparisonResponse, ..Reference, ..Window, ..Row) — no new map[string]interface{}.
   • Validation before any work (security(api): bound public analytics response caches #430 "reject before work" pattern): ref required, hours ∈ {1,6,24}, observers required/bounded (1–40, must not include ref, raw param length capped before parsing).
   • Visibility: inactive/blacklisted observers are silently excluded from the compared set (same policy GetObservers/IsObserverBlacklisted already apply elsewhere); an unknown/blacklisted reference is a 404.
   • Direction: UPPER(direction) = 'RX' — matched case-insensitively, but NULL/TX/anything else is excluded. This matters in practice: the committed e2e-fixture.db has direction = NULL on every row (it predates that field), so without a seed the endpoint correctly reports "no data" rather than guessing.
   • Dedup: SELECT DISTINCT observer_idx, transmission_id, so a transmission observed via multiple paths by one observer counts once.
   • Scan: one query over the composite (observer_idx, timestamp) index (migration obs_observer_ts_idx_v1, already present), then two linear passes in Go to build per-observer shared/total counts — O(matched rows), not O(observers × packets). A hard observerComparisonScanRowLimit (var, lowerable in tests) fails the request closed with an explicit error rather than silently truncating the aggregate population.
   • Cache: 30s TTL, ceiling-evicted (observerComparisonCache, mirrors channelListCache/nodeAdvertRouteCache), singleflighted per (ref, hours, sorted other ids).
   • Registered in cmd/server/openapi.go (TestOpenAPICompleteness was red until this was added — see CI below).

2. Frontend (public/compare.js, public/style.css, public/app.js)

   • Mode toggle ([data-mode-btn]), bound once on the stable .compare-page root in init() — not re-bound per render, so no listener stacking (fix(ui): stop observers.js leaking delegated handlers on #app (#405) #419/fix(ui): stop compare/channels/live leaking handlers on #app (#428) #437 precedent followed, not re-litigated).
   • Reference/window selects, sortable result table (client-side re-sort of the already-fetched response, no refetch), drill-down links reusing the existing tr[data-action="navigate"] delegated handlers.
   • Stale-response guard: a monotonically increasing request token so a slower, superseded request's response is dropped.
   • pairHours: the existing two-observer view's hardcoded 24h packet-fetch window is now parameterized from ?hours=, so a drill-down from a 1h/6h reference comparison opens the pairwise detail at the same window. Legacy #/compare?a=&b= links carry no hours= and are unaffected.
   • All new colors reuse existing CSS variables (--accent, --text-muted, --space-*, --fs-*, …) — no hardcoded hex.
   • scripts/check-xss-sinks.sh --diff origin/master is clean.

3. Tests

   • Go: cmd/server/observer_comparison_test.go — pure-function table tests (empty/zero/perfect/partial/asymmetric overlap, the issue's own 1000/800/600 → 60%/75% example scaled down and matched exactly), cache TTL/ceiling-eviction/singleflight, request validation, RX/TX/unknown-direction exclusion, dedup across paths, window-uses-observation-timestamp-not-first_seen, exact [start,end) boundaries (at the DB-scan level, so it doesn't depend on wall-clock timing), visibility filtering, v3-schema guard.
   • Go perf (observer_comparison_perf_test.go, AGENTS.md rule 0 proof): 30,000 transmissions / 8 observers / ~90K RX observation rows, full comparison via the real HTTP handler, budgeted well above the measured -race time (CI runs -race for cmd/server) — see "Perf proof" below.
   • Frontend unit: test-issue-122-compare-reference-many.js (sort/format/other-ids helpers, including the "null percentage sorts last in both directions, never as 0" case).
   • E2E: test-issue-122-compare-reference-many-e2e.js, registered in scripts/e2e-shards.json (shard 3) — mode selection, window change + stale-response race, sorting, drill-down, reload/deep-link restore, empty state, narrow viewport. A new fixture seed (test-fixtures/seed-122-observer-comparison.sql, wired into deploy.yml) gives the E2E fixture real (non-null-direction) overlap data, since the committed fixture has none.

Perf proof (AGENTS.md rule 0)

Dataset: 30,000 transmissions, 8 observers (1 ref + 7 compared), ~90,000 RX observation rows (TestObserverComparisonPerf_RealisticFixtureEndToEnd).

	without -race	with -race (CI's cmd/server config)
Real implementation (single bulk scan)	~130ms	~4.7s
Mutant: one query per compared observer (the per-API-call anti-pattern #122 forbids)	—	~10.2s

The committed budget (12s) sits well above the real implementation's measured -race time and below the mutant's, so CI noise can't make a correct change flaky while a regression toward per-observer queries would still fail it. A second test (TestComputeObserverComparison_ScalesLinearlyInObservers) asserts the pure Go aggregation scales linearly in input size as the compared-observer count grows from 4 to 40 at a fixed per-observer workload.

Known limitation

The server endpoint caps compared observers at 40 per request (observerComparisonMaxObservers) and returns a 400 past that, rather than silently truncating. On an install with more eligible observers than that, the page currently can't show all of them in one reference-to-many table — the per-pair detail drill-down still works for any individual pair. Flagging as a known gap, not addressed in this PR.

Not in scope (per issue)

SNR/RSSI delta analysis, automatic health classification/alerting, multi-reference consensus, map overlays, 7-day windows, historical radio-configuration tracking.

[dborup-agent]

Rapport — CS-MacBook PR#450 afslutning — head 8c7970f

Review feedback addressed (commit 8c7970ff)

There were no review comments or findings posted on this PR (checked issue comments, inline comments and reviews), so I audited the #122 requirements myself. Findings, most important first:

1. Query plan was claimed but never verified. The PR relies on the (observer_idx, timestamp) index. That index is built by the ingestor's async migration obs_observer_ts_idx_v1, not by the server. Fix: the scan SQL is now one shared function (observerComparisonScanSQL), and a new test runs EXPLAIN QUERY PLAN on that exact statement and asserts it uses idx_observations_observer_idx_timestamp. Before that migration finishes, the scan falls back to the timestamp index: slower but correct. Documented. [T]
2. Over-long ref / observer ids were not rejected before work. Only the whole observers list was capped. Fix: per-id cap of 128 chars, 400 before any lookup (security(api): bound public analytics response caches #430 "reject before work"). [T]
3. A remembered reference for a vanished observer opened the page on "Error: observer not found". Fix: new resolveRememberedReference. An explicit URL ref is kept (stale shared links still surface the 404); a localStorage-only id is used only while that observer is still eligible. [T]
4. The endpoint was undocumented. observer_comparison.go points at docs/api-spec.md, but no section existed. Added one covering parameters, semantics, limits, cache and index. The issue asks for operational limits to be documented. [T]

Requirement verification

• Overlap computed server-side over the full window, not the 10k browser sample: [T] (Go tests; perf test 30K tx / ~90K RX rows).
• Window [start,end) on observations.timestamp, returned in the response: [T].
• RX only (case-insensitive), NULL/TX excluded, dedup per transmission, nil percentage on a zero denominator: [T].
• Caps and cache follow security(api): bound public analytics response caches #430 (30s TTL, 64-key ceiling eviction, singleflight, hashed long keys, scan row limit fails closed): [T].
• Deep link (mode, ref, hours) and reference remembered in localStorage: [T] (E2E).
• E2E registered in scripts/e2e-shards.json (shard 3) and passing in CI: [T]. Seed wired into deploy.yml: [K].
• Perf: the PR's -race timings were not re-measured by me. I reran the whole suite, including the perf test, under -race locally and in CI. [T]

Disagreements / accepted limits (not changed)

• The 40-observer cap is kept as the PR describes it. The UI shows an explicit "Showing N of M" warning. It is not silent. [K]
• The pairwise drill-down is the existing view. It still fetches at most 10,000 packets per observer and does not filter on RX direction, so its numbers can differ from the server summary on very busy observers. Only the window is aligned. The issue allows the drill-down to be paginated. I did not rework the existing pairwise mode here. [K]
• Scan-limit errors are returned as 500 with an explicit message, as in the PR. I left that unchanged. [K]

Tests (red, then green)

• Go: OverlongIDRejectedBeforeWork and QueryPlanUsesObserverTimestampIndex (new). The first failed to compile before the constant existed.
• Node: resolveRememberedReference unit test (16/16 passing).
• E2E: new step "remembered reference for a vanished observer is dropped" (11/11 against a local Go server on port 13900, with a fixture prepared as in CI plus seed-122-observer-comparison.sql).
• Local suites: sh test-all.sh 250/250; node test-frontend-helpers.js 713/713; go test -race ./... in cmd/server passed. It started before the last test-only refactor, so I reran -run ObserverComparison afterwards, which passed. [T]
• origin/master (288c5b7) merged by merge-commit 519fa66e, clean.

Mutants (one per finding, each caught)

• Finding 1: +observer_idx defeats the index → plan test fails.
• Finding 2: ref check disabled, then observer-id check disabled → the test fails for each.
• Finding 3: resolve returns the stored id unconditionally → both the unit test and the E2E step fail.
• Finding 4 is docs only, so there is no mutant.

CI per job (head 8c7970ff)

• Validate Docker Image: pass. E2E 1/3, 2/3, 3/3: pass. Verify E2E Shards & Coverage: pass. Build & Publish Docker Image: pass.
• Go Build & Test: the first attempt failed with panic: test timed out after 20m0s (running test TestTrackedBytesTracksTheHeap_113, 59s in). That is a suite-wide timeout and not a test failure, and it is not one of the known flakes flaky: #180 packets URL E2E 'reload shows the same list' depends on fixture age (default 15-min window) #271 or test(server): data race between startBackgroundIndexBuilds and PacketStore.Load under -race (TestHashMigrate_LogsMaxWriteLockHold_215) #301. I re-ran the job once and it passed.
• Release Artifacts, Deploy Staging, Publish Badges & Summary: skipped.

PR remains draft. No merge, ready-for-review or issue closing.

[dborup]

Rapport — CS-Minimax PR#450 #122 — head 8c7970f

Dom: APPROVE with nits

Independent, read-only review. Note on the head SHA: this review is of 8c7970ff, not ad4d1939. The branch moved on after ad4d1939 (whose CI run was cancelled by the newer push); 8c7970ff is the current head and was unchanged before and after this review (git ls-remote). The branch is 0 commits behind origin/master (288c5b7e) and already contains a merge of it, so the merge-tree of head onto master (d68a8cb3) is byte-identical to the head tree — everything below therefore applies to both. The PR is a draft; I changed nothing on it.

Evidence tags: [T] test I ran myself · [A] code/diff analysis · [K] manual check against a locally built server.

Findings

#	Severity	Where	What
1	note (CI, mostly pre-existing)	cmd/server	CI attempt 1 of this run failed, and not on the flaky test. go test -timeout 20m -race in cmd/server hit panic: test timed out after 20m0s (FAIL github.com/corescope/server 1200.319s, no --- FAIL line anywhere); the test that happened to be running was the unrelated TestTrackedBytesTracksTheHeap_113 (59 s). Attempt 2 passed with the Go step at 20m57s wall. So the package sits right on its own timeout. I measured this PR's share A/B on one machine, same tree, one variable: with the new perf test 587.1 s, with observer_comparison_perf_test.go removed 571.0 s — +16.2 s (+2.8 %) [T]. The PR nudges an already-marginal budget; it did not create it. Worth an owner decision (raise -timeout, or gate the perf test behind -short), because it will keep producing red first attempts on this package regardless of this PR.
2	nit (test gap)	cmd/server/observer_comparison.go:166	The SQL DISTINCT is unpinned for the compared observers. Dropping DISTINCT from observerComparisonScanSQL passes the entire ObserverComparison suite — TestHandleObserverComparison_DedupPerTransmission included. That test gives only the reference two paths, and the reference is deduped twice over (SQL DISTINCT plus the Go refSet map), so it cannot detect the loss. The compared observers' total[...]++ / shared[...]++ counters have no set of their own, so correctness there rests entirely on DISTINCT. A probe that gives the compared observer three paths for one transmission passes at head and reports Other total = 3, want 1 with DISTINCT removed [T]. Maps to a named acceptance criterion ("Tests cover … repeated observations on different paths"); one extra case closes it. computeObserverComparison's doc comment would also be the right place to state the distinct-input precondition.
3	nit (test gap)	cmd/server/observer_comparison_perf_test.go:123	Nothing asserts that counts exceed the old 10,000-packet cap. The perf test builds 30,000 transmissions / ~90,000 RX rows but asserts only len(resp.Rows) and elapsed time. I probed the real response at that scale: reference.Total = 11250, rows[0].Total = 11250, shared = 7500 [T] — so the behaviour is right, but the criterion "Summary counts include eligible records beyond the existing 10,000-packet cap" has no assertion. A one-line if resp.Reference.Total <= 10000 { t.Fatalf(…) } in the test that already builds the fixture pins it for free.
4	nit (fixture doc)	test-fixtures/seed-122-observer-comparison.sql:21-24	The seed's documented numbers are off by one in the real pipeline. Its header states `
5	note (test gap, trivial)	cmd/server/observer_comparison.go:198	The scan row limit's exact trip point is not pinned: >= → > survives, because TestScanObserverComparisonPairs_ScanRowLimit lowers the limit to 3 and inserts 5 rows, so both forms error [T]. Immaterial at the real 1,000,000 limit; listed only for completeness.
6	note (deployment)	cmd/server/routes.go:335	No feature flag. The endpoint is registered unconditionally and the UI mode toggle is always present. The issue's wording — "Inspect query plans and benchmark realistic multi-million-observation datasets before enabling this on a live instance" — reads as expecting a deliberate enabling step, and there is none. Defensible: it is a bounded read-only analytics read (≤40 observers, ≤24 h, 1 M-pair cap, 30 s cache, singleflight) and every other /api/analytics/* route also ships unflagged. Flagging it in case a gate was expected.
7	note	docs/api-spec.md	The hardcoded limits (40 observers, 1,000,000 pairs, 30 s TTL, 64 cache keys, the {1,6,24} window set) are documented in docs/api-spec.md and the OpenAPI description, but not named as customizer candidates anywhere, which the issue's "track customizer exposure where appropriate" asks for. Also: exceeding the 1 M-pair cap returns 500. It is explicit and documented, which satisfies "return an explicit limitation/error", but a client-correctable condition ("narrow the window") would read better as 400/413.

Nothing here blocks the PR. Findings 2–5 are missing tests and a stale comment around code that behaves correctly at head.

Answers to the review points

Was the a11y test weakening legitimate? test-issue-1644-redesign.js is the only pre-existing test this PR modifies: both regexes gained a data-cview requirement so they no longer match the new data-mode-btn tablist. I checked the worrying reading — new markup carved out of an existing guard with no replacement coverage — and it does not apply. The mode toggle carries role="tablist", role="tab", aria-controls="compareControls compareContent" and aria-selected; renderModeToggle() keeps aria-selected in sync with .active; and the new E2E asserts aria-selected on it both at load and after a drill-down [A]/[T]. The un-narrowed regex would have demanded aria-controls="compareDetail" on a toggle that controls different panels — the wrong assertion. The narrowing is correct. One residual gap: the mode toggle has no roving tabindex, unlike the detail tabs.

Exact and explicit overlap semantics. SELECT DISTINCT observer_idx, transmission_id … WHERE observer_idx IN (…) AND timestamp >= ? AND timestamp < ? AND UPPER(direction) = 'RX', then two linear passes in Go — O(matched rows), with no per-observer re-scan [A]. [start, end) over observations.timestamp, never transmissions.first_seen; the effective interval is echoed in window [T]/[K]. Each transmission_id counts once per observer. TX, NULL and any unknown direction are excluded; "RX" matches case-insensitively. referenceOnly = |R|-|S|, otherOnly = |O|-|S|, both percentages one-decimal, and each is null rather than 0 when its denominator is zero [T]. No silent fallback to an older window.

Beyond the 10,000-packet cap, independent of pagination. There is no pagination and no packet-list cap anywhere in the path; the only bound is the 1 M-pair backstop, which fails the request closed instead of truncating [A]. Measured at scale: 11,250 unique transmissions for the reference through the real HTTP handler [T] — see finding 3 for the missing assertion.

Validation before work (#430). ref required and length-capped, hours whitelisted to {1,6,24}, observers required, raw-length-capped before splitting, per-id length-capped, de-duplicated, rejected if it contains ref, and capped at 40 — all before any cache lookup, DB query or singleflight [A]/[T]. Verified live: a missing ref is 400 with {"error":"ref is required"} [K].

Visibility. Compared observers that are unknown, inactive (inactive IS NULL OR inactive = 0) or blacklisted are silently excluded; an unknown or blacklisted reference is 404; all-ineligible is 400 [T].

Perf and query plan. TestScanObserverComparisonPairs_QueryPlanUsesObserverTimestampIndex asserts SEARCH on idx_observations_observer_idx_timestamp, and its DDL is identical to what the ingestor's async migration obs_observer_ts_idx_v1 creates (cmd/ingestor/db.go:190) — so the pinned plan is the production plan, not a test fiction [A]/[T]. The documented pre-migration fallback also checks out: idx_observations_timestamp exists in the base schema [A]. Measured on the 30,000-transmission fixture: 137 ms without -race and 4.98 s with, against a 12 s budget; the linearity test reports 4 others: 224 ms; 40 others: 2.50 s (linear prediction 2.24 s, budget 6.71 s) [T]. The PR body's ~130 ms / ~4.7 s figures reproduce.

Cache. Keyed by (ref, hours, sorted other ids) — observer-controlled, so not enum-keyed and therefore correctly subject to the #430 eviction cap: 64 entries, expired evicted first then nearest-expiry, with a SHA-256 fallback past 512 key bytes and singleflight collapsing concurrent misses [A]/[T].

Read-only and typed. The endpoint only SELECTs over the mode=ro handle; no schema or write SQL is added to cmd/server [A]. No new map[string]interface{} outside tests: 0 [A]. Three new bare interface{} lines exist, all forced by signatures — two []interface{} SQL-arg slices and one singleflight callback — and []interface{} is the established convention here (89 occurrences in cmd/server vs. 1 of []any) [A].

Frontend. All new colours use existing tokens (--accent, --text, --text-muted, --space-*, --fs-*, --fw-semibold, --mono) — no hardcoded hex [A]. Every node-controlled field in the new markup goes through escapeHtml, including observerName, the reference name, the drill-down href and the title attribute, which matters because the gate has a documented blind spot for row-builder functions [A]. The mode toggle is bound once on the stable .compare-page root rather than per render, and destroy() bumps the request token so an in-flight response cannot land on a later page [A].

Invariants. Fork guards at head: deploy.yml 9, release-fast-path.yml 1 — unchanged, even though deploy.yml is modified (the diff adds only the #122 seed step) [A]. scripts/check-xss-sinks.sh --diff origin/master run against the two changed public/*.js files in an isolated scratch repo: clean, exit 0 [K]. No closing keywords in the PR body or any commit message [A]. All three commits carry the expected author name and address [A].

The prior review. There is no review on this PR on GitHub — no reviews, no inline comments, no issue comments [A] — so I could not check the nits against it. The follow-up commit 8c7970ff names four fixes in its subject and all four are verifiable in the tree: the id-length bound (observerComparisonIDMaxLen plus TestHandleObserverComparison_OverlongIDRejectedBeforeWork, whose comment is literally labelled "PR #450 review"), the stale remembered reference (resolveRememberedReference plus its unit test), the pinned query plan, and the endpoint documentation (docs/api-spec.md, +43 lines) [A]/[T].

Tests and mutants

Head = merged tree (the branch already contains master), Go 1.26.0, darwin/arm64:

Suite	Result
cmd/server go test ./...	ok, 52 s [T]
cmd/server go test -race ./... (full package)	ok, 587 s [T]
New tests + OpenAPI under -race	45 tests, all pass, no data race [T]
sh test-all.sh	250 passed, 0 failed [T]
node test-frontend-helpers.js	713 passed, 0 failed [T]
node test-issue-122-compare-reference-many.js	16 passed, 0 failed [T]

E2E against a locally built server on a CI-prepared e2e-fixture.db (freshen, the grouped-packet seed SQL, corescope-migrate, seeds 2073, 199 and the new 122), on a port I checked was free; stopped by port lookup and pid [K]:

Suite	Result
test-issue-122-compare-reference-many-e2e.js	11 passed, 0 failed [T]
test-e2e-playwright.js (core)	154/157 passed, 3 skipped — identical to the baseline, no regression [T]

Live endpoint checks against the seeded fixture [K]: a valid 24 h request returns the full typed body with both asymmetric percentages; ref missing → 400; and the response shape matches docs/api-spec.md exactly.

CI. Run 38565563716, head 8c7970ff, final conclusion success — Go Build & Test, all three Playwright shards, Verify E2E Shards & Coverage, Validate Docker Image and Build & Publish Docker Image all pass; Badges / Deploy Staging / Release Artifacts skipped as expected on a PR. But that is attempt 2: attempt 1's Go Build & Test failed on the 20-minute package timeout described in finding 1, which also failed Build & Publish Docker Image behind it. The failure is neither of the two known-flaky tests nor an assertion failure of any kind — it is a whole-package timeout. I did not re-run anything; attempt 2 already existed when I started.

My own mutants. 12 applied to the head tree, each reverted from a pristine copy afterwards. 10 killed, 2 survived.

Mutant	Result
drop AND UPPER(direction) = 'RX'	killed (RXOnly: total 5, want 2)
UPPER(direction) → direction (case-sensitive)	killed (RXOnly: total 1, want 2)
timestamp < ? → <=	killed (ExactBoundaries)
drop SELECT DISTINCT	SURVIVED → finding 2
ReferenceOnly: refTotal - s → t - s	killed (PartialAsymmetric)
row limit >= → >	SURVIVED → finding 5
cache key drops hours	killed (DifferentHoursDiffer)
if t > 0 → if t >= 0 (serve NaN instead of null)	killed (EmptySets, OtherTotalZero_PctNil)
window always −24 h	killed (WindowUsesObservationTimestamp, WindowEchoed)
drop the result sort	killed (MultipleOthersSortedByName)
drop inactive IS NULL OR inactive = 0	killed (InactiveObserverExcluded)
drop the compared-observer blacklist check	killed (BlacklistedObserverExcluded)

For the meaningful survivor I wrote the missing test to prove it is a real gap and not an equivalent mutant: it is green at head and red with DISTINCT removed (Other total = 3, want 1).

Left for the owner

• cmd/server is at its CI timeout (finding 1). The first attempt of this very run died on it. Raising -timeout or gating the heaviest perf tests behind -short is an owner call, and it affects every PR touching this package, not just this one.
• The 40-observer ceiling vs. the first acceptance criterion. The author flags this as a known limitation, correctly. The endpoint 400s rather than truncating — the right failure mode — but on an install with more than 40 eligible observers the reference-to-many table cannot show them all, so "compare the reference with other eligible observers in a single table" is unmet at that scale. The per-pair drill-down still works for any individual pair.
• No feature flag and no customizer exposure for the new hardcoded limits (findings 6 and 7).
• Benchmarking at "realistic multi-million-observation" scale is not done. The committed evidence is a 30,000-transmission / ~90,000-row fixture, which is a sound bounded-growth proof but two orders of magnitude below what the issue names. Worth a measurement on a real instance before enabling it there.

What I did not verify

• Behaviour at multi-million-observation scale, on real hardware or a production-sized database.
• The three skipped core-E2E tests (skipped by design on a local fixture) and the mobile/VCR suite.
• Cross-browser rendering: the E2E ran Chromium only, as CI does.
• I did not re-run any CI job, did not merge, did not mark the PR ready, and used no staging or production access and no server API key.
• Whether the pre-existing cmd/server timeout margin was already failing attempts before this PR — I measured this PR's contribution (+2.8 %) but did not run master's full -race package time on a CI-equivalent runner.

[adminopenclaw8-sketch]

Review — CS-Macmini PR#450 — head 8c7970f

Dom: APPROVE med nits

This is a follow-up commit (8c7970ff) on top of ad4d1939/519fa66e, addressing four issues the author self-identified against #122 in the PR's own Rapport comment (no separate reviewer had posted findings on this PR, so the self-audit is the "earlier review" being closed out here). All four points were checked against #122 and the fixture-backed behaviour, independently, on a local server.

Fund-tabel

#	Punkt	Rød→grøn test?	Verificeret	Dom
1	Query-plan var påstået, ikke bevist	Ja — TestScanObserverComparisonPairs_QueryPlanUsesObserverTimestampIndex	[T] Mutant: +observer_idx in the shared SQL → plan flips to SEARCH … USING INDEX idx_observations_timestamp + TEMP B-TREE → test fails as expected	OK
2	Over-lange ref/observers-id'er blev ikke afvist før arbejde	Ja — TestHandleObserverComparison_OverlongIDRejectedBeforeWork	[T] Two mutants (drop ref check, drop per-id check in parseObserverComparisonObserverIDs) each fail the same test with the expected 404-instead-of-400 symptom; manual curl against the live server reproduces both 400s	OK
3	Glemt reference til en forsvundet observer viste "Error: observer not found"	Ja — Node unit test + new E2E step	[T] Mutant on resolveRememberedReference (drop eligibility check) fails both the unit test and the E2E step with the exact pre-fix symptom (Error: observer not found)	OK
4	Endpointet var udokumenteret	Docs only, no mutant expected	[T] docs/api-spec.md section added; params/limits/semantics/cache/index match the code exactly (128-char id cap, 1–40 observers, 4096-char raw cap, 1,000,000-row scan limit, 30 s/64-key cache)	OK

Svar på punkterne (#122 krav, gennemgået enkeltvis)

• Full-window server-side overlap, ikke 10k-sample: confirmed — scan is one SELECT DISTINCT observer_idx, transmission_id over observations within [start,end), aggregated in Go. [T] (Go tests + direct curl against the seeded fixture reproduced the issue's own 60%/75% and the seed's second pair 30%/100% exactly.)
• [start,end) on observations.timestamp, not first_seen, returned in response: confirmed in both code and a live response body. [T]
• RX only, case-insensitive, NULL/TX excluded, dedup per transmission, null% on zero denominator: confirmed by the Go table tests; this was already true before the follow-up commit and untouched by it. [T]
• Caps/cache/security(api): bound public analytics response caches #430 pattern (reject-before-work, 30 s TTL, 64-key ceiling, singleflight, scan-limit fails closed): confirmed — the follow-up commit specifically closes the one remaining "reject before work" gap (per-id length). [T]
• Deep link + localStorage-remembered reference: confirmed, including the fix — a stale localStorage id is now dropped when the observer is no longer eligible, while an explicit URL ref is kept (so a shared stale link still surfaces the 404, by design). [T] E2E.
• Two-observer comparison still works: ran the existing test-issue-1640-compare-discovery-e2e.js (11/11) against the local server with this head checked out — pairwise discovery, selection, legacy deep links and drill-down all still pass. [T]
• Known 40-observer cap: server returns 400 past 40 (no silent truncation); when the eligible population exceeds what fits the request, the frontend renders an explicit "Showing N of M eligible observers" warning (public/compare.js:925), not a silent gap. Matches the PR's own "known limitation" framing. [T]

Scope creep

None found. The follow-up commit touches exactly five files: the server validation/SQL-sharing, one Go test file, one doc file, compare.js, and the two #122 test files. The one non-obvious cross-cutting change is .github/workflows/deploy.yml gaining an 8-line seed step for #122 E2E data — but that's from the first commit (ad4d1939), not the follow-up, and it's the same mechanical pattern as the existing #245/#2073/#199 seed steps already in that job. No new config surface, no behavior change beyond the four stated fixes.

Tests og mutanter kørt af mig

• Merged tree built against origin/master dd003389 via git merge-tree --write-tree → 9ec381d5. All checks below ran against that tree in a fresh worktree (_wt-pr450-review, head 8c7970ff), not against anyone else's checkout.
• go test -race ./... in cmd/server: PASS (450.6 s). go test ./... in cmd/ingestor: PASS (106.8 s).
• sh test-all.sh: PASS, 0 failures (46 suites, 10 skips — all the known macOS setsid gap documented in an earlier PR's follow-up, unrelated to this PR).
• node test-frontend-helpers.js: 713/713 PASS.
• node test-compare-overlap.js, node test-compare-flood-filter.js: PASS (existing pairwise helpers, untouched).
• Local E2E against a real Go server (port 13900, fixture freshened → migrated → seeded with seed-2073/seed-199/seed-122, frontend instrumented as in CI):
  • test-issue-122-compare-reference-many-e2e.js: 11/11 PASS.
  • test-issue-1640-compare-discovery-e2e.js (pairwise/legacy): 11/11 PASS.
  • Manual curl against the live endpoint reproduced both the over-long-id 400s and the exact overlap numbers from the fixture seed.
• Mutants (4, one per finding + one cross-check), each run against the real test suite, each caught, each reverted cleanly (verified with git diff = clean on the mutated files afterward):
  1. Drop the ref-length check → TestHandleObserverComparison_OverlongIDRejectedBeforeWork fails (404 instead of 400).
  2. Drop the per-id length check in parseObserverComparisonObserverIDs → same test fails on the observer case.
  3. +observer_idx in observerComparisonScanSQL (classic index-defeat) → TestScanObserverComparisonPairs_QueryPlanUsesObserverTimestampIndex fails, plan shows SEARCH … USING INDEX idx_observations_timestamp + TEMP B-TREE.
  4. resolveRememberedReference reverted to unconditionally returning the stored id → both the Node unit test and the E2E step fail with the exact pre-fix symptom (Error: observer not found).
• CI, head 8c7970ff: all jobs green — Go Build & Test, Playwright E2E 1/3–3/3, Verify E2E Shards & Coverage, Validate Docker Image, Build & Publish Docker Image all pass; Release Artifacts/Deploy Staging/Publish Badges skipping as expected for a draft. No flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256/flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267 flakes present in this run.

Repo-invarianter

• cmd/server stays read-only: no INSERT/UPDATE/DELETE/Exec in observer_comparison.go; the index migration obs_observer_ts_idx_v1 lives only in cmd/ingestor (async_migration.go, db.go) — the server only references its name. [T]
• No new map[string]interface{} outside tests anywhere in the full PR diff (288c5b7e..8c7970ff). [T]
• No hardcoded colors: the new CSS block in public/style.css uses only existing var(--...) tokens; scripts/check-xss-sinks.sh --diff origin/master exits 0 with no output in the worktree. [T]
• Commit authors across all three PR commits are dborup <kontakt@meshview.dk>. [T]
• No closing keywords (closes/fixes/resolves #...) in any commit message in the PR range. [K]
• "Fork-guards 9/1" — I could not map this term to anything in this diff: no .github/workflows fork-PR guard pattern (head.repo.full_name etc.) exists anywhere in the repo's workflows today, and this PR's only workflow change is the unrelated 8-line #122 seed step. Flagging as not verified rather than guessing at an unrelated interpretation.

Hvad jeg ikke har verificeret

• The PR's own -race perf numbers (130 ms / 4.7 s / 10.2 s at 30k tx / 90k obs) were not independently re-measured at that scale; I verified correctness and the index usage, not the specific timing figures.
• Staging/production query-plan behavior once obs_observer_ts_idx_v1 is actually live on a multi-million-row database — only the local fixture-scale plan was checked, as prescribed (no staging access).
• CI's own cached run artifacts (coverage %, exact shard timing) beyond the pass/fail status surfaced by gh pr checks.

[dborup-agent]

Rapport — CS-MacBook PR#450 master-sync — head 1051237

Review feedback addressed (commit 10512375)

1. Merge conflict with master (feat(packets): Full Names toggle and better path popover (port of upstream 2090) (#217) #449 Full Names and other merges). origin/master (c511966a) merged into the branch with a plain merge commit (no rebase, no force-push). The only conflict was scripts/e2e-shards.json. Both entries are kept: test-issue-122-compare-reference-many-e2e.js (shard 3) and test-issue-217-packets-full-names-e2e.js (shard 1). The JSON parses. The other auto-merged files (deploy.yml, openapi.go, routes.go, style.css, test-all.sh) needed no manual edits. [T]
2. Review nits. The Macmini review was APPROVE with no actionable nits. I have nothing to fix under Rester except the item below.

Tests (local, merged tree)

• go test -race ./... in cmd/server: ok (428.7 s). [T]
• sh test-all.sh: 252 passed, 0 failed. [T]
• node test-frontend-helpers.js: 713 passed, 0 failed. [T]
• node test-ci-shards.js: 9/9 pass. [T]
• E2E against a local Go server (port 13900) on a fixture prepared as in CI (freshen, migrate, seeds 2073/199/245/122, instrumented frontend):
  • test-issue-122-compare-reference-many-e2e.js: 11/11. [T]
  • test-issue-1640-compare-discovery-e2e.js: 11/11. [T]
  • test-issue-217-packets-full-names-e2e.js: 48/48. [T]

Mutants

• No review findings, so no per-finding mutants. I did one on the conflict resolution: I removed the Evaluate upstream packet-table sizing, Full Names and path popup improvements (2090) #217 or the Feature request: Reference-to-many observer comparison with complete-window packet overlap #122 entry from the shard manifest. test-ci-shards.js stays green in both cases, so it does not guard manifest completeness locally. Only the CI job "Verify E2E Shards & Coverage" does, and it passed on the resolved manifest. [T]

CI per job (head 10512375)

• Pass: Go Build & Test, Go Ingestor Tests, Go Libraries & CLI, Go Server Tests 1/3, 2/3 and 3/3, JavaScript Unit Tests & Lint, Script & Container Unit Checks, Playwright E2E 1/3, 2/3 and 3/3, Validate Docker Image, Build & Publish Docker Image, Verify E2E Shards & Coverage, Verify Go Test Assignments. [T]
• Skipped as expected: Publish Badges & Summary, Release Artifacts, Deploy Staging.
• No flaky-test reruns needed (flaky: #180 packets URL E2E 'reload shows the same list' depends on fixture age (default 15-min window) #271 and test(server): data race between startBackgroundIndexBuilds and PacketStore.Load under -race (TestHashMigrate_LogsMaxWriteLockHold_215) #301 did not occur).

Rester

• The Macmini review mentioned "Fork-guards 9/1" but could not map it to anything in the diff. I could not map it either and did not guess. [K]
• The disagreements from the previous report still apply: the 40-observer cap, the pairwise drill-down still capped at 10,000 packets with no RX filter, and scan-limit errors returned as 500. [K]

PR remains draft. No merge, ready-for-review or issue closing.