Skip to content

feat(#1751): show transported region scopes in repeater sidebar#1752

Open
ArcanConsulting wants to merge 2 commits into
Kpa-clawbot:masterfrom
ArcanConsulting:feat/1751-show-transported-region-scropes-in-repeater.patch
Open

feat(#1751): show transported region scopes in repeater sidebar#1752
ArcanConsulting wants to merge 2 commits into
Kpa-clawbot:masterfrom
ArcanConsulting:feat/1751-show-transported-region-scropes-in-repeater.patch

Conversation

@ArcanConsulting

Copy link
Copy Markdown

Closes #1751.

…debar

Surface which region scopes a repeater has transported. For repeater/room
nodes, accumulate the deduplicated, sorted set of transmissions.scope_name
across every non-advert packet where the node appears as a path hop
(byPathHop), expose it as `transported_scopes`, and render a "Transported
scopes" row in the node-detail sidebar.

- store: add StoreTx.ScopeName, populated (gated by db.hasScopeName) in all
  four loader paths — Load, loadChunk, IngestNewFromDB and the chunked
  scanAndMergeChunk. scope_name is appended as the last selected column so
  the conditional scan-arg order stays stable.
- enrich: add RepeaterRelayInfo.TransportedScopes; accumulate in both the
  bulk computeRepeaterRelayInfoMap and the per-node computeRelayInfoFromEntries
  (threaded via relayEntry). Scope accumulation is NOT time-windowed (unlike
  relayCount1h/24h) and is taken before the timestamp-parse gate.
- routes: emit transported_scopes on /api/nodes and node detail only when
  non-empty, so the field is absent for no-scope nodes and on schemas without
  the column (hasScopeName=false).
- web: render a gated "Transported scopes" badge row in nodes.js.
- test: transported_scopes_1751_test.go covers bulk dedup/sort/advert
  exclusion, per-node parity, and the empty-when-no-scope contract.
@ArcanConsulting ArcanConsulting force-pushed the feat/1751-show-transported-region-scropes-in-repeater.patch branch from 3c3c9c5 to 93b4a0b Compare June 19, 2026 13:06
@Kpa-clawbot

Copy link
Copy Markdown
Owner

🤖 Hourly PR-watch review (round 1)

Verdict: APPROVE-WITH-CONCERNS — CI gated on operator workflow approval (first-time-contributor action_required).

Surfaces transported_scopes (deduped, sorted, advert-excluded) on repeater/room nodes via RepeaterRelayInfo.TransportedScopes. Threaded through both the bulk path (computeRepeaterRelayInfoMap) and the per-node path (GetRepeaterRelayInfocomputeRelayInfoFromEntries via the new relayEntry.scope field) so /api/nodes and node-detail stay in parity. JSON field omitted via omitempty + len > 0 guards in routes — clean schema-fallback behavior for hosts without transmissions.scope_name.

Reviewed by: carmack (backend) + munger (semantics) + kent-beck (TDD gate) + tufte (sidebar render).

Local verification

  • go test -run TestTransportedScopes -count=1 ./cmd/serverPASS locally on the branch.
  • go build ./cmd/server / go vet clean.
  • Pre-existing flake TestDistanceConcurrentRequestsDuringBuildReturn202 fails on master too (only 9 did / only 5 did / only 1 did across -count=3 runs) — NOT introduced by this PR.

Findings

Carmack (backend)

  • scope_name appended as the last selected column in all four loader SQL builders (Load, loadChunk, IngestNewFromDB, scanAndMergeChunk) — keeps the conditional scanArgs ordering stable. Correct pattern.
  • scopeSet is per-key local (bulk path) and per-call local (per-node path) — no shared-state race.
  • MINOR: scopeSet := map[string]struct{}{} in computeRepeaterRelayInfoMap is allocated unconditionally inside the per-key loop even when the schema has no scope_name. On a host with hasScopeName=false and 10k repeater keys, that's 10k throwaway maps per recompute. Cheap, but the lazy-allocate pattern from computeRelayInfoFromEntries (only allocates on first scope hit) is the right one — port it to the bulk path for symmetry.

Munger (semantics / footguns)

  • ✅ Advert exclusion is correctly checked before scope accumulation in both paths. A self-advert carrying scope wouldn't prove transport — exclusion is the right call and the test pins it.
  • ✅ Scope accumulation deliberately NOT timestamp-gated (comments in both paths explain). Reasonable: an unparseable first_seen doesn't disprove the repeater carried that scope's traffic. Documented in both spots.
  • MINOR: TransportedScopes is unbounded across the in-memory window. A misbehaving sender flooding distinct scope names through one repeater would inflate the per-node JSON. Practically limited by how many region scopes you actually run — fine today, worth a soft cap (or ScopeName validation at ingest) before scope_name becomes operator-controllable.

Kent Beck (TDD gate)

  • NOTABLE: Single commit 93b4a0b3 contains both the feature and the test. Per the project's TDD section this is the failure mode (no visible red commit, no proof the test gates the change). For a community PR this is comment-only feedback, but the test itself is real-behavior — reflect.DeepEqual on a []string{"region-east","region-west"} slice (deduped, sorted, advert excluded) would fail if the feature were reverted. Verified by reading: TransportedScopes field is the only producer; removing the assignment empties the test. Real assertion, not a tautology.
  • ✅ Three behavior contracts pinned: bulk dedup+sort+advert-exclusion, bulk-vs-per-node parity, empty-when-no-scope. Covers the API-omission contract.
  • MINOR: Add a 4th case for the cross-bucket fold (the if len(key) >= 2 { ... snap[prefix] ...} branch in computeRepeaterRelayInfoMap): a tx in the 1-byte prefix bucket should contribute its scope to the full-pubkey key with dedup by tx.ID. Untested in this PR.
  • NIT: scopeTx's hash uses time.Duration(id).String() — produces "1ns", "2ns", "3ns" for ids 1/2/3. Works but reads strangely; strconv.Itoa(id) would be clearer.

Tufte (sidebar render)

  • ✅ Gated on n.role === 'repeater' || n.role === 'room' + Array.isArray + length — three layers of safety against malformed payloads.
  • escapeHtml(String(sc)) — XSS-safe even if scope name carries <>".
  • ✅ Tooltip explains the field. ⓘ icon convention matches existing rows.
  • MINOR: Inline style="margin:0 2px" on each <span class="badge"> repeats per scope. If badges are common in this sidebar, fold into .badge CSS (or a single wrapper with gap:). Cosmetic.
  • NIT: When a repeater carries many scopes (10+), they'll wrap awkwardly inside the table cell. Not a blocker but a <div class="badge-row"> wrapper with flex-wrap would render nicer at scale. Punt unless real data needs it.

TDD verdict

Same-commit test+impl is the only structural concern, and it's comment-only for a community PR per project policy. Subsequent ranking/behavior changes from this contributor would benefit from the red-then-green split.

3-axis merge gate

  • ✅ mergeable: MERGEABLE
  • ❌ mergeStateStatus: BLOCKED — first-time-contributor CI workflow requires operator approval (action_required on runs 27827192999 / 27827546731). Auto-merge gate cannot evaluate.
  • ⚠️ Polish: 0 BLOCKER / 0 MAJOR / 4 MINOR / 2 NIT

Operator action required

  1. Click Approve and run workflows on the PR page to unblock CI.
  2. Once CI is green and mergeStateStatus flips to CLEAN, the polish round is clean enough to squash. None of the MINORs are gating; address as follow-ups if desired.

— Not auto-merging (BLOCKED). Not auto-fix-spawning (community author, no prior signal). Comment-only per watcher policy.

…e cap, fold test, badge wrap)

Follow-up to the Kpa-clawbot#1751 PR-watch review (4 MINOR / 2 NIT, none gating):

- Carmack (MINOR): lazily allocate the bulk-path scopeSet on first scope hit
  (was unconditional per byPathHop key) — hosts without scope_name now pay
  nothing per key. Factored the set->sorted-slice logic into a shared
  sortedCappedScopes() helper so the bulk and per-node paths stay in exact
  parity.
- Munger (MINOR): soft-cap TransportedScopes at maxTransportedScopes (32),
  keeping the lexicographically-first names, so a flood of distinct scope
  names through one repeater can't inflate the node JSON unboundedly.
- Kent Beck (MINOR): add TestTransportedScopes_CrossBucketFold covering the
  1-byte prefix-bucket fold (a scope seen only in the prefix bucket surfaces
  on the full-pubkey key; a tx in both buckets is not double-counted). Add
  TestTransportedScopes_Capped for the new cap.
- Kent Beck (NIT): scopeTx hash now uses strconv.Itoa(id) instead of
  time.Duration(id).String() (which read as "1ns"/"2ns").
- Tufte (MINOR+NIT): render scope badges in a single inline-flex wrapper with
  gap + flex-wrap instead of a per-badge inline margin — DRYs the markup and
  wraps gracefully when a repeater carries many scopes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: show transported region scopes in repeater sidebar

2 participants