[dborup-agent]

Relates to #393

Non-blocking follow-ups from the round-3 re-review of #373 and the round-2 re-review of #394. Each concrete point gets a red-before / green-after test and a mutant shown red. perf.js is test-only: the sinks were already escaped, and these tests close the coverage gap. The only cmd/server change is one test file, perf_slowpath_392_test.go, which pins N4. No server code changed.

What changed

#	Point	Fix	Test (red→green)	Mutant
N6 (a11y)	analytics.js:1105 repeater <div> had role="row" with no table/grid ancestor (11 critical axe violations)	role="row" → role="link", keep tabindex="0"	test-xss-escape-sinks.js "#393 N6 analytics repeater row: a standalone interactive role, no role needing a context". It allow-lists link/button and rejects every WAI-ARIA role that needs a context role (round 2, F4).	re-add role="row", or role="cell", or drop the role → red
N7 (test gap)	delegated selector [data-action="navigate"] (broadened in #373) was unpinned	new test drives a DIV row through the real handler	"#393 N7 analytics delegated handler: a DIV row navigates on click"	narrow selector back to tr[...] (M4) → red
N9 (onclick)	compare.js:529 kept a per-row inline onclick (latent '-breakout in the double-quoted/JS context)	drop the inline onclick; add a delegated click handler on #compareContent, the page's own container (round 2, F1)	"#393 N9 … no inline on* handler on the row or its children" + "… delegated click handler navigates" + "#393 F1 … no click listener on the shared #app" + E2E in test-issue-1640-compare-discovery-e2e.js	re-add onclick on the <tr> or a child <td> / remove the handler / bind it to app → red
N10 (guard)	loadLocalEsc counted only function esc(; a const esc = … second escaper slipped past	countEscDefs now counts function/const/let/var forms	"#393 N10 countEscDefs catches a second, differently-declared esc"	weaken guard to function-only → red
N1 (docs)	api-spec.md documented the slow-path bound as ≤257 runes and "≤128-rune-per-half" (both ignore the per-half ellipsis)	≤259 runes (2×128 + two "…" + the join space); per-half wording fixed	new test-issue-393-slowpath-doc-bound.js pins the doc against slowPathRawCap	revert doc to 257 → red
N3 (comment)	check-xss-sinks.sh header claimed the generic locals path/q.path/k "cannot be added without a false-positive explosion" — empirically one extra finding, not a flood	soften the wording; the accumulator shape is the real blind spot (gate logic untouched, allowlist unchanged)	— (prose; --diff origin/master stays clean)	—
N4 (test gap)	DEL (0x7f) half of the control-char strip in sanitizeSlowPathComponent was unpinned	pinned in cmd/server/perf_slowpath_392_test.go (round 2, F2)	"DEL is stripped from both halves, '~' is kept"	r < 0x20 || r == 0x7f → r < 0x20 → red
N5 (test gap)	the issue named ~30 newly escaped perf cards; only 5 had a test	drive every escapeHtml-wrapped perf-num card: 42 cards in 7 groups (round 2, F3)	"#393 F3 perf.js: every listed card wraps its perf-num value in escapeHtml" + one hostile-value test per card	drop escapeHtml on any card (e.g. sq.rows.nodes, c.hitRate) → red
N6 (#394, test gap)	escapeHtml(err.message) at perf.js:374 had no test	add a load-error sink test	"#393 N6 perf.js error paragraph: err.message is escaped"	drop escapeHtml(err.message) (M6) → red

Round 2 (review on this PR)

• F1: the N9 click handler was bound to the shared #app element and outlived the compare page. After one compare visit, an observers row click at ≤1023px navigated instead of opening the [#1050] Task 3: Packets/Nodes/Observers tables — fluid columns + +N hidden pill Kpa-clawbot/CoreScope#1056 AC#4 slide-over. It is now bound to #compareContent, which the next page's render discards. The pre-existing fix(store): charge the path_json fallback and pin the hash-migration accounting (#202) #209 keydown handler in compare.js is still on #app. It has the same leak for Enter/Space, but this PR did not add it, so it is left for the owner.
• F2: the earlier reason for not pinning N4 was wrong. The cmd/server read-only invariant (bug(db): vacuumOnStartup fails with SQLITE_BUSY when ingestor + server share DB (auto_vacuum migration #919 broken in single-container topology) Kpa-clawbot/CoreScope#1283) covers DB writes and does not forbid a _test.go file. N4 is now pinned (see the table).
• F3–F5: these were test-strength fixes, as listed in the table.

Left for the owner

• N2: the fix(ui): escape perf dashboard values (#392) #394 PR description is stale. The round-1 body no longer matches the merged code: numeric metrics are now escaped, both halves are capped, and the test counts differ. This is the description of an already-merged PR, not a repo artifact, and editing another PR is out of scope for this branch. It is left for the owner to refresh if desired.
• The compare keydown handler on #app (see F1 above).
• Gate policy: test-a11y-axe-1668.js runs a rule whitelist without aria-required-parent/aria-required-children. It therefore reports 0 violations on #/analytics?tab=topology on master too, and it is not evidence for N6. The unit test above is what pins N6. Whether to add those two rules to the gate is the owner's call.

Verification

Round 2, on the merged tree with origin/master 23f50b4f:

• node test-xss-escape-sinks.js → 121/0.
• node test-frontend-helpers.js → 712/0.
• sh test-all.sh → 238/238.
• cmd/server go test ./... → ok.
• scripts/check-xss-sinks.sh --diff origin/master → exit 0.

E2E ran against a local Go server with e2e-fixture.db prepared as in CI:

Test	Result
test-issue-1640-compare-discovery-e2e	11/0 (incl. 2 new F1 steps)
test-slideover-1056-e2e	27/0
test-xss-333-pages-e2e	11/0
test-xss-333-e2e	8/0
test-xss-392-perf-e2e	1/0
test-issue-236-analytics-sorting-e2e	pass
test-issue-205-analytics-subtab-deeplinks-e2e	15/0

No new map[string]interface{}, no hardcoded colours, and no workflow changes.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-MacBook PR#402 #393 — head d378285

Status: Implemented and self-verified; draft open, all CI jobs green; no merge/ready/approve actions taken; #271 flake not observed (no rerun needed).

Evidence tags: [T] = I ran it. [A] = from reading the code. [K] = from CI.

Scope recheck

Rechecked current master ce29f5dc (= merge of #394). Issue body points map to the #373 round-3 re-review (N6/N7/N9/N10); the comment maps to the #394 round-2 re-review (N1–N6). #373's N8 (raw perf.js sinks) is explicitly tracked as a separate security issue and is out of scope. cmd/server has no .go change in this diff. [A]

Per-requirement evidence (test red→green + mutant)

#	Point	Fix	Test (red before → green after)	Mutant
N6	analytics.js:1105 repeater <div> carried role="row" with no table/grid ancestor (11 critical axe violations)	role="row" → role="link", tabindex="0" kept	test-xss-escape-sinks.js "#393 N6 … no role=row on the bare <div>"	[T] re-add role="row" → red
N7	delegated [data-action="navigate"] selector (broadened in #373) was unpinned	new test drives a DIV row through the lifted real handler	"#393 N7 … a DIV row navigates on click"	[T] narrow selector to tr[...] (M4) → red
N9	compare.js:529 kept a per-row inline onclick (latent '-breakout in the double-quoted/JS context)	drop inline onclick; add a delegated click handler beside the #209 keydown handler	"#393 N9 … no inline on* handler" + "… delegated click handler navigates"	[T] re-add onclick → red; [T] remove handler → red
N10	loadLocalEsc counted only function esc(; a const esc = … 2nd escaper slipped past	countEscDefs counts function/const/let/var	"#393 N10 countEscDefs catches a second, differently-declared esc"	[T] weaken to function-only → red
N1	api-spec.md slow-path bound documented as ≤257 runes + "≤128-rune-per-half" (both ignore the per-half ellipsis)	≤259 runes (2×128 + two "…" + join space); per-half wording fixed	new test-issue-393-slowpath-doc-bound.js pins the doc against slowPathRawCap	[T] revert doc to 257 → red
N3	check-xss-sinks.sh header claimed generic locals path/q.path/k "cannot be added without a false-positive explosion" — empirically 1 extra finding, not a flood	soften wording; accumulator shape is the real blind spot; gate logic + allowlist untouched	— prose only; --diff origin/master stays clean	— (not behaviourally testable)
N5	only the Go-runtime health cards were covered; the Node-branch cards (m.rss, el.*) were not	add perf.js Node-branch card tests	"#393 N5 health(node): m.rss / el.maxLagMs / el.currentLagMs"	[T] drop escapeHtml(m.rss) (M5) → red
N6 (#394)	escapeHtml(err.message) at perf.js:374 had no test	add a load-error sink test	"#393 N6 perf.js error paragraph: err.message is escaped"	[T] drop escapeHtml(err.message) (M6) → red

All 8 mutants verified red; all suites green after restore. perf.js is test-only (sinks already escaped — N5/N6 close the coverage gap, not a code fix).

Left for the owner

• N2 — fix(ui): escape perf dashboard values (#392) #394's PR description is still the round-1 text and no longer matches the merged code. It is the body of an already-merged PR, not a repo artifact, and editing another PR is out of scope for this branch. Left for the owner to refresh.
• N4 — the DEL (0x7f) half of the control-char strip in sanitizeSlowPathComponent (cmd/server/routes.go:1430) is unpinned. Pinning it needs a Go test in cmd/server (the function is unexported in package main, unreachable elsewhere), and this issue keeps cmd/server read-only. Ready-to-apply pin: TestSanitizeSlowPathComponentStripsDEL asserting sanitizeSlowPathComponent("a\x7fb") == "ab"; mutant if r < 0x20 || r == 0x7f → if r < 0x20 then goes red. Not applied here to honour the read-only rule.

CI per job (run 38356292602, head d378285) [K]

Job	Result
Go Build & Test	✅ pass 23m14s
Playwright E2E Tests	✅ pass 27m17s
Build & Publish Docker Image	✅ pass 1m02s
Release Artifacts / Deploy Staging / Publish Badges	skipped (fork guard)

No #271 flake appeared; no rerun needed.

Local verification

• [T] node test-xss-escape-sinks.js → 77/0; node test-issue-393-slowpath-doc-bound.js → 3/0; node test-frontend-helpers.js → 712/0.
• [T] sh test-all.sh → 236 passed, 1 failed (237 files). The one failure, test-backup-sqlite-safety.js, is the macOS "missing tool setsid" environmental gap (9 signal-group cases, Linux-only); unrelated to this diff, which does not touch the backup script. CI (Linux) runs it green.
• [T] cmd/server go test ./... → ok (no Go changes).
• [T] scripts/check-xss-sinks.sh --diff origin/master → exit 0, scanning the changed public/*.js (run in the real worktree against the committed tree).
• [T] E2E against a local Go server with the CI-prepared e2e-fixture.db on a free high port: test-xss-333-pages-e2e 11/0, test-xss-333-e2e 8/0, test-xss-392-perf-e2e 1/0, test-a11y-axe-1668 zero violations across 120 cells, test-issue-1640-compare-discovery-e2e 9/0, test-issue-236-analytics-sorting-e2e pass, test-issue-205-analytics-subtab-deeplinks-e2e 15/0.
• [T] Live browser probe of #/analytics?tab=topology: the 11 repeater rows are <div role="link" tabindex="0"> (no role="row") and a click navigates to #/nodes/<pubkey>. The single console error during the probe is a 404 on the destination node page fetching fixture-absent data — unrelated to the fix.

Checklist

Item	Result
Every point has a red-before/green-after test	Yes [T], except N2/N4 (left for owner) and N3 (prose-only comment fix)
At least one mutant per point	8 mutants, all red [T]
cmd/server read-only, no new map[string]interface{}	No .go change; 0 new map[string]interface{} in the diff [T]
No hardcoded colours	None added (only #NNN issue refs) [T]
scripts/check-xss-sinks.sh --diff origin/master	exit 0 [T]
Fork guards	9 in deploy.yml, 1 in release-fast-path.yml — unchanged [T]
Closing keywords	none; closingIssuesReferences empty; body opens "Relates to #393" [T]
Commit author/committer	dborup <kontakt@meshview.dk> [T]
Scope	7 files: 2 public/*.js, 1 doc, 1 gate comment, 2 test files + 1 new test, test-all.sh registration [T]

[dborup-agent]

Review — CS-pve-agent2 PR#402 — head d378285

Dom: REQUEST CHANGES — the N9 delegated click handler is attached to the persistent #app element and outlives the compare page. That causes a user-visible regression on the observers slide-over (Kpa-clawbot#1056 AC#4). The fix is small and verified locally. Everything else is APPROVE with nits.

Evidence tags: [T] = I ran it, [A] = from reading the code, [K] = from CI or earlier review threads.

Findings

#	Severity	Where	Finding
F1	blocking (regression)	public/compare.js:125	The new app.addEventListener('click', …) binds to #app. That is the shared SPA container (app.js passes document.getElementById('app') to every page's init). destroy() never removes it, so after one visit to #/compare it fires on every later page for any tr[data-action="navigate"]. On #/observers at ≤1023px, the observers handler opens the slide-over and calls preventDefault() instead of navigating (Kpa-clawbot#1056 AC#4). The leaked compare handler then sets location.hash anyway. Browser probe at 800×800 on the fixture [T]: going home → observers and clicking a row leaves the hash at #/observers on both master and the PR. Going home → compare → observers and clicking a row gives #/observers on master and #/observers/D5EF2ED7… on the PR. It also adds one more listener on every compare visit (unbounded). The old inline onclick only lived on compare's own rows, so this is a behaviour change outside the PR's purpose.
F2	nit	PR body, "Left for the owner" N4	The rationale for not pinning DEL misreads the read-only invariant. "cmd/server is read-only" (AGENTS.md, Kpa-clawbot#1283) means the server opens SQLite mode=ro and does no DB writes. A _test.go file in cmd/server does not touch that. The ready-to-apply pin is a few lines (sanitizeSlowPathComponent("a\x7fb") == "ab"). Either add it here or correct the reason so the owner does not inherit a false constraint [A].
F3	nit	test-xss-escape-sinks.js (#393 N5)	The issue says "only 5 of ~30 newly escaped perf cards have a test". I count 36 perf-num card sites wrapped in escapeHtml in perf.js, and 8 now have a test (5 old + 3 new) [A]. The PR body narrows the point to "the Node-branch cards were not covered" and presents it as closed. The other ~28 are neither tested nor listed under "Left for the owner". Either list them honestly or cover them all at once, e.g. one table-driven test over every perf-num">${…} interpolation that asserts escapeHtml(…) or a numeric coercion.
F4	nit	test-xss-escape-sinks.js (#393 N6), axe gate	The N6 unit test only rejects the literal role="row". My mutant role="row" → role="cell" (equally invalid without a table ancestor) stays 77/0 green [T]. The CI axe gate does not back it up either: test-a11y-axe-1668.js runs a 13-rule whitelist without aria-required-parent/aria-required-children. It reports 0 violations on /analytics?tab=topology on master too (120/120 cells clean on both trees) [T]. The PR body's "test-a11y-axe-1668 zero violations" is therefore not evidence for N6. With axe's full ruleset scoped to .repeater-list I did reproduce the bug [T]: on master, 11 nodes fail aria-required-parent and 11 fail aria-required-children (critical); on the PR, 0. Suggest asserting an allowed role (link) instead of the absence of one value. Adding the two rules to the gate is a gate-policy decision for the owner.
F5	nit (low)	test-xss-escape-sinks.js (#393 N9a)	The no-inline-handler test only inspects the <tr …> opening line. An onclick on a child <td> of the same row survives (R9) [T]. Low value, mentioned for completeness.

F1, suggested fix (verified in scratch, not pushed): bind the click handler to document.getElementById('compareContent') instead of app. #compareContent is recreated by app.innerHTML = … on every init, so its listeners die with it. With that one-line change [T]:

• the leak probe stays on #/observers after visiting compare;
• compare rows still navigate on click and on Enter (#/packets/420a6702392db6b6);
• there are no page errors.

Please add an E2E regression test for that flow: observers → compare → observers at 800px, click a row, the hash stays #/observers and the slide-over is open. The N9 unit test cannot see this because it lifts only the handler body. The pre-existing #209 keydown handler in compare.js:114 has the same leak for Enter/Space. It is not introduced here, so moving it is the owner's call; it is the same one-line change if you want consistency.

Issue points, one by one

Point	Status	Red before → green after	Notes
#373 N6 a11y (role="row" on repeater <div>)	fixed (role="link", tabindex="0" kept)	red on master [T] → green	Full-ruleset axe 22 critical nodes → 0 [T]. Click and Enter still navigate in the browser [T]. Test strength → F4.
#373 N7 delegated [data-action="navigate"] pin	pinned	green on master (coverage pin of correct code); red under mutant R8 [T]	Fine.
#373 N9 inline onclick in compare.js	removed; delegated click added	2 tests red on master [T] → green	0 inline on* attributes on rendered rows; click and Enter navigate [T]. Regression F1.
#373 N10 loadLocalEsc second esc	fixed (countEscDefs)	green on master (test of the guard helper); red under R4 [T]	Fine.
#394 N1 doc bound 257 → 259	fixed	2 of 3 red on master [T] → green	259 = 2×128 + 2 "…" + space matches sanitizeSlowPathComponent/normalizeSlowPath and the existing Go test's 2*slowPathRawCap + 3 [A].
#394 N2 stale #394 PR body	Left for the owner	—	Reasonable: it is another PR's description, not a repo artifact.
#394 N3 gate header wording	softened, gate logic and allowlist untouched	— (prose)	Matches the #394 review's recommendation ("soften the wording, not to touch the gate") [K]. Diff is comment-only [A].
#394 N4 DEL unpinned	Left for the owner	—	Honest, but the stated reason is wrong → F2. Confirmed still unpinned: no 0x7f/DEL case in perf_slowpath_392_test.go [A].
#394 N5 perf cards	partial	green on master; red under R2 [T]	8 of 36 → F3.
#394 N6 err.message	test added	green on master; red under R3 [T]	Fine.

Scope: no product decision or new config slipped in. Changes are limited to the 7 files, and there is no .go change [T]. The only behaviour change outside the stated purpose is F1.

Tests and mutants

Trees: the merge-base is ce29f5dc (= master at review start), so the merged tree equals the head tree. Master moved to 1aaa41c9 during the review, so I also re-merged against it (clean merge, tree 3da3c5b4).

Unit and suite runs [T]:

• Merged tree vs ce29f5dc:
  • node test-xss-escape-sinks.js → 77/0.
  • node test-issue-393-slowpath-doc-bound.js → 3/0.
  • node test-frontend-helpers.js → 712/0.
  • sh test-all.sh → 237/0. A first run gave 236/1 (test-channels-client-state-152.js, timing). It ran concurrently with the Go suite and my mutant runs, and is 3/3 green in isolation on both trees.
• cmd/server go test ./...: 3 runs, 2 hit 503 {"error":"index loading"} in different tests each time (_1352, _1144, PrefixCollisionExclusion, AnalyticsSubpathDetail), and the 3rd was ok. -run 'TestHandleNodePaths_.*_1352' -count=3 also fails on master, so this is a pre-existing local timing flake. The PR has no Go change.
• Merged tree vs 1aaa41c9: sh test-all.sh → 238/0, test-frontend-helpers.js → 712/0, cmd/server go test ./... → ok.
• cmd/ingestor: not run (no ingestor change, not touched by this PR).
• scripts/check-xss-sinks.sh --diff origin/master → exit 0, no findings.

E2E [T] against a local Go server with e2e-fixture.db prepared as in CI (freshen, seed SQL, corescope-migrate, seeds 2073, 199 and 245; servers stopped by port):

Test	Result
test-xss-333-pages-e2e.js	11/0
test-xss-333-e2e.js	8/0
test-xss-392-perf-e2e.js	1/0
test-issue-1640-compare-discovery-e2e.js	9/0
test-issue-236-analytics-sorting-e2e.js	PASS
test-issue-205-analytics-subtab-deeplinks-e2e.js	15/0
test-slideover-1056-e2e.js	27/0 (does not visit compare first, so it misses F1)
test-a11y-axe-1668.js	0 violations / 120 cells, on master too (see F4)

Extra browser probes [T]:

• compare row click and Enter → #/packets/<hash>, no page errors;
• analytics repeater row click and Enter → #/nodes/<pubkey>;
• the F1 leak probe (master vs PR, with the fix applied in scratch);
• full-ruleset axe on .repeater-list.

Red before on master [T]: I ran the PR's test files against master's code. Expected red: N6 (1), N9 (2), N1 (2). Green on master by design: N5, perf-N6, N7, N10. These are coverage pins for code that was already correct, so their "red" is shown by mutants.

Own mutants [T] (9, applied to the merged tree and restored, byte-identical to head afterwards):

#	Mutant	Result
R1	compare delegated click: drop location.hash = …	red (N9b)
R2	perf.js: drop escapeHtml(el.currentLagMs)	red (N5)
R3	perf.js: escapeHtml(err.message) → String(err.message)	red (perf-N6)
R4	countEscDefs: (?:const|let|var) → (?:const|var)	red (N10, let case)
R5	api-spec.md: ≤259 runes → ≤258 runes	red (N1)
R6	analytics repeater: role="link" → role="cell"	survives → F4
R7	analytics repeater: drop tabindex="0"	red (N6)
R8	analytics handler selector → tr[data-action="navigate"]	red (N7)
R9	compare.js: add onclick on a child <td>	survives → F5

Checklist

Item	Result
Every point has a red→green test or an honest deferral	Mostly yes [T]. N5 is partial (F3); the N4 reason is wrong (F2).
≥2 own mutants	9 [T]
No behaviour change beyond the purpose	No → F1 [T]
cmd/server read-only, no new map[string]interface{}	no .go change; 0 occurrences added [T]
No hardcoded colours	none added [T]
check-xss-sinks.sh --diff origin/master	exit 0 [T]
Fork guards	9 in deploy.yml, 1 in release-fast-path.yml [T]
Closing keywords	none in body or commit; closingIssuesReferences empty [T]
Commit author / committer	dborup <kontakt@meshview.dk> / same [T]
CI per job (run 38356292602)	Go Build & Test ✅, Playwright E2E ✅, Docker ✅; Release / Deploy Staging / Badges skipped (fork guard). #256/#267 not involved [K].
Head stable	ls-remote = d3782853 before and after the review [T]

Not verified

• The author's own 8 mutants were not re-run one by one. I ran my own 9, which overlap in intent with M4–M6.
• The N3 claim "exactly one extra finding" with path/q.path/k allowlisted. I relied on the fix(ui): escape perf dashboard values (#392) #394 review's measurement [K]. The change is prose-only.
• Real touch devices and Safari: F1 was reproduced only with Playwright Chromium at 800×800.
• How the pre-existing compare.js keydown leak behaves on pages other than observers.
• cmd/ingestor tests (not touched).
• Release, Deploy Staging and Badges jobs (skipped on the fork).

[dborup-agent]

Rapport — CS-pve-agent3 PR#402 runde-2 — head 536fab7

Review feedback addressed (commit 536fab77). Commits on top of d3782853:

• f7c1c75f: merge of origin/master 23f50b4f (no conflicts).
• 20d3c74e: F1 tests, red.
• 2a56fe65: F1 fix.
• 536fab77: F2–F5 test tightening.

1. F1 (blocking): fixed. In public/compare.js the delegated row click handler is now bound to #compareContent instead of the shared #app. app.innerHTML recreates #compareContent on every init, and the next page's render discards it, so no listener survives navigation and none stacks per visit [A][T]. A grep of the PR diff for addEventListener on app shows this was the only one the PR added [T]. The pre-existing fix(store): charge the path_json fallback and pin the hash-migration accounting (#202) #209 keydown handler (compare.js:114) still sits on #app. It has the same leak for Enter/Space, but this PR did not add it, so I left it alone and listed it under "Left for the owner" in the PR body. New tests:
   • Unit test "Follow-ups to #373: analytics row a11y, delegated navigate test, compare.js onclick, loadLocalEsc #393 F1 compare.js init: no click listener on the shared #app, none stacked per visit". It runs the real init() twice in a vm, with a fake #app whose innerHTML recreates its children. N9b now drives the click handler that this real init() installs, instead of a regex-lifted body.
   • E2E: two steps added to test-issue-1640-compare-discovery-e2e.js (already in CI with CHROMIUM_REQUIRE=1). At 800×800 the flow is home → compare. A compare row click still reaches #/packets/<hash>. Then → observers, where a row click must open the slide-over, leave the hash at #/observers and fire 0 hashchange.
   • Both are red on 20d3c74e [T]. The E2E failed with #/observers/D5EF2ED7…, which reproduces the review probe.
2. F2: fixed. The PR body's N4 rationale is corrected: the read-only invariant (bug(db): vacuumOnStartup fails with SQLITE_BUSY when ingestor + server share DB (auto_vacuum migration #919 broken in single-container topology) Kpa-clawbot/CoreScope#1283) is about DB writes and does not cover _test.go files. DEL is now pinned in cmd/server/perf_slowpath_392_test.go with the subtest "DEL is stripped from both halves, '~' is kept". It checks sanitizeSlowPathComponent("a\x7fb~") == "ab~" and that normalizeSlowPath strips DEL from the key half and the raw half. No server code changed [T].
3. F3: fixed. The test now covers every escapeHtml-wrapped perf-num card. My count is 42: the review's 36 plus the six cards with a style= attribute (Heap Used, Event Loop p95, Server/Client Hit Rate, WAL Size, Freelist) [T]. It has two parts:
   • A per-group label list (overview, Go runtime, Node runtime, SQLite stats, cache, packet store, SQLite) that fails by name when any card loses escapeHtml.
   • One test per card that evaluates that card's real template, with a hostile Proxy behind every field it reads [T]. The 20 cards that are not escaped (numeric coercions, fmtRate, composed locals) stay outside this test's scope, as before.
4. F4: fixed. The N6 test now requires the clickable repeater row to carry role="link" or role="button". It also rejects every WAI-ARIA 1.2 role that needs a context role (row, cell, gridcell, listitem, option, tab, …) anywhere in the row [T]. The PR body no longer cites the axe gate as evidence for N6. Adding aria-required-parent/-children to the gate is listed as an owner decision.
5. F5: fixed. The no-inline-handler test now scans the whole row expression, from <tr through '</tr>', so an on* attribute on a child <td> is caught [T].

No findings disputed.

Mutants (one or more per finding) [T]

Each mutant was applied, run and restored byte-identically. The "round-1 tests" column is the d3782853 version of test-xss-escape-sinks.js. Its only failure under the fixed code is its N9 regex lift, which no longer matches. Apart from that, it missed all of these mutants.

#	Mutant	New tests	Round-1 tests
F1	click handler bound back to app	red (unit F1 + N9b; E2E step 2 red with #/observers/D5EF…)	green
F2	r < 0x20 || r == 0x7f → r < 0x20 in routes.go	red (go test -run TestNormalizeSlowPath)	— (no prior pin)
F3a	drop escapeHtml on sq.rows.nodes	red (label set)	missed
F3b	drop escapeHtml on the styled card c.hitRate	red (label set)	missed
F3c	${escapeHtml(ps.trackedMB)}${ps.trackedMB} (partial wrap)	red (per-card "Tracked Memory")	missed
F4a	role="link" → role="cell"	red	missed
F4b	drop the role entirely	red	missed
F5	onclick on a child <td>	red	missed

Local tests [T]

Run on the merged tree, head 536fab77:

• node test-xss-escape-sinks.js → 121/0.
• node test-frontend-helpers.js → 712/0.
• node test-issue-393-slowpath-doc-bound.js → 3/0.
• cmd/server go test -count=1 ./... → ok. cmd/ingestor was not run, because it is not touched.
• sh test-all.sh → 238/238. The first run gave 237/1: test-channels-client-state-152.js F1 "Decrypting…" failed while the Go suite ran in parallel. That is the same timing failure the review saw. It passed 75/0 three times in isolation, and the second full run was clean.
• scripts/check-xss-sinks.sh --diff origin/master → exit 0.

E2E ran against a local Go server on a free high port, with e2e-fixture.db prepared as in deploy.yml: freshen, inline seed SQL, corescope-migrate, seeds 2073/199/245.

Test	Result
test-issue-1640-compare-discovery-e2e	11/0
test-slideover-1056-e2e	27/0
test-xss-333-pages-e2e	11/0
test-xss-333-e2e	8/0
test-xss-392-perf-e2e	1/0
test-issue-236-analytics-sorting-e2e	PASS
test-issue-205-analytics-subtab-deeplinks-e2e	15/0
test-a11y-axe-1668	0 violations / 120 cells

CI per job (run 38365456692, head 536fab7) [K]

Job	Result
Go Build & Test	✅ success (24m10s); test-all.sh 238/0, XSS escape sinks 121/0
Playwright E2E Tests	✅ success (28m47s); both #393 F1 steps ✅ in the log
Build & Publish Docker Image	✅ success
Release Artifacts / Deploy Staging / Publish Badges	skipped (fork guard)

#271/#301 did not appear, so no rerun was needed.

Scope / checklist

• Diff vs f7c1c75f: 4 files [T]:
  • public/compare.js: 1 line + comment.
  • test-xss-escape-sinks.js.
  • test-issue-1640-compare-discovery-e2e.js.
  • cmd/server/perf_slowpath_392_test.go.
• No workflow change, no new map[string]interface{} and no hardcoded colours [T].
• PR body updated through --body-file. The PR is still a draft, and closingIssuesReferences is empty [T].
• Commit author/committer is dborup <kontakt@meshview.dk>. Only plain pushes were made: no rebase, no amend and no force-push [T].
• Not verified: real touch devices and Safari; the compare keydown leak on other pages.

[dborup-agent]

Review — CS-pve-agent2 PR#402 — head 536fab7

Dom: APPROVE with nits. F1 is fixed: the compare click handler no longer survives the page, and the new unit and E2E tests go red when the #app binding is restored. F2–F5 are addressed, and the stronger tests caught 9 of my 11 mutants; the other two (N1, N3) are below. None of the remaining points blocks this PR.

Evidence tags: [T] = I ran it, [A] = from reading the code, [K] = from CI or earlier review threads.

Findings (round 3)

#	Severity	Where	Finding
N1	nit	test-xss-escape-sinks.js, ARIA_ROLES_NEEDING_CONTEXT	The list is described as "every WAI-ARIA 1.2 role with a required context role", but it leaves out caption (required context: figure/grid/table/treegrid). My mutant puts role="caption" on the child .repeater-name, and it stays 121/0 green [T]. axe-core 4.12.1 reports exactly that markup as a critical aria-required-parent violation [T]. Fix: add 'caption' to the list. The row-level allow-list (link/button) still holds, so this affects only the child-role half of the check.
N2	info	test-xss-escape-sinks.js, F1/N9b harness	When the handler is bound to document instead of #app, the unit tests go red only because the fake document has no addEventListener, so init() throws. No assertion catches it [T]. The E2E step still fails it with a real assertion (#/observers/D5EF…) [T], so the leak class is covered. A no-op addEventListener on the fake document, plus an assertion that no click listener lands on it, would make the unit test say why it failed. Optional.
N3	info	test-xss-escape-sinks.js N9b	The unit click target is the <tr> itself, so a handler that drops closest() (e.target.tagName === 'TR') passes the unit tests [T]. Real clicks land on a <td>. The E2E step "#393 F1 compare row click still navigates" catches this mutant (timeout) [T], and that E2E runs in CI. No action needed.
N4	pre-existing, out of scope	public/observers.js:182	observers.js also binds its delegated click (and keydown) handlers to the shared #app and never removes them. After an observers visit, that handler is still attached on compare: getEventListeners(#app).click = 1 on both master and PR [T]. In every flow I probed, the user-visible result was the same on master and the PR (see below). This PR did not add it. It belongs with the compare keydown leak already listed under "Left for the owner".

Round-2 findings, one by one

Finding	Status	Evidence
F1 (blocking): click handler on the shared #app	fixed	compare.js:127 binds to #compareContent, which app.innerHTML recreates on every init and the next page discards [A]. In the browser at 800×800 [T], PR and master give identical results in every flow: (a) fresh #/compare → row click: one hashchange to #/packets/420a6702392db6b6, no extra navigation. (b) compare → observers → row click: hash stays #/observers, 0 hashchange, slide-over open. (c) Enter on a compare row navigates once. (d) observers → compare → row click: one hashchange. With the PR, #app carries 0 click listeners on a fresh compare page. After 4 compare visits the count does not grow; the remaining listeners belong to observers, see N4. There were no page errors.
F1 mutant: restore the app binding	red	Unit: 119/2 (N9b + "#393 F1 … no click listener on the shared #app") [T]. E2E test-issue-1640-compare-discovery-e2e.js: 10/1, failing on #/observers/D5EF2ED7…, which reproduces my round-2 probe [T]. The round-1 compare.js (d3782853) placed into the merged tree is red the same way in both unit and E2E [T]. On master's own code the E2E is 11/0, as expected: master had no leak, because it used an inline onclick [T].
F2: N4 rationale / DEL pin	fixed	The PR body now states the read-only invariant correctly [A]. The pin is the subtest DEL is stripped from both halves, '~' is kept in perf_slowpath_392_test.go, a test file only, with no cmd/server code change [A]. Mutant r < 0x20 || r == 0x7f → r < 0x20 in routes.go:1430 makes it red (got "a\x7fb~") [T].
F3: perf cards 8 → all	fixed	The test lists 42 cards (the set I counted in round 2 was 36; I missed the six style=-attributed cards). The label-set check plus one hostile-Proxy test per card read sensibly [A]. Mutants: drop escapeHtml on the styled card sq.walSizeMB (WAL Size) → red. Drop escapeHtml(m.heapTotal) in the Heap Used label → red, caught by the label set [T]. Newly added unescaped cards stay out of scope, as the report says.
F4: ARIA role allow-list	fixed, with nit N1	role="presentation" → red. A child role="cell" → red. A child role="caption" survives → N1 [T].
F5: inline handlers on child elements	fixed	An onmouseover on the <span> inside a <td> → red [T].

Scope: since f7c1c75f the PR changes 4 files: compare.js (one line plus a comment), two test files and one Go test file (git diff --stat f7c1c75f 536fab77) [T]. No product or behaviour change beyond the purpose: the compare row click behaves exactly as master's inline onclick did in every probed flow [T]. There is no scope creep.

Tests and mutants

Trees: the merged tree is git merge-tree --write-tree origin/master 536fab77… = a7fe4290, with origin/master at 5cceea08; the merge is clean. Everything ran from git archive exports in scratch.

Suites on the merged tree [T]:

• node test-xss-escape-sinks.js → 121/0.
• node test-issue-393-slowpath-doc-bound.js → 3/0.
• node test-frontend-helpers.js → 713/0.
• sh test-all.sh → 238/0.
• cmd/server go test -count=1 ./... → ok (first run, no flakes this time).
• scripts/check-xss-sinks.sh --diff origin/master → exit 0, on both the merged commit and the head.

Red before on master [T]: the PR's test-xss-escape-sinks.js run against master's code gives 117/4. The failures are N6, N9a, N9b and F1. Those are the ones expected to fail.

E2E [T]: run against a local Go server with e2e-fixture.db prepared as in deploy.yml (freshen, inline seed SQL, corescope-migrate, seeds 2073, 199 and 245). Servers were stopped by port.

Test	Result
test-issue-1640-compare-discovery-e2e.js	11/0 (incl. both #393 F1 steps)
test-slideover-1056-e2e.js	27/0
test-xss-333-pages-e2e.js	11/0
test-xss-333-e2e.js	8/0
test-xss-392-perf-e2e.js	1/0
test-issue-236-analytics-sorting-e2e.js	PASS
test-issue-205-analytics-subtab-deeplinks-e2e.js	PASS

Own mutants [T] (12, applied to a scratch copy of the merged tree; the scratch copy was byte-identical to the merged tree again afterwards):

#	Mutant	Result
M1	compareContent click → app.addEventListener('click'	red: unit 2 tests; E2E F1 step 2
M2	compareContent click → document.addEventListener('click'	red: unit (harness throw, see N2); E2E F1 step 2 (assertion)
M3	perf: drop escapeHtml(sq.walSizeMB) (styled card)	red (F3 label set)
M4	perf: drop escapeHtml(m.heapTotal) in the Heap Used label	red (F3 label set)
M5	analytics: child .repeater-name gets role="cell"	red (N6)
M6	analytics: role="link" → role="presentation"	red (N6)
M7	compare: onmouseover on a child <span>	red (N9a)
M8	compare handler: closest() → e.target.tagName === 'TR'	unit green (N3); red in E2E F1 step 1
M9	analytics: child .repeater-name gets role="caption"	survives, see N1 (axe: critical aria-required-parent)
M10	Go: r < 0x20 || r == 0x7f → r < 0x20	red (TestNormalizeSlowPath/DEL…)
M11	round-1 compare.js (d3782853) in the merged tree	red: unit 2 tests; E2E F1 step 2

Checklist

Item	Result
Every issue point has a red→green test or an honest deferral	yes [T]. The F1/F3/F4/F5 pins are green on the head and red under mutants.
≥2 own mutants	12 [T]
No behaviour change beyond the purpose	yes. Compare click flows are identical to master [T].
cmd/server read-only, no new map[string]interface{}	only a _test.go change; 0 map[string]interface{} added [T]
No hardcoded colours	none added [T]
check-xss-sinks.sh --diff origin/master	exit 0 [T]
Fork guards	github.repository == appears 9 times in deploy.yml and once in release-fast-path.yml; no workflow change [T]
Closing keywords	none in the body or the commits; closingIssuesReferences is empty [T]
Commit author / committer	dborup <kontakt@meshview.dk> on all 5 commits [T]
CI per job (run 38365456692, head 536fab77)	Go Build & Test ✅, Playwright E2E ✅, Docker ✅. Release Artifacts, Deploy Staging and Publish Badges were skipped by the fork guard. Neither #256 nor #267 is involved [K].
Head stable	ls-remote = 536fab77 before and after the review; the PR is still a draft [T]

Not verified

• cmd/ingestor go test ./...: not run, because the PR does not touch the ingestor.
• test-a11y-axe-1668.js was not re-run this round. It does not cover the N6 rules (round 2, F4), and the analytics markup did not change since round 2.
• The author's own F1–F5 mutants were not re-run one by one. My M1/M3/M5–M7/M10 overlap with them in intent.
• Real touch devices and Safari. Everything ran in Playwright Chromium at 800×800.
• How the pre-existing #app leaks (compare.js keydown, observers.js click/keydown, N4) behave on pages other than compare and observers.
• Release, Deploy Staging and Badges jobs (skipped on the fork).