Repository navigation
refactor(ffi)!: separate announce input from route observation and trim the binding surface - #6473
Conversation
Quest-Claim: 0f07d6d5973a7b14
…unused binding surface
MoqBroadcastProducer::announce takes MoqAnnounceRoute { cost, epoch }; MoqRoute
is observation-only. MoqBandwidthConsumer reads rate/rate_changed. MoqGroupDemand
and MoqGroupRequest::demand are gone. resolve takes a MoqRendition. moq-c drops
moq_bandwidth_set_target and Kotlin drops its clientDefaults wrapper.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nothing calls it beyond the Python, Swift, and Go pass-throughs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The quest is implemented and
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: d18c77a4ba
ℹ️ 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>
|
@codex review (Written by Claude Opus 5.5) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review (Written by Claude Opus 5.5) |
|
@codex review (Written by Claude Opus 5.5) |
|
|
Codex Review: Didn't find any major issues. 🎉 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". |
# Conflicts: # dart/moq/MIGRATE.md # dart/moq_ffi/MIGRATE.md # dart/moq_ffi/lib/src/moq.dart # doc/lib/dart/index.md
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: bd0d7fdaa0
ℹ️ 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".
| pub async fn resolve(&self, rendition: MoqRendition) -> Result<Arc<MoqBroadcastConsumer>, MoqError> { | ||
| let (reference, epoch, _, _) = rendition.into_parts(); |
There was a problem hiding this comment.
For raw Kotlin callers upgrading across this Unreleased batch, kt/moq-ffi/MIGRATE.md:29 still instructs them to pass epoch after reference, but this new signature accepts a single MoqRendition; following the package's migration instructions therefore targets an overload that no longer exists. Update the Kotlin raw-binding entry to explain constructing and passing the rendition, as the other binding guides do.
AGENTS.md reference: AGENTS.md:L60-L61
Useful? React with 👍 / 👎.
Completes quest/m1/api-audit/binding-trim.md.
Problem
The moq-ffi surface, audited against
release, had inputs and observations sharing one type, two watch spellings, and objects with no consumer:MoqRoutewas both theannounceinput and whatMoqAnnounceConsumerreports. Itsanonymouswas ignored on input and itshopswas writable, so a local publisher could fabricate a relay hop chain.MoqBandwidthConsumerspelled its watchpeek/changed, whileMoqTrackDemanduseswanted/wanted_changed.MoqGroupRequest::demandandMoqGroupDemandhad no consumer beyond wrapper pass-throughs.MoqBroadcastConsumer::resolve(reference, epoch)took two adjacentOption<String>s that are easy to swap, while every other rendition consumer takes aMoqRendition.moq_bandwidth_set_targethad no consumer beyond its README, and moq-ffi'sMoqBandwidth::set_targetnone beyond the Python, Swift, and Go pass-throughs.Approach
MoqBroadcastProducer::announcetakes a newMoqAnnounceRoute { cost, epoch }.MoqRouteis observation-only: no defaults,anonymousderived fromhops. With no hop chain to validate,MoqError::InvalidRouteis gone (Go'sErrInvalidRoutewith it). The route docs no longer pair a route withMoqOriginProducer::dynamic, which takes none since moq-dev/moq#5678.MoqBandwidthConsumer::peek/changedbecomerate/rate_changed.MoqGroupRequest::demandandMoqGroupDemandare removed, with their wrappers.MoqBroadcastConsumer::resolvetakes aMoqRenditionand reads the reference and epoch from it.MoqBroadcastFinishdocs (moq-ffi, Go,doc/lib/{kt,go,dart}) say a garbage-collected language closes the broadcast only when the collector reclaims an unreferenced handle.moq_bandwidth_set_target. Native rung disable keeps a disabled rung's reservation at its ceiling instead of setting a target, and OBS goes through the generated C++ package, so nothing uses it. moq-ffi'sMoqBandwidth::set_targetgoes too, with the PythonBandwidth.set_target, SwiftBandwidth.setTarget, and GoBandwidth.SetTargetpass-throughs, and Kotlin, Dart, and C++ lose the generated method.moq_client_defaults: Kotlin's hand-writtenclientDefaults()goes. Swift, Python, Go, and Dart never wrapped it, and C++'s wrapper becomes the generated function under cpp-free-function-prefix, so every language reaches the generated function directly.doc/lib/*follow. A Python test that announced a fabricated hop chain now re-prices by cost.Impact
MoqAnnounceRouteadded;MoqBroadcastProducer::announce(MoqAnnounceRoute);MoqRoutefields lose their defaults;MoqError::InvalidRouteremoved;MoqBandwidthConsumer::{rate, rate_changed}replace{peek, changed};MoqGroupDemandandMoqGroupRequest::demandremoved;MoqBroadcastConsumer::resolve(MoqRendition).moq_bandwidth_set_targetremoved.MoqBandwidth::set_targetremoved, with PythonBandwidth.set_target, SwiftBandwidth.setTarget, GoBandwidth.SetTarget, and the generated Kotlin, Dart, and C++ method. NoMIGRATE.mdentry, since no release shipped it.moq.AnnounceRoute;BroadcastProducer.announce(AnnounceRoute);Bandwidth.Consumer.rate()/rate_changed();BroadcastConsumer.resolve(id, rendition);GroupDemandandGroupRequest.demand()removed.AnnounceRoute;announce(route: AnnounceRoute);Bandwidth.Consumer.rate()/rateChanged();resolve(_: Media.Rendition);GroupDemandremoved.AnnounceRoutealias;GroupDemandalias andclientDefaults()removed; generated methods renamed as in moq-ffi.moq.AnnounceRoute;Announce(AnnounceRoute);BandwidthConsumer.Rate()/RateChanged(ctx);Resolve(ctx, rendition);GroupDemand,GroupRequest.Demand,ErrInvalidRouteremoved.AnnounceRoutetypedef;GroupDemandtypedef removed; generated methods renamed; raw bindings regenerated.moq::AnnounceRoute,rate()).MIGRATE.mdentries in every affected released package. Wire: none.Alternatives
MoqRouteand refuse a non-emptyhopson announce: a runtime refusal for what the type can make unrepresentable.resolve(reference: MoqReference { broadcast, epoch }): a new record for one call, whenMoqRenditionalready carries both and is what the caller holds.moq_bandwidth_consumer_peek/_changed. Skipped: the hand-written crate is being replaced by the generated C API (moq-dev/moq#6052), which picks up moq-ffi's names.Decisions
moq_client_defaultsalignment: remove Kotlin'sclientDefaults()(its only consumer was its own test), so no wrapper hand-writes it and every language reaches the generated function. Rejected: addingclient_defaults()to Swift, Python, Go, and Dart, which would expose the FFIClientConfigthose wrappers otherwise hide.MoqBandwidth::set_target: removed with its wrappers, on the same no-consumer finding as moq-c's.Follow-ups
None new. The origin announces will move
announceoff the broadcast producer and can keepMoqAnnounceRouteas its input.Checks
just checkpasses (Swift skipped: no toolchain on this host).just test interop --allpasses.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code