Repository navigation
feat(net): scope origin announcement interest - #4234
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe change adds rooted, pattern-limited producer and consumer views that translate paths between relative and origin-rooted forms. Wire advertisements carry scoped claims, and announcement forwarding subscribes to namespace prefixes derived from origin interests. The update also adds tests and benchmark cases for scoped routing and discovery, updates scope and announcement documentation, and removes the JS origin scope quest entry and page. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Scoped paths and announcement forwarding have no identified issue requiring a fix before merge. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Scoped views narrow publishing and discovery, but a scoped producer can still close the entire shared origin. That matters if an application gives a scoped producer to code that should not control other namespaces. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d06fd5eb82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const scope = scopes.find( | ||
| ({ pattern, head }) => | ||
| (exact ? pattern.matches(path) : scopeOverlaps(pattern, path)) && | ||
| (hidden || !hiddenBelow(head, path)), |
There was a problem hiding this comment.
When a broad dynamic route is created through a narrow producer, such as scope("", ["*/chat"]).dynamic(""), a disjoint scoped reader such as room/video still sees that route because this predicate checks only the reader's patterns against the advertised prefix. snapshot() and originated have already discarded the route's RouteEntry.scope, so the same invalid route is also forwarded when that disjoint consumer is used as a connection's publish origin, even though bestEntry later refuses every request under it. Preserve the originating scope in the derived advertisement and require it to overlap the reader scope to avoid false local and wire announcements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f61bda: routes now carry their producer scope's claim, and both local readers and the wire advertised view require it to overlap the reader scope, matching moq-net's RouteEntry::overlaps. Regression test added.
(Written by Claude Opus 5.5)
| const relative = Path.stripPrefix(this.root, path); | ||
| if (relative !== null) out.set(relative, value); | ||
| else if (!exact && Path.hasPrefix(path, this.root)) out.set(Path.empty(), value); |
There was a problem hiding this comment.
When more than one advertised prefix is above a rooted view, every ancestor is projected to the empty relative path and the last map entry silently wins. For example, if room/alice is inserted before room, a view rooted at room/alice advertises the broader room route's cost and hops even though requests resolve through the more-specific room/alice route; insertion order therefore changes routing metadata exposed to peers. Select the most-specific covering prefix when multiple entries collapse to the same relative key rather than overwriting unconditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f61bda: prefixes that collapse onto a rooted view's root keep the most specific one, in both broadcasts/announced and the wire view, matching moq-net's scoped_cursor_advertises_most_specific_covering_route. Regression test added.
(Written by Claude Opus 5.5)
…ost specific cover A route inserted through a scoped producer now carries its claim, so readers whose scope it cannot serve no longer see it, locally or on the wire. Prefixes collapsing onto a rooted view's root present the most specific one, matching the route requests resolve through. Both mirror moq-net. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge prep:
(Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f61bdaeb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const prefix of originWire.interests()) { | ||
| // Hidden routes are mirrored too; each local reader opts in on its own. | ||
| const announced = conn.announced(Path.Pattern.subtree(prefix), { hidden: true }); | ||
| const inserted = new Map<Path.Valid, Dynamic>(); |
There was a problem hiding this comment.
With multiple disjoint heads and a peer advertising a prefix above them, such as "" for a/** and b/**, every conn.announced stream emits the same advertisement, but this per-stream map creates a separate Dynamic and drive task for each copy. The IETF subscriber explicitly sends one broad advertisement to every matching consumer in #attachAnnounce, so route storage and repricing work grow with heads times routes; if one stream ends, closing the selected duplicate can also rematerialize active requests through an identical entry. Share or reference-count received prefixes across these streams, and benchmark interest heads against routes. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
# Conflicts: # js/net/src/origin.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7609212409
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const entry = preferredEntry(routes); | ||
| if (!entry) continue; | ||
| const value = { identity: entry.identity, route: entry.route.peek() }; | ||
| const value = { identity: entry.identity, route: entry.route.peek(), claim: claimOf(entry, path) }; |
There was a problem hiding this comment.
When two scoped dynamics advertise the same prefix but cover disjoint patterns, snapshot() chooses the globally preferred entry before #listed applies the reader's scope. For example, if cheap */chat and costlier */video routes both advertise "", a video-scoped reader sees no announcement even though bestEntry correctly routes room/video through the video entry. rebuildOriginated() makes the same early selection, so a connection publishing that scoped view also omits the usable route. Keep the candidates until scope filtering, then choose the preferred overlapping entry. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
forwardAnnounced now opens one announce stream per origin interest (#4234), so each stream takes its own replay hold and the origin's live marker waits for every interest's initial set. The live event is handled before the prefix filter, since it carries no prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
JS origins always requested the empty announcement prefix, even when an application only needed a specific namespace. They also lacked Rust's rooted permission scopes.
Approach
Add shared rooted origin views that intersect nested pattern scopes and enforce them for publishing, routing, requests, and wire-facing views. Subscribe once per distinct literal head, omit heads already covered by another subscription, and filter the received routes locally. Keep hidden routes on the transport subscriptions and leave visibility to each reader.
Impact
Origin.Producer.scope(root, patterns), returning a handle on the same origin with root-relative paths and narrowed permissions;consume()retains that view.Alternatives
Filtering announcement output alone would keep requesting the empty prefix and would leave publishing and blind requests outside the requested scope. Scope the shared routing boundaries instead.
Validation
nix develop --command just checkpassed, including all 912 net tests.just test interop --all: 27/32 cells passed. Four C cells could not build because the external mbx cache omittedtarget/include/moq.h; Python-to-browser stalled. Both blockers are also recorded in fix(test): isolate concurrent interop harness runs #4228.Follow-ups
The external C-header cache defect and Python/browser stall remain outside this change; #4228 records the same failures and investigation.
(Written by GPT-6)