Conversation
Delete the in-tree packaged fixture quest. Record each 2026-09-12 audit disposition in merge-dev, add migration notes beside Connection, catalog Snapshot, JSON modes, and moq-native, and cover relay embedding with a custom-route plus origin-worker test. Co-Authored-By: grok-4.6 <noreply@x.ai>
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Putting the unpublished-API proof in smoke, instead of an in-tree packaged fixture, is the right split. Smoke already owns "does a real consumer see the exported surface"; this repo should not grow a second copy of that. Migration notes beside Connection, catalog Snapshot, JSON modes, and moq-native → moq-tokio are the part that belongs here. The demo frame → frames fix is correct (RendererInput.frames). Worth doing.
Complexity is fine if the gate stays honest. Deleting /quest/m1/api-release-proof.md and dropping it from merge-dev Required before smoke#36 is green overclaims the proof. embed.rs is a good public example of Relay::load + custom /app + cluster.origin; it is not the owning runner the embedding quest still requires, and the stop path currently aborts the serve task.
Would I do it differently? Same split. I would keep a Required line until from-dev has a recorded passing revision, cite #3645 (not only #3651) for the FFI lane finding, and wait on web.serve after shutdown_trigger instead of aborting.
Merge after: smoke#36 from-dev is green, the merge-dev table cites the actual fixing PRs, and embed shutdown is a real stop or the docs stop calling it one. Not a rethink.
(Written by grok-4.6)
| The 2026-09-12 external API audit is closed. Proof is | ||
| [moq-dev/smoke](https://github.com/moq-dev/smoke) `./dev.sh` (unpublished | ||
| `dev` checkout, not crates.io/npm latest) plus | ||
| `rs/moq-relay/tests/embed.rs` for custom routes and origin workers. |
There was a problem hiding this comment.
"The audit is closed" plus dropping this quest from Required is stronger than the evidence in this PR. The packaged-consumer proof is moq-dev/smoke#36, which is still open, and that job checks out moq dev rather than a recorded revision.
The original gate was "fixture in CI + exact revisions." Pointing at ./dev.sh is the right surface, but merge-dev should stay blocked on a green from-dev run (or keep a Required line that names smoke#36 / the SHA it passed on). Closing the audit here lets dev merge with only a link to an unmerged sibling.
Also say what smoke does not cover: JS-only against a local relay, no native crate consumer, no two-copy package-identity case. Those were explicit non-goals after the smoke split; write that down so the next merge does not treat them as done.
(Written by grok-4.6)
|
|
||
| | Finding | Disposition | Revision | | ||
| |---|---|---| | ||
| | FFI pending reads serialize independent datagram/group lanes | Fixed: independent group and datagram lanes | `1f7b2b45b` (#3651) | |
There was a problem hiding this comment.
The independent-lane fix is #3645 (fix(ffi): let group and datagram reads progress independently). #3651 / 1f7b2b45b is the follow-up that keeps the same-lane guard alive for the whole read so two next_groups cannot overlap.
Cite #3645 as the disposition for this row (and #3651 only if you want the follow-up on the same line). As written, the table points at the wrong revision for the audit finding.
(Written by grok-4.6)
| shutdown_trigger.start(); | ||
| web_handle.abort(); | ||
| let _ = web_handle.await; | ||
| drop(cluster); | ||
|
|
||
| let deadline = std::time::Instant::now() + Duration::from_secs(2); | ||
| loop { | ||
| if TcpListener::bind(("127.0.0.1", port)).is_ok() { | ||
| break; | ||
| } | ||
| if std::time::Instant::now() >= deadline { | ||
| panic!("stopping the embedder should release the HTTP listener"); |
There was a problem hiding this comment.
shutdown_trigger.start() then web_handle.abort() does not prove an embedder stop. Aborting the serve task skips axum shutdown, so the later bind check is "the task died," not "Drop/shutdown released the listener."
Wait on web.serve after the trigger (and fail on a serve error) before asserting the port is free. Same for cluster: drop is fine, abort is not the owning-runner story the embedding quest still owns.
This test is still useful as the public route + origin-worker example. It should not be described as covering embedder stop semantics.
(Written by grok-4.6)
![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: 9efa24fd8b
ℹ️ 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".
| cluster, | ||
| shutdown: _, | ||
| shutdown_trigger, | ||
| .. |
There was a problem hiding this comment.
When runtime.workers is unset, as it is here, Relay::load binds the normal QUIC listener into Relay::server; this .. immediately drops that server, and the test then starts only web.serve and connects explicitly over WebSocket. The advertised public embedding example therefore silently disables QUIC/WebTransport while still passing. Retain and drive server alongside the custom web router, and verify the worker-owned variants separately.
Useful? React with 👍 / 👎.
|
|
||
| ```ts | ||
| const track = broadcast.track(Catalog.TRACK).subscribe({ priority: Catalog.PRIORITY.catalog }); | ||
| const catalog = new Json.Snapshot.Consumer<Catalog.Root>({ |
There was a problem hiding this comment.
The imports shown above this new example define only Catalog and Container, but the example dereferences Json. A consumer copying the documented migration receives Cannot find name 'Json' at build time, or a ReferenceError in untyped JavaScript, before it can read the catalog. Add the @moq/json namespace import to the snippet.
Useful? React with 👍 / 👎.
1 participant
Problem
The 2026-09-12 API audit required an external packaged-consumer proof and an explicit fix or deferral for every finding before
devmerges. An in-tree fixture would not exercise unpublished exports the way a real consumer does.Approach
Proof lives in moq-dev/smoke#36
./dev.sh(from-dev channel: path or gitdev, not crates.io/npm latest). This PR records that, deletes the quest, and covers what smoke cannot host./quest/m1/api-release-proof.mdand drop it from merge-dev Required.broadcast.track().subscribe(), Ordered, JSON Snapshot/Window, hang catalogs, andmoq-native→moq-tokio.rs/moq-relay/tests/embed.rs: custom HTTP route plus an origin worker. Worker/uring ownership stays on the embedding quest.frame→frames) so JS check of the docs still typechecks.Public API: no additional API change beyond the fixing quests. Wire: none.
Alternatives
A new in-tree packaged fixture, as the quest first asked. Rejected: smoke already owns that proof surface, and embedding the relay crate does not belong there.
(written by grok-4.6)