Repository navigation
quest: plan the API changes worth making before the breaking cut - #6346
Conversation
From an external API audit: pull small breaks into m1 so they ship with the cut, add the FFI encoder config, warmup draft, migration guide and API-diff quests, saturate wall-time expiries, and order public routes after cluster routing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
|
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 3 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 |
# Conflicts: # quest/m2/README.md
|
@codex review (Written by Claude Opus 5.5) |
![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: d305b2624f
ℹ️ 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>
The API audit's MIGRATE quest already owns the binding guide fixes, so migrate-guides folds into it (adding the untimed-default entry). The audit epic's API-diff link points at the split-out m1 quest, and error-cleanups links the clean-close quest it touches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
(Written by Claude Opus 5.5) |
# Conflicts: # quest/m2/README.md
|
Merged main: (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: 0a23dd658c
ℹ️ 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".
| Public API: breaking in moq-ffi and every binding wrapper, each with a | ||
| `MIGRATE.md` entry. Wire: none. |
There was a problem hiding this comment.
Because this quest explicitly includes cpp/moq among the wrappers, requiring a MIGRATE.md entry for every wrapper directs the implementer to add a C++ migration that never occurred for users. The same commit's quest/m1/api-audit/migrate.md says to empty that file because the package has never been released, and cpp-free-function-prefix.md likewise says it needs no upgrade note. Exempt unpublished wrappers here so the quests do not impose contradictory completion criteria.
Useful? React with 👍 / 👎.
| `video_encoder_config` and `audio_settings` never read it. Nest it in a | ||
| publication options record taken by the producers, beside the existing | ||
| `catalog` and `bandwidth` arguments, rather than adding a fourth argument. |
There was a problem hiding this comment.
When this quest is implemented, both MoqVideoProducer::new and MoqAudioProducer::new already take four arguments: catalog, input, output, and bandwidth. Placing a new publication-options record beside the existing catalog and bandwidth arguments would therefore create a fifth argument, despite the plan claiming this avoids adding a fourth. Clarify which existing arguments the record absorbs so the planned API can satisfy the repository's argument-count rule.
AGENTS.md reference: AGENTS.md:L68-L69
Useful? React with 👍 / 👎.
Plans the findings from an external pre-release API audit that hold up against the code. Every audit claim was verified first: the integer framerate in moq-ffi, the ignored
trackon codec-only encoders,Wallrefusing expiries past 2554 in #6298, theanswer.mdvsroutes.mdconflict, the warmup draft vsdecode::Consumer, and the three MIGRATE.md errors.Changes
from-str-trait,drop-dead-items,error-cleanups,cpp-free-function-prefix. The cut breaks anyway, so these ship with it.binding-encoder-config[M]: a rational framerate, andtrackmoves to producer-only publication options.warmup-draft[XS]: the hang draft'swarmuprule is amended to match the shipping dual decoder. It was split from m2'splan-boundary.api-diff[L]: the tooling, split out ofmigrate-audit.m2/release/migrate-auditto the cut audit, and added the rule that each Unreleased section states the net published → final change.public-model/answer.mdsplit: the route-model items (Hop::from_wire,Cost::charged, thevia(hop)identity,excluding, the escape hatches, JS hop/advertise/receive) move to a newpublic-model/routes.md. It requirescluster-routing/routes.md, which replacesHopsand saturatingCost.api-audit/migrate.mdalready owns the contradictoryfinishentries and the missing async shutdown, so the plannedmigrate-guidesquest became one added bullet there (the wrong "timed by default"). The epic's API-diff link now points atm1/api-diff.md, anderror-cleanupslinksapi-audit/clean-close.md.time/wall.md: certificate and auth expiries saturate instead of refusing. Presentation walls still refuse.Declined from the audit
Position#[non_exhaustive]: moq-net breaks every release anyway, and the byte-holes quest may choose another shape.Decisions
trackon EncoderOutput?Public API / wire: none (quest files only).
quest checkpasses.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code