Skip to content

fix(dashboard): length-prefix layer-edge aggregation key to avoid '|'-separator collisions#457

Open
tirth8205 wants to merge 1 commit into
Egonex-AI:mainfrom
tirth8205:fix/edge-aggregation-layer-key-collision
Open

fix(dashboard): length-prefix layer-edge aggregation key to avoid '|'-separator collisions#457
tirth8205 wants to merge 1 commit into
Egonex-AI:mainfrom
tirth8205:fix/edge-aggregation-layer-key-collision

Conversation

@tirth8205

Copy link
Copy Markdown
Contributor

Problem

  • aggregateLayerEdges builds the pair-merge key as ${a}|${b} from the two sorted layer ids, with NO length prefix. If layer ids can contain the '|' separator, two genuinely distinct layer pairs collapse to one key. Example: pair ("x|y","z") sorts to ["x|y","z"] -> key "x|y|z", and pair ("x","y|z") sorts to ["x","y|z"] -> key "x|y|z" — identical. The two pairs merge into a single aggregation: one layer-edge is silently dropped and its count/edgeTypes are folded into the surviving entry under the wrong sourceLayerId/targetLayerId. Downstream computePortals() (same file) then reads agg.sourceLayerId/agg.targetLayerId from the merged entry, so a layer whose real pair was absorbed (e.g. active layer "x") returns an empty/incorrect portal list — the x->y|z connection vanishes entirely. Layer.id is typed as z.string() in core schema (schema.ts:398) with no constraint forbidding '|', and layer…

Fix

  • Length-prefix the canonical key the same way aggregateContainerEdges does, so the boundary between the two ids is unambiguous regardless of which separator characters appear in layer ids. Replace const key = ${a}|${b}; with const key = ${a.length}:${a}|${b}; (and keep the same value for collision avoidance). This is a 1-line change; no API/shape change since the key is internal to the Map.

Testing

Adds unit test(s) that fail before the change and pass after. The full dashboard test suite, eslint, and tsc --noEmit all pass locally on this branch.

Found via a static correctness audit of the dashboard layer-edge aggregation.

🤖 Generated with Claude Code

…-separator collisions

aggregateLayerEdges built its pair-merge key as `${a}|${b}` with no length
prefix, so distinct layer pairs whose ids contain the '|' separator (e.g.
("x|y","z") and ("x","y|z")) collapsed to one key. One layer-edge was
silently dropped and its count/edgeTypes folded into the wrong pair, making
computePortals() return an empty/incorrect portal list.

Length-prefix the first sorted id (`${a.length}:${a}|${b}`), matching the
guard already used by the sibling aggregateContainerEdges, so the boundary
between the two ids is unambiguous. Key stays internal to the Map.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@thejesh23 thejesh23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Defensive fix against an unreachable input.
The only producer of Layer.id is toLayerId in core/src/analyzer/layer-detector.ts:72, which emits layer:<lowercased-dashed-name> — '|' cannot appear. The schema-level z.string() is the only path that could let it through, and no current loader constructs layers from untrusted input. The fix is harmless, but framing it as a real correctness bug (PR body: "one layer-edge is silently dropped") overstates impact; this is defensive hardening, and the commit/PR title should say so.

2. Stale comment above the Map.
The // Key: "layerA|layerB" (sorted) → aggregation comment one line above the new key construction still describes the old shape and no longer matches ${a.length}:${a}|${b}. Update it (or drop it) so the next reader doesn't grep for the old key format.

3. Structural key would remove the class of bug entirely.
Length-prefixing copies the pattern from aggregateContainerEdges, which is consistent, but a tuple-key Map keyed on [a,b] via a nested Map<string,Map<string,…>> (or JSON.stringify([a,b])) eliminates separator reasoning altogether and is self-evidently collision-free. Worth at least a one-line justification in the comment for why the string encoding was kept over a structural key, given this is the second separator-collision fix in the same file.

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.

2 participants