Repository navigation
Conversation
rind's m0 adapter fixes change calls MoQ's main makes and ship as patch releases inside MoQ's caret requirements, so the first lockfile update after them stops building. Planned from rind's quest audit of 2026-11-09. Co-Authored-By: Claude <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 5 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 663905efd809976fc7886347f7c3c84b031d11e5.
No actionable defects found in the two changed quest documents. The proposed provider and key-generation/serialization replacements match the inspected source in rind #521, #519, and #515 and the listed MoQ production call sites. Raising the minimum adapter versions together with the call-site changes is the right direction; keeping rind 0.4 separate avoids expanding this migration.
Limits: GitHub-only static review. No builds, tests, or quest checks were run, and the repository-wide call-count claims were not independently reproduced. The upstream PRs remain unmerged, so this reviews the coordination plan, not the eventual integration or release readiness.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only — the plan's call-site inventory checks out; three small corrections inline.
Reviewed changes
quest/m0/rind-adapters.md— new quest planning MoQ's adaptation to three rind adapter PRs (rind#521, #519, #515) that make each adapter's API its upstream's, published as hand-set patch releases inside MoQ's caret requirements.quest/m0/README.md— one## Requiredbullet linking the new quest.
Verified
- Every shipped call site and grep count in the plan is exact at this head:
rind_rustls::default_provider()105 lines/40 files,KeyPair::generate(&78/38,to_pkcs8().secret_pkcs8_der()60/33, plus exactly 5 baredefault_provider()calls in the twouse-importing test files. The remaining hits are tests/benches (moq-rtmp/src/server.rs:3030is inside#[cfg(test)]). - No MoQ call to anything else the three PRs remove; the 3 non-rcgen
to_pkcs8()calls are rind core key types, as claimed. - The migration shapes match the open PRs (
DEFAULT_PROVIDERas astatic CryptoProvider,generate_for,serialize_der/serialized_der), andcrate::cpu::check()staying inmoq-crypto/src/provider.rspreserves the refusal that rind-rustls drops. - "Not a shipped release" holds: crates.io
moq-auth 0.2.4/moq-token 0.7.4requireaws-lc-rs, andorigin/release:Cargo.tomlnames no rind crate. - The body's "Noticed, not changed" is accurate: rind#510 merged and deleted
rind-instant-acme, whilequest/m5/rind-instant-acme.mdand its consumerquest/m5/relay-acme.mdstill wait on it.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| @@ -0,0 +1,112 @@ | |||
| # [S] MoQ builds against rind's corrected adapters | |||
|
|
|||
| ## Goal | |||
There was a problem hiding this comment.
The title sizes this [S] while the PR body's ## Models says Size: XS; recent quest PRs keep the two in agreement. Given the ~243 mechanical edits across ~80 files plus the requirement and lockfile changes, S looks right, so the body likely needs updating.
| `main` once the three PRs merge. The rind-rcgen PR and the one for | ||
| rind-jsonwebtoken and rind-object-store are still drafts, so re-read each | ||
| PR's Public API section when it merges; a signature may have moved. |
There was a problem hiding this comment.
| `main` once the three PRs merge. The rind-rcgen PR and the one for | |
| rind-jsonwebtoken and rind-object-store are still drafts, so re-read each | |
| PR's Public API section when it merges; a signature may have moved. | |
| `main` once the three PRs merge. All three are ready for review rather than | |
| drafts, so re-read each PR's Public API section as it merges; a signature may | |
| have moved. |
rind deleted rind-instant-acme as having no consumer yet, so the wait-only quest pointed at a quest that no longer exists. Co-Authored-By: Claude <noreply@anthropic.com>
rind set 0.4.0 by hand and holds its release for the CPU-check removal, so the adapter fixes publish with it and each requires rind 0.4. Co-Authored-By: Claude <noreply@anthropic.com>
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 56cebc5
Incremental review since: 663905e
Base unchanged: d9800c1
[P3] Reconcile the provider API promise with deleting its error type — quest/m0/rind-adapters.md:135
The new plan recommends deleting UnsupportedCpu but still promises unchanged moq_crypto::provider signatures. At this head, provider() returns Result<Provider, UnsupportedCpu> and install_default() returns Result<(), UnsupportedCpu> (rs/moq-crypto/src/provider.rs:17,34). Both promises cannot hold together. Update the public-API scope to include these return-type changes; making the functions infallible matches the proposed removal of the only failure source and avoids preserving a meaningless error.
The combined rind 0.4/adapter migration now matches the release decision in rind #548, superseding my earlier comment about keeping 0.4 separate. Moving ACME adapter planning to its future consumer is also consistent with rind #510.
Limits: GitHub-only static review of this planning delta and relevant source/upstream decisions. No builds, tests, quest checks, or release integration were run. COMMENT only, not approval.
There was a problem hiding this comment.
Important
The new rind 0.4 section's CPU-check inventory is incomplete: it names 7 of the 14 moq_crypto::cpu::check() callers and none of the vendored providers' five direct rind::cpu uses, so an implementer following it would miss a dozen compile errors.
Reviewed changes
quest/m0/rind-adapters.md— folds rind 0.4 into the quest: the goal, plan, non-goals, the CPU-check/agreement/CBC changes, and the## Waitingrelease condition now cover 0.4 alongside the four adapter fixes.quest/m0/README.md— the required bullet now names rind 0.4 as well.quest/m5/relay-acme.md— records that relay ACME plans its rind adapter when it starts, and waits on instant-acme 0.9 instead of the deleted adapter quest.quest/m5/rind-instant-acme.md— deleted, with itsquest/m5/README.mdbullet.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| - No runtime CPU check: `rind::cpu::check` and `rind::cpu::MissingFeature` are | ||
| gone, since a backend now requires only its target's compile-time features. | ||
| `rs/moq-crypto/src/cpu.rs` wraps both (`UnsupportedCpu` at line 6, `check()` | ||
| at line 18), and its callers are `rs/moq-crypto/src/provider.rs:22` and | ||
| `:48`, `rs/moq-crypto/src/e2ee/credential.rs:60`, | ||
| `rs/moq-auth/src/crypto.rs:6`, `rs/moq-hls/src/server/mod.rs:188`, | ||
| `rs/moq-quic/src/config/endpoint.rs:122`, and `rs/moq-rtmp/src/dial.rs:84`. | ||
| Recommended: delete `moq_crypto::cpu::check` and `UnsupportedCpu` and every | ||
| call, keeping the `Dit` re-export, rather than leave a check that cannot | ||
| fail. That removes public items from moq-crypto, so say so in the PR. |
There was a problem hiding this comment.
The plan's job is to name every site rind 0.4 breaks, and this list is short by half: seven more moq_crypto::cpu::check() callers exist, and the two vendored providers call rind::cpu::check() and name rind::cpu::MissingFeature directly. An implementer following the list would hit a dozen compile errors the plan says it already covers.
Technical details
# rind 0.4's CPU-check removal touches more sites than listed
## Affected sites
- `rs/qmux/src/tls.rs:76` — `connect` calls `moq_crypto::cpu::check()`.
- `rs/moq-uring/src/quic/driver/mod.rs:126` — `server_config` calls it.
- `rs/moq-hls/src/export/mod.rs:147` — `Broadcaster::new` calls it.
- `rs/moq-webrtc/src/session.rs:674` — `rtc_config` calls it.
- `rs/moq-rtmp/src/dial.rs:104` — `with_stream` calls it (line 84 is listed, 104 is not).
- `rs/moq-rtmp/src/server.rs:269` and `:499` — `bind` and `accept_stream` call it.
- `rs/moq-webrtc/src/vendor/dimpl/crypto/rind/mod.rs:24-25` — returns `rind::cpu::MissingFeature` and calls `rind::cpu::check()`.
- `rs/moq-webrtc/src/vendor/str0m_rind/lib.rs:19-20` — the same pair.
- `rs/moq-webrtc/src/vendor/dimpl/error.rs:363` — `UnsupportedCpu(rind::cpu::MissingFeature)`.
- `rs/moq-crypto/src/cpu.rs:17` — the wrapper `check()` is on line 17, not 18.
## Required outcome
- The plan names every site the 0.4 bump breaks, including the two vendored providers, or states explicitly which sites the 0.4 PR leaves alone and why.| `rind_rustls::default_provider()`. `crate::cpu::check()?` on line 22 stays | ||
| until rind 0.4. |
There was a problem hiding this comment.
crate::cpu::check()? on line 22 cannot stay once this quest also moves rind to 0.4: 0.4 deletes cpu::check, so the same PR removes this call. The sentence was true when the 0.4 move was separate and now contradicts the recommendation on lines 113-115.
| - moq-dev/rind: publishes rind 0.4.0 and the adapter patch releases in one | ||
| release, once its CPU-check removal has landed and this PR is ready. Check | ||
| that `cargo search rind` shows 0.4.0 and that `cargo search rind-rustls`, | ||
| `rind-jsonwebtoken`, `rind-object-store`, and `rind-rcgen` each show a | ||
| release that requires it; then set the requirements, update `Cargo.lock`, |
There was a problem hiding this comment.
cargo search reports a crate's latest version and description, not its dependency requirements, so the adapter half of this wait cannot be checked with the command named. Name a check that reads the dependency, such as the crates.io dependencies endpoint or cargo tree after raising the requirement.
Plans one quest and settles a stale one.
New: MoQ builds against rind 0.4 and its corrected adapters. rind's m0 epic makes each adapter's API exactly its upstream's, and those fixes now publish in the same release as rind 0.4, each adapter requiring it. So one MoQ PR moves to both, ready before they publish:
default_provider()in three adapters and rind-rcgen'sKeyPair.rind::cpu::checkandMissingFeature, whichmoq_crypto::cpuwraps and seven call sites use, and changesagreement::PrivateKey::public_key(), which the vendored dimpl provider calls.The quest lists every shipped call site and what it becomes. No crate MoQ has published names a rind adapter yet, so
mainand the next cut are exposed, not a shipped release.Settled: relay ACME plans its rind adapter when it starts.
quest/m5/rind-instant-acme.mdwaited on a rind quest that no longer exists, since rind deleted the adapter in moq-dev/rind#510 as having no consumer. The wait-only quest is deleted, andquest/m5/relay-acme.mdsays it begins by planning the adapter in rind, and waits on instant-acme 0.9 (0.8.5 is the latest today).Decisions
Decided with the maintainer on 2026-11-09, in rind's quest audit (moq-dev/rind#546):
rind-instant-acmewhen it starts (rejected: re-plan the adapter in rind now, ahead of its consumer; leave the stale wait).Public API and wire
None. Quests only. The planned PR may remove
moq_crypto::cpu::checkandUnsupportedCpu, which the quest flags.Models
Size: XS
Implementer: Claude Code, claude-fable-5-2, high effort
Review
Rewritten by
quest reviewafter each round.01a1c2e7-461a-71e1-b159-c2c3a87ff6ed, round 2The incremental changes only update coordination plans and remove a stale waiting quest, with no actionable defects or runtime changes.🤖 Generated with Claude Code
(Written by Claude Fable 5 with high effort)