Repository navigation
Show sender-selected path-hash width on channel messages - #212
Conversation
dborup
left a comment
dborup
left a comment
There was a problem hiding this comment.
Reviewed commit fd7e55df. One P2 finding should be addressed before merge: the packet-detail hash-size summary can describe the original transmission while the raw-byte breakdown describes a different selected observation. See the inline reproduction and suggested correction.
Validation performed independently: relevant server/channel tests and the packetpath suite passed with the race detector; 709 frontend-helper checks, 13 channel evidence/cache checks, and all 8 channel browser checks passed. A separate browser reproduction confirmed the mismatch below and compared it with the base packet-detail implementation. The full server suite and staging were not rerun as part of this review.
| const plOff = getPathLenOffset(pkt.route_type); | ||
| const rawPathByte = pkt.raw_hex ? parseInt(pkt.raw_hex.slice(plOff * 2, plOff * 2 + 2), 16) : NaN; | ||
| const hashSize = (isNaN(rawPathByte) || (rawPathByte & 0x3F) === 0) ? null : ((rawPathByte >> 6) + 1); | ||
| const hashSize = senderPathHashSize(pkt.raw_hex); |
There was a problem hiding this comment.
[P2] Read the hash width from the selected observation
renderDetail uses effectivePkt for the selected observation's raw bytes and field table, but this line reads the original transmission's pkt.raw_hex. Reproduced in Chromium with an original zero-hop flood beginning 1540 and a selected direct-zero-hop observation beginning 1600: the summary displays Hash Size: 2 bytes, while the byte table correctly displays hash_count=0 (no encoded hash size). With the base packet-detail code, the summary did not emit a width for this zero-hop case. This breaks the PR's per-frame/zero-hop display contract when a transmission has multiple observation variants. Use the same raw-byte source as the displayed observation (effectivePkt.raw_hex || pkt.raw_hex) and add a browser regression that selects between the two observations, asserting that the summary and byte table agree.
dborup
left a comment
dborup
left a comment
There was a problem hiding this comment.
Follow-up review — changes still recommended before merge
Rechecked head fd7e55df8c71a79a3a73e8a4aea8ebfc10820412 against master 8bafcdf2a6bb0014127bce4822c8ad0399bd2f12. The PR head is unchanged since the earlier review. All 12 changed files in the local review export were verified byte-for-byte against this commit.
1. Existing P2: selected-observation hash-size mismatch remains
The original inline finding is still applicable; I am linking it rather than duplicating the thread. The Chromium reproduction still shows Hash Size: 2 bytes for a selected direct-zero-hop observation while its byte table says no encoded hash size. The original transmission begins 1540, the selected observation begins 1600; the summary still reads pkt.raw_hex rather than the selected observation's effective bytes. The base implementation emits no summary width for this zero-hop case. Use the same effective raw-byte source for the summary and byte table and add an observation-switching regression.
2. New P2: shared helper is missing from ESLint globals
The failed CI job stops in frontend lint, with three senderPathHashSize is not defined errors. These reproduce locally in public/channels.js:74 and public/packets.js:3415,3770. The corresponding base files pass the same check. The helper exists in app.js at runtime; this is a missing cross-file lint declaration, not a claim that the browser cannot load the helper. Details and the suggested correction are attached inline.
Independently rerun in this pass
- Targeted server channel/hash-size tests with
-race -count=1: passed. - Full
internal/packetpathsuite with-race -count=1: passed. - Frontend helper checks: 709 passed, 0 failed.
- Channel evidence/cache checks: 13 passed, 0 failed.
- Channel browser regression on a disposable local server: 8 passed, 0 failed, including the 375 px layout and WS/REST cases.
- Separate selected-observation browser reproduction: the existing mismatch is still present.
- Frontend lint: 3 errors, matching CI. The downstream CI Playwright and Docker jobs were skipped, not passed.
The full server/ingestor suites and staging/production were not rerun in this pass. No source changes, pushes, merges or deployments were made. Please address both P2 items and rerun the full CI pipeline before merging.
Posted as a comment review because the signed-in account is also the PR author.
| } | ||
|
|
||
| function senderSizeFromRawHex(rawHex) { | ||
| return typeof senderPathHashSize === 'function' ? senderPathHashSize(rawHex) : null; |
There was a problem hiding this comment.
[P2] Register the shared senderPathHashSize helper with ESLint
This new cross-file call, plus the two new calls in packets.js at lines 3415 and 3770, fails the mandatory no-undef gate because senderPathHashSize is absent from .eslintrc.json's shared globals. CI run 37190814810 fails with exactly these three errors, and running ESLint locally on the changed frontend files reproduces them; the corresponding base files pass. The runtime function is present in app.js, so declare it as a read-only shared global following the existing helper conventions (or use an explicitly exported namespace consistently), without disabling no-undef. Then rerun frontend lint and the complete CI pipeline: the current failure prevents the downstream browser and Docker checks from running.
…vation Addresses both P2 findings on PR #212, plus a red test gate neither review reported. 1. Frontend lint (no-undef). senderPathHashSize lives in app.js but was not declared in .eslintrc.json, so the three cross-file calls in channels.js and packets.js failed the mandatory no-undef gate. Declared as a read-only shared global alongside the other app.js helpers. 2. Selected-observation mismatch. renderDetail built the byte table from the selected observation (effectivePkt) but read the "Hash Size" summary from the original transmission's raw_hex. Observations of one transmission carry their own frames (routes.go fills observations[].raw_hex), so a selected direct zero-hop observation showed "Hash Size: 2 bytes" above a byte table that said it encodes no width. The summary now reads the same frame. 3. test-packets.js was left stale by the original change: the two buildFieldTable hash_size tests still expected the pre-PR "direct advert" wording and a 4-byte hash size, so `sh test-all.sh` -- a CI gate that runs before lint -- was red. Replaced with tests for the new contract, and restored the information the new wording had dropped: the byte breakdown now distinguishes TRACE SNR path bytes, an invalid 0b11 width field, and sendZeroHop's 0x00 direct marker instead of calling all three "no encoded hash size". Also replaced the hand-rolled senderPathHashSize stub in test-channels-observed-path-hash-size.js with the real app.js helper; the stub claimed a 3-byte width for '59C0', which the shipped rule reports as unknown. New browser regression test-packet-detail-sender-hash-size-obs-e2e.js switches between a zero-hop flood observation and a direct zero-hop one and asserts the summary and byte table agree; it fails on the pre-fix code.
Rapport — CS-Macmini PR#212 runde 2 — head 0cdfc66Review feedback addressed (commit
Tests [T]All on the merged tree at
Browser suites ran against a disposable local Go server on a high loopback port with a scratch copy of
Mutants [K]One per finding; each was reverted immediately after.
CI per jobNo CI signal was produced — not because of a test, but because no hosted runner was ever assigned. The pipeline run for
Every attempt of the
So no step ran — not checkout, not the Go suite, not The failure is not specific to this branch: in the same window CI therefore still needs to be rerun once runners are available. Everything in the Tests section above was run locally on the merged tree and is the only verification currently backing this head. Analysis notes [A]
Remaining / not done
|
Review — CS-Minimax PR#212 — head 0cdfc66Dom: APPROVE with nits Independent read-only review. Head verified as Both P2 items from the two earlier formal reviews are fixed, and each is backed by a test that is red on the pre-fix code and green on this head. The three undisclosed items the round-2 report added (stale Findings
N2, demonstrated through the suite's own Reachability is narrow: it needs an observation frame whose route differs from the transmission's in transport-ness (0/3 vs 1/2), not merely in route, so the flood-vs-direct case the original P2 used does not trigger it. Pre-fix, The points verified1. ESLint — confirmed. 2. The P2 — fixed, with a red-before test. Mutant M2, restoring 3. The other round-2 items — all three correct.
4. The merge commit is clean. 5. Channel and packets E2Es — green. All against a local Go server on a high loopback port, with a scratch copy of
Extra verificationGo ↔ JS parity. The REST row gets TestsOn the merged tree against
Neither known flaky test appeared: no Hash Stats sort failure and no backfill write-hold failure in any run. MutantsSix, all mine, each reverted from a scratch snapshot immediately after (never with
Standing checks
CI per jobAttempt 4 of the pipeline run for this head got a runner for the first time, so there is now a real signal for the gating job — better than the three all-cancelled attempts the round-2 report described.
The Go job's passing steps include the ones that matter here: The Playwright job again never acquired a hosted runner — zero steps executed, so this is not a test failure and not either known flaky test. The E2E coverage in this review is therefore local only, and the Playwright job still needs a CI run once runners are available. Not verified
|
2 participants
Summary
Adapt the sender-selected path-hash display from Kpa-clawbot/CoreScope PR Kpa-clawbot#2089 for this fork. Channel messages now show
Sent with: N-bytefrom the transmission's raw frame header instead of presenting relayed observation-path widths as the sender's choice. The existing observation evidence is retained internally.The fork also uses the same header rule in View packet. A zero-hop flood still encodes the sender's width and displays it; a direct zero-hop marker does not encode a width and remains unlabeled. This avoids the cross-view inconsistency in a direct port of the upstream change.
Verification
cmd/serverGo suite andgo vet ./...passed locally.internal/packetpathtests passed, including 1/2/3-byte, transport, zero-hop flood, direct-zero, TRACE and malformed headers.Scope and limitations
senderPathHashSizeto channel-message API rows (0 means unknown); documents its per-frame meaning.