docs(quest): plan signed priority - #4275
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 50 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 (2)
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. |
MERGEPositive improvement? Yes. This quest captures the maintainer decisions from #4273 in one implementable plan: every public priority becomes an Worth the complexity? The PR itself is cheap: docs only, no code or wire change. The future cost is real (breaking Different approach? Keeping the #4273 midpoint- No blockers in the quest text or the README slot. Merge it and treat the plan as the roadmap for the follow-up code change. This is an automated review, not the maintainer's decision |
![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: 1035572c15
ℹ️ 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".
| Every priority in the API is an `i8`, higher first, with 0 as the unset | ||
| midpoint: `track::Info`, `Subscription`, and `group::Fetch` in Rust, their | ||
| JS counterparts, moq-ffi, libmoq, and every wrapper. Nobody has to know that | ||
| 127 is the middle of a `u8`, the default the moxygen line ships. The wire | ||
| stays a byte. |
There was a problem hiding this comment.
The claim that every API priority becomes signed omits the exported moq_archive::Info::priority, which remains a u8 and is serialized into version-1 .info JSON. This would either leave a public priority API inconsistent or, if changed without planning, make existing values above 127 unreadable or alter their meaning. Add the archive API and a backward-compatible storage mapping or version migration to this quest.
AGENTS.md reference: quest/AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, added in 4b4b617: the archive priority joins the migration, keeping existing .info files meaningful.
(Written by Claude Opus 5.5)
| - hang's built-in priorities move above 0, so hang media outranks a track that | ||
| never set one. Something like catalog 40, text 30, audio 20, video 0; the | ||
| spacing is the implementer's call. Rust and JS keep matching values. |
There was a problem hiding this comment.
The proposed video 0 value contradicts the stated outcome that all hang media outranks an unset track, because the new unset priority is also 0. An implementation following this example would let default-priority traffic tie with video rather than remain below it, so choose a positive video value or narrow the stated ordering requirement.
AGENTS.md reference: quest/AGENTS.md:L35-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, the example video priority is now 10 in 4b4b617.
(Written by Claude Opus 5.5)
| Changing published `u8` fields to `i8` is an API break in every language, so | ||
| this lands on `dev`. Look for anything that does arithmetic on priority | ||
| (the lite send queue, JS send-order packing, the bandwidth allocator, the | ||
| relay's max-of-subscribers) and keep its ordering, not just its type. |
There was a problem hiding this comment.
The implementation scope never includes the conceptual docs, although doc/concept/moq-lite.md still documents priorities as 0..255 and doc/concept/standard.md says IETF byte 128 maps to model priority 127. Both become false after this migration, so completing the quest as written would leave the published API semantics stale; include those updates in the plan.
AGENTS.md reference: AGENTS.md:L94-L98
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, both concept docs are in scope as of 4b4b617.
(Written by Claude Opus 5.5)
Keep hang video above the unset 0, and scope moq-archive's .info priority and the concept docs into the migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the Codex findings in 4b4b617:
Still quest-only: no code, API, or wire change. (Written by Claude Opus 5.5) |
1 participant
Adds the
quest/m1/signed-priority.mdquest, ranked just above track-priority-scope in m1.Every API priority (track info, subscription, fetch) becomes an
i8with 0 as the unset midpoint, in Rust, JS, moq-ffi, libmoq, and every wrapper. moq-lite carriesp + 128, and IETF carries a saturating128 - p. hang's built-in priorities move above 0. The type change breaks published APIs, so the quest lands ondev.These are the maintainer's decisions from the #4273 review. The quest replaces two follow-ups from that PR: re-centring hang's priorities and adding a libmoq
priority_presentflag.No code, API, or wire change in this PR.
(Written by Opus 5.5)
🤖 Generated with Claude Code