Repository navigation
quest: plan the 2026-10-24 pre-cut audit findings - #6707
Conversation
Adds the m1 pre-cut audit epic (34 fixes from an eight-area audit of main at 151a529), ten m2 quests for the redesigns behind the highest-churn mechanisms and inherited costs, and audit evidence in the quests that already own subscribe ranges, the drop set, per-origin routes, the drafts 14-17 removal, and buffered playback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 18 minutes. View limit details
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: 0420e38410
ℹ️ 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".
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 0420e38
P2 — quest/m1/cut-audit/jitter-admission.md:25–26: bound admission by the newest queued/next candidate frame, not queue front. The oldest frame remains near its real-time departure while a 3x producer appends distant frames behind it; the proposed front > now+delay+lead check stays false and preserves the unbounded backlog. Specify tail/candidate-based admission and assert bounded queued state in the 1x/3x regression.
P2 — quest/m1/ietf-drop/d14-17.md:60–62: a scalar per-parity high-water mark cannot replace Spent without preserving out-of-order membership. Independent streams can deliver ID 4 before ID 2; later first use of 2 must remain valid without permitting reuse of spent lower IDs. Missing intervals also determine GOAWAY's smallest-unprocessed cutoff. Preserve these semantics or specify a protocol-compatible retirement bound, with reordered-arrival and GOAWAY tests.
Also confirmed the existing invalid-element-input refusal finding; reference rather than duplicate: #6707 (comment)
Separating concrete pre-cut fixes from larger ownership redesigns is sensible. These are planning defects, not runtime regressions introduced by this documentation-only PR.
Verification: static quest/diff and selected surrounding-code inspection only; no tests or reproduction runs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 057233a
The invalid-element-attribute refusal finding is addressed: the plan requires observable failure without defaulted execution. The Nix-cache verification clarification correctly waits for release to carry the tag workflow.
No new actionable findings in this update. The jitter-admission and reordered-request-ID planning findings in the previous review remain unchanged and open: #6707 (review)
Separating concrete pre-cut fixes from larger ownership redesigns remains sensible. Verification: static delta and relevant workflow inspection only; no tests or reproduction runs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 057233af2d
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7656f33bc
ℹ️ 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".
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e7656f3
The jitter-admission planning finding is fixed: admission checks the newest queued or incoming frame instead of queue front. No new actionable findings in this update.
The reordered-request-ID finding remains open and unchanged, now independently corroborated: #6707 (comment)
The correction fits bounded admission without extra machinery. Verification: static delta and surrounding-code reasoning only; no tests or reproductions run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 8c8eace
The reordered-admission part of the previous finding is addressed: lower, first-use IDs must remain valid, and dropping reuse detection is now an explicit design choice.
The prior finding remains partially open at quest/m1/ietf-drop/d14-17.md:60–68: missing-ID history also supplies GOAWAY's smallest-unprocessed cutoff, not just duplicate detection and Publisher::joins. Control::goaway uses modern.missing; its existing test accepts IDs 0 and 4 and requires cutoff 2. Specify how either replacement preserves that cutoff for legitimate reordered streams, and retain the GOAWAY regression. Related independent finding: #6707 (comment).
No additional actionable findings. Making the trade-off explicit improves the plan, but the GOAWAY contract must be included before removing history. Static delta and relevant control-code inspection only; no tests run.
The maintainer decided refusal wins over js/AGENTS.md's warn-and-fall-back rule; update the rule and record it in the element-attributes plan. The d14-17 request-ID choice must keep GOAWAY's smallest-unprocessed cutoff. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dae5b1dfc
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 885a01e
The remaining request-ID planning finding is addressed: the plan preserves GOAWAY’s smallest-unprocessed cutoff and reordered-arrival regression. Invalid-element-input refusal is consistent with updated JS guidance.
The independent lost-datagram finding is addressed: lite-07 retains bounded settlement for unseen datagram holes unless reliable sequence accounting replaces it: #6707 (comment)
No new actionable bugs found. Focused corrections preserve existing contracts without runtime machinery; larger redesigns remain separate.
Verification: static delta and relevant control-code/test inspection against last published review, accounting for main changes. No tests or reproductions run.
|
Maintainer decisions (2026-10-25), recorded in moq-dev/moq#6798; please apply them to this PR's quests:
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nfig Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…anning-700add Record the 2026-10-25 answers in the quests #6707 and #6790 landed: datagram size, description-less AAC, catalog parity, MKV/FLV codecs, partial groups, drop finality, and resume. decode-start stays under the hand-written m1/c line, and api-audit/requests renames moq-c too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Take #6744's export removals (Version::from_alpn, is_ietf, Error::TooManyParameters, SetupOptions remove*). The SETUP codec gates on is_lite() instead. Restore quest/m1/ietf-drop/d14-17.md, since #6707 added an open decision on the request-ID history to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
The maintainer asked for an extensive audit of main before promoting it to release, with quests for every issue and attention to high-churn areas that may need a redesign. This PR only adds and amends quests; no code changes.
Eight parallel audits at 151a529 covered the moq-net track/resume model, origin and sessions with kio, moq-transport, moq-lite with its codecs and fuzzing,
js/netwithjs/qmux, moq-mux with hang (Rust and JS), the browser media packages, and the runtimes and binaries. Each skipped what the 2026-10-22 to 24 audits already filed, and every finding was checked against the quest tree and the open PRs. I re-read the code for the top findings in each area before filing.m1: Pre-cut audit findings, 34 children
Most severe:
<moq-publish>removal: leaves camera and mic capturing; a cleanup registry pins every element.The rest cover correctness, Rust/JS parity, bounded costs, and cleanups. Five children carry a maintainer decision with a recommendation: datagram size, description-less AAC, catalog strictness, the MKV unsupported-codec rule, and the partial-group rule.
m2: redesigns behind the churn
Main landed about 2,400 commits in 30 days, 823 of them fixes, concentrated in
rs/moq-netandjs/net. Each redesign below is internal (no API or wire change), so the audit placed them after the cut:resume.rssince the per-reader design replaced splice on 2026-10-04.Also new in m2: media parser fuzzing, announce interest memory, resumed-read budget, and transcode rungs.
Amended quests
Audit evidence was added to subscribe ranges, drop set, per-origin routes, drafts 14-17 removal, and buffered playback.
Decisions
js/AGENTS.mdnow says an invalid attribute value is refused (the setter throws and the element surfaces the error) instead of warning and falling back, and the quest's Plan records the decision.Decisions for the maintainer
resume.rsis new at this cut. The audit recommends after the cut, to keep the hard-won test corpus intact right before release.Review follow-ups
Impact
Public API: none (quests and one
js/AGENTS.mdrule). Wire: none.Checks
quest checkpasses;just checkpassed.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code