Repository navigation
port(upstream#1957): carry observer_id into replayed packets - #41
Merged
Merged
Conversation
…yed packets (Kpa-clawbot#1957) Closes Kpa-clawbot#1898. Closes Kpa-clawbot#1900. Two issues, one omission. With a region filter active, VCR replay on the Live map rendered **nothing at all** (Kpa-clawbot#1898), and the Replay button on packet detail **silently did nothing** (Kpa-clawbot#1900). ## Cause `packetMatchesRegion` (`public/live.js:80-92`) matches a packet group by looking up `packets[i].observer_id` in the observer roster map. A packet whose `observer_id` is null is skipped, and when none match it returns `false` and the caller drops the whole group (`live.js:3374`). `dbPacketToLive()` returned `observer` (the resolved name) but never `observer_id`. So every replayed packet was skipped, and every group was dropped. The Replay button had the same gap: both branches passed `obsName(o.observer_id)` and threw the id itself away. **The value was there the whole time.** The VCR builds its entries with `Object.assign({}, p, obs, ...)`, so the observation's `observer_id` is on the input, and the server has returned `observer_id`, `observer_name` and `observer_iata` per packet since `cmd/server/db.go:345-347`. Only the object literal dropped it. ## Fix Carry `observer_id`, and `observer_iata` alongside it so `obsIataBadgeHtml` (`live.js:102-108`) can use the direct field for replayed packets instead of falling back to the roster map. Three lines of behaviour, in two files. ## Verification Four regression tests in `test-live-region-filter.js`. **Three fail without the fix**, checked by reverting `live.js` and re-running: ``` ❌ Kpa-clawbot#1898: dbPacketToLive carries observer_id through ❌ Kpa-clawbot#1898: a replayed packet survives an active region filter ✅ Kpa-clawbot#1898: dropping observer_id is what broke it (guards the regression) ❌ Kpa-clawbot#1898: observer_iata is carried so the badge needs no roster lookup ``` The one that passes either way does so on purpose: it asserts a packet carrying **no** `observer_id` is still dropped, pinning the mechanism so a future change cannot make the filter match everything. That test's sandbox needed `getParsedDecoded` and `getParsedPath`. `live.js:14` captures those from `packet-helpers.js` at load time and the sandbox does not load it, so they are stubbed in the sandbox definition rather than assigned afterwards. Assigning later is too late for that capture, which cost me two attempts. Other suites unaffected: `test-live.js` 95 passed, `test-packet-filter.js` 99, `test-frontend-helpers.js` 656. `test-1110-live-filter.js` fails identically on unmodified master with `ERR_CONNECTION_REFUSED`; it is an E2E test needing a server on port 13581. ## Note Both issues were filed separately and neither names the other. They are the same root cause in sibling code paths, which is why they are fixed together rather than in two PRs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 9488e9c) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mutation testing showed half this PR was unverified: removing observer_id from live.js's dbPacketToLive fails 2 of the 13 region-filter tests, but removing it from packets.js's replay mapping passed the entire suite. The only coverage lived in test-live-region-filter.js, which reaches live.js alone — so the Kpa-clawbot#1900 half could have regressed silently. The mapping sat inline in the replay button's anonymous click handler, so nothing could call it. Extracted it as buildReplayPackets(pkt, data, ctx) and exported it through _packetsTestAPI, following the precedent applyObserverFilter set in Kpa-clawbot#1748: test the real production mapping, not a re-implementation of it that cannot fail when the original does. The extraction is behaviour-neutral — all existing suites are unchanged. Two tests now pin both branches: a single-observation packet carries observer_id and observer_iata alongside the resolved name, and a multi-observer packet gives each observation its OWN observer_id rather than the transmission's representative. Removing observer_id from either branch now fails. One test detail worth naming: the assertions join the ids into a string instead of using deepStrictEqual. The array comes from the vm sandbox's realm, so its prototype differs from the test realm's and a strict deep compare fails on identical contents. Verified: live-region-filter 13/13, frontend-helpers 705 passed (up from 703; the 2 remaining failures are the pre-existing favStar ones that also fail on master), XSS gate 34/34, eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent review proved the buildReplayPackets extraction is behaviour-neutral — a differential harness ran master's verbatim inline block against the extracted function over 5000 randomized inputs (0-3 observations, nulls, garbage timestamps, malformed path_json/decoded_json) with 0 mismatches, comparing key order and the parse-cache side effects as well as values. Two findings fixed here. The port's own test named "Kpa-clawbot#1898: dropping observer_id is what broke it (guards the regression)" never called dbPacketToLive. It hand-built { observer: 'Obs One' } and asserted on packetMatchesRegion alone, so it duplicated the existing "does not match when packet has no observer_id" test and survived the very mutation its name claims to guard. It now drives the real mapping and is named for what it actually checks: a source row without an observer_id yields a packet that the filter skips. The genuine regression guard is the sibling test, which does kill that mutation. Switched my multi-observer assertions from joined strings to assert.deepEqual. The loose form compares correctly across the vm sandbox's realm — which is why deepStrictEqual fails there on identical contents — and asserts more than a join. Also dropped a stale "line ~85" reference from a comment. Filed as follow-up, not fixed here: the "Multibyte only" gate six lines above the region gate has the identical defect. groupIsMultibyte reads raw_hex and route_type, and neither replay mapping emits either (they emit `raw`, and no route_type at all), so with that option enabled Replay still renders nothing. Pre-existing, opt-in, and untested because the existing E2E hand-builds packets with a literal raw_hex and bypasses both mappings. Verified: region-filter 13/13 and still kills the live.js mutation (11/2); frontend-helpers 705 passed and still kills both packets.js branches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #25 (commit
5c93ebfbthere). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
9488e9c31cc9b5d6c4e07ca6fdfb8266f9a407b8git cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.public/live.js,public/packets.js,test-live-region-filter.js.Problem
With a region filter active, VCR replay on the Live map rendered nothing, and the Replay button on packet detail silently did nothing. The region filter (
packetMatchesRegion) matches onobserver_id, which replayed packet objects never carried.Change
Replay packet objects built in
live.jsand in the packet-detail Replay handler now carryobserver_idandobserver_iata.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5xss-gate-diffchannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-linttest-issue-1375-scope-stats-fetch.jsExactly oneapi('/scope-stats'call exists (the fixed loader) — found 2test-issue-1648-m4-emoji-scan.jsmap.js has 1 emoji/misc-icon hit(s):test-a11y-axe-routes-coverage.jsaxe ROUTES missing analytics tabs (issue #1706): areas, foreign-traffic, wardrivingtest-frontend-helpers.jsfavStar returns empty star for non-favorite: The expression evaluated to a falsy value:,favStar returns filled star for favorite: The expression evaluated to a falsy value:Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Local Go server (built from master) on the committed E2E fixture DB (freshened, migrated and seeded exactly like CI), serving this branch's
public/, compared side by side with the same server serving master'spublic/. Real UI flow with the Live region filter set to SFO (meshcore-region-filter): opened packet 76c71994d1680a57 (31 hops, observer in SFO) on the Packets page and clicked Replay; the object written tosessionStorage['replay-packet']was captured. master:observer_idisundefined. this branch:observer_id= the SFO observer (C0FFEEC7…) andobserver_iata=SFO— the fieldpacketMatchesRegionneeds. Same result with advert 4953f0d6c2c602d2 (B8714384…, SFO). Partial: the visible replay itself could not be observed in this environment — a master control run without any region filter also showed "0 pkts" and no route animation after Replay (animations draw on canvas; screenshots 2 s after the click show nothing in all three runs). So the browser check proves the data fix on the real UI path, not the rendered outcome; the region-matching path for replayed packets is covered by the extendedtest-live-region-filter.js(in CI's JS unit list): 9 cases on master, 13 on this branch, all passing locally.Not run:
eslint(not installed locally; CI installs it on the fly).-race/tests for modules this PR does not touch (unchanged code, identical to master).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34751057436 on
61dddb7e. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code