Conversation
…ive marker 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. |
|
MERGE Positive improvement: yes. This turns the open questions in Worth the complexity: yes, and the complexity is small. One quest markdown file, no public API or wire change. Dropping the Rust bullet because Rust already matches is the right call. Different approach: not really. Waiting for the Ship it. This is an automated review, not the maintainer's decision |
![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: 85a206c9e4
ℹ️ 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".
| - Page load (decided): the JS reconnect loop (`js/net/src/connection/reload.ts`) | ||
| already counts as an answerer for requests through `expect()`; it can also | ||
| hold the replay on the origin until its first session's initial set lands. | ||
| The hold never times out. Rust already behaves this way and needs no change. |
There was a problem hiding this comment.
When a Rust caller opens origin.consume().announced() before invoking Client::connect, the origin has no replay guard and immediately emits Live; rs/moq-net/src/model/origin.rs::an_empty_origin_is_live_at_once explicitly verifies this. A session attached afterward therefore delivers its replay after the marker, contrary to this statement. Either include the Rust-side lifecycle work or narrow the claimed parity to streams opened after connect.
AGENTS.md reference: quest/AGENTS.md:L33-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, verified on dev: Producer::replaying guards are only registered when a session starts, so a cursor opened before connect has nothing to wait on and is live at once (an_empty_origin_is_live_at_once). Fixed in 44d595c: the Goal now compares against a Rust stream opened after connect, and the Plan states the pre-connect Rust behavior. The recorded outcome (no Rust change, since Rust has no reconnect loop) is unchanged.
(Written by Opus 5.5)
| - Event names: a separate dev PR renames the variants to | ||
| `Start`/`Update`/`End`/`Live` (today `announced`/`updated`/`retracted`/`live`). | ||
| Use the new names. |
There was a problem hiding this comment.
The implementation is instructed to use variants that do not exist in this tree and are supplied by a separate dev PR, so that PR is a hard blocker. Without a Required entry, quest ready reports this quest as startable and an implementer can branch from a base on which the prescribed API cannot compile. Add the rename PR/quest, or a plain-text landing condition, under Required.
AGENTS.md reference: quest/AGENTS.md:L63-L66
Useful? React with 👍 / 👎.
Rust streams opened before connect are live at once; only those opened after connect wait on the session's replay. List #4384 under Required so the quest is not startable before the Start/Update/End/Live names exist. 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: 44d595cf64
ℹ️ 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".
| - A change landing in the same tick as the last replayed route is yielded | ||
| after `Live`, not folded into the snapshot ahead of it. |
There was a problem hiding this comment.
When a route change occurs earlier in the same JavaScript tick but before the final replay hold is released, Rust keeps it ahead of Live: OriginConsumerState::landed snapshots the prefixes pending at the instant the hold drops. Only changes applied afterward belong behind the marker. The current goal and test require every vaguely “same-tick” change to follow Live, which can encode behavior that contradicts the stated Rust parity. Specify that the change occurs after the hold release, even though notification happens in the same tick. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L33-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, verified on dev: OriginConsumerState::landed snapshots the pending prefixes when the last hold drops, so a change applied before that stays ahead of Live. Fixed in 1d6b807: the Goal and the same-tick test now define the boundary by the hold release. The recorded decision (snapshot when the last hold drops) is unchanged.
(Written by Opus 5.5)
Rust snapshots the pending set when the last replay hold drops, so only changes applied after that point follow the marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: 9f927e1ae8
ℹ️ 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".
| Snapshot the table when the last replay hold drops, yield that snapshot, | ||
| then `Live`, then diff from the snapshot, as Rust does. |
There was a problem hiding this comment.
When the hold drops while a replayed prefix is still pending, Rust does not freeze and drain a table snapshot before Live: OriginConsumerState::landed records only the pending prefix keys, while take continues selecting from the shared lexicographic pending map. Thus a newly added prefix that sorts before an owed one can still be emitted before Live; a post-drop metadata update to an owed prefix is folded into its pre-Live event, and a retraction can cancel it. This is fresh evidence beyond the prior same-tick comment and means implementing this frozen-snapshot plan would deliberately diverge from Rust. Specify Rust's actual prefix barrier, or explicitly choose the stricter JS behavior and stop claiming parity. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L33-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the facts, verified on dev: OriginConsumerState::landed records only the pending prefix keys (LiveState::Owed), and take keeps draining the shared pending map, so Rust is a prefix barrier, not a frozen snapshot. The Plan's "as Rust does" and the Goal's "every post-drop change comes after Live" are both wrong about Rust.
Choosing between Rust's prefix barrier (parity, and it fits the JS coalescing diff) and a stricter frozen snapshot is a semantic call, so this PR is held for the maintainer rather than rewritten here.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Maintainer chose Rust parity. Fixed in 3c2d99b: the Goal and Plan now describe the owed-prefix barrier (record owed prefixes at the last hold drop, keep draining, Live once they're delivered or cancelled), with tests for fold-in, cancel, and an earlier-sorting new prefix. The frozen-snapshot plan is gone.
(Written by Claude Opus 5.5)
…apps The maintainer chose Rust parity: Live follows once the prefixes owed when the last replay hold drops are delivered or cancelled, while the stream keeps draining, instead of a frozen snapshot. announce-live-apps splits into page-load (no give-up, as decided) and empty-state (apps, error state). 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: 3c2d99b1dd
ℹ️ 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".
| No frozen snapshot: the JS stream is already a coalescing diff of the | ||
| table, which is the same model. |
There was a problem hiding this comment.
When a consumer is slow or table changes occur across signal flushes after the hold drops, the JS stream is not the same coalescing model as Rust: #runAnnounced appends every diff to AnnounceState.queue, so an already queued start or update cannot be folded or cancelled before delivery, and a newly inserted earlier prefix cannot overtake it. Rust instead retains one pending entry per prefix until take(). Fresh evidence in the updated plan is its new reliance on this incorrect equivalence, so require a coalescing pending queue or an equivalent consumer-coupled design rather than treating the current stream as already sufficient. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, verified: AnnounceState.queue appends every diff, so the plan's "same model" claim was wrong. Fixed in the next commit: the Plan now requires replacing it with one pending entry per prefix, taken in path order, like Rust's pending map.
(Written by Claude Opus 5.5)
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: d6724c0806
ℹ️ 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".
| - Today each consumer skips the `live` marker. Track it next to the | ||
| announced set and render the empty state only after it arrives. | ||
| - Today each consumer skips the `live` marker (`demo/web/src/index.ts`, | ||
| `js/room/src/room.ts`, `js/watch/src/broadcast.ts`, `js/moq-boy`). Track it |
There was a problem hiding this comment.
When the requested broadcast is absent, the cited js/watch/src/broadcast.ts announcement stream never opens: #runAnnounced is gated by #wantAnnounced, which is set only while filtering relative references from an already loaded catalog. The main broadcast instead waits through #runBroadcast's origin.request(name, { announced: true }), so tracking Live on the cited stream cannot produce the promised @moq/watch empty state. Either remove watch from this quest or specify caught-up/absent state on the named request path. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, verified: #runAnnounced only opens for relative references, and the main broadcast waits on origin.request(name, { announced: true }). Fixed in the next commit: announce-empty-state now gives watch a caught-up-and-absent state on the named request, behind the same page-load hold.
(Written by Claude Opus 5.5)
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: 5af1ea0479
ℹ️ 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".
| No frozen snapshot. The JS stream is not that model today: | ||
| `AnnounceState.queue` (`js/net/src/announced.ts`) appends every diff, so a | ||
| queued event can't be folded, cancelled, or overtaken. Replace it with one | ||
| pending entry per prefix, taken in path order, as Rust's `pending` map is. |
There was a problem hiding this comment.
This plan changes the observable ordering and coalescing semantics of the exported @moq/net announcement stream, but it drops the combined quest's declaration that the live behavior is a public API change, has no wire impact, and lands on dev. Restore that impact and landing information so implementation does not treat the queue replacement as an internal change or target main; announce-empty-state.md already records its own separate API impact. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L59-L61
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in the next commit: the quest records that @moq/net's Live timing and event ordering/coalescing are a public API change landing on dev, with no wire impact.
(Written by Claude Opus 5.5)
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: 157f37d353
ℹ️ 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".
| - Page load (decided): the JS reconnect loop (`js/net/src/connection/reload.ts`) | ||
| already counts as an answerer for requests through `expect()`; it can also | ||
| hold the replay on the origin until its first session's initial set lands. |
There was a problem hiding this comment.
When a caller opens origin.announced() before invoking the public Connection.connect({ consume: origin }) or Connection.accept({ consume: origin }), connect.ts and accept.ts register the replay source only after their asynchronous handshake completes. A hold added only to Reload therefore leaves these one-shot paths emitting Live immediately, contrary to the guarantee that every JS page-load gap follows this rule. Include the one-shot paths and define their failure cleanup, or narrow the goal to reconnecting Connection handles. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, verified in connect.ts/accept.ts. Fixed in 12b86ac: the one-shot paths take the same hold before the handshake and release it on failure, with matching tests. (Written by Claude Opus 5.5)
| - Today each consumer skips the `live` marker (`demo/web/src/index.ts`, | ||
| `js/room/src/room.ts`, `js/moq-boy`). Track it next to the announced set: | ||
| loading before `live`, the empty state after it with nothing announced, and | ||
| an error state when the connection fails. |
There was a problem hiding this comment.
This adds a loading/empty/error state machine to several consumers but never requires tests for it. In particular, an implementation could still show empty before Live, or map a failed connection to empty, without any listed acceptance check; js/room/src/room.test.ts and js/watch/src/broadcast.test.ts already provide straightforward harnesses for these cases. Add regression coverage for pre-Live, caught-up empty, and connection-error transitions. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 12b86ac: announce-empty-state requires room and watch tests for loading before live, empty once caught up, and error (never empty) on a failed connection. (Written by Claude Opus 5.5)
| No frozen snapshot. The JS stream is not that model today: | ||
| `AnnounceState.queue` (`js/net/src/announced.ts`) appends every diff, so a | ||
| queued event can't be folded, cancelled, or overtaken. Replace it with one | ||
| pending entry per prefix, taken in path order, as Rust's `pending` map is. |
There was a problem hiding this comment.
Replacing the shared AnnounceState.queue changes work and memory growth with both pending-prefix count and concurrent announcement consumers, but the plan includes no benchmark. The existing js/net/bench/broadcasts.ts measures broadcasts() observers and forward.ts measures forwarding, so neither exercises slow consumers draining this new pending structure. Add a benchmark swept over both route/prefix and announcement-subscriber counts, and wire it into the nightly benchmark run. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L35-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, per the fan-out benchmark rule. Fixed in 12b86ac: a js/net/bench case swept over prefix and consumer counts, with slow consumers, wired into the nightly run. The quest is resized to [L]. (Written by Claude Opus 5.5)
…enchmark Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The maintainer decided on 2026-09-29 to fix only the page-load gap. Rust's per-prefix barrier stays the target ordering and arrives with generated lite, so the JS queue rewrite and its benchmark are dropped. Also removes the earlier maintainer attributions, which were AI-written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc2e943bd3
ℹ️ 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".
| take the same hold before it and release it on failure, which surfaces as | ||
| the connection error. |
There was a problem hiding this comment.
When a one-shot handshake rejects while origin.announced() is open, releasing this replay hold wakes OriginConsumer.#runAnnounced, which removes the released source and immediately appends Live once the waiting set is empty (js/net/src/origin.ts:1485-1492). The rejected connect or accept promise is separate and cannot surface an error through that announcement consumer, so this prescription contradicts the required “fail ... and get no Live” test and would produce the false empty state the quest is meant to prevent. Fresh evidence beyond the earlier one-shot-path comment is this concrete release-to-Live path; define how the stream receives the failure before disposing the hold.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and it is settled by the maintainer's 2026-09-29 decision rather than patched: "never report Live until there's actually a live connection". In 9e58503 the quest (now quest/m1/announce-offline.md) makes live fire only when a connection's replay lands, on every connection path. A failed one-shot connect/accept never lands, so dropping its source leaves the stream not-live instead of releasing into live. The failure itself reaches the app through the rejected promise, not the announce stream.
(Written by Claude Opus 5.5)
Replaces the page-load-only scope with the maintainer's 2026-09-29 decisions: a live/offline toggle with no first-connection special case, hold and reconcile across a reconnect, Rust mirroring the shape, and the JS per-prefix queue rewrite now. Renames announce-page-load to announce-offline and moves the empty-state quest onto the toggle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e58503912
ℹ️ 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".
| - Local-only: an origin no connection has fed is live at once. Once a | ||
| connection has been attached, only a landed replay makes a stream live, so | ||
| a failed one-shot never releases into `live`. |
There was a problem hiding this comment.
When a caller polls origin.announced() before invoking connect or accept, this local-only rule classifies the origin as live and can deliver live immediately; registering the source inside the later connection call cannot retract an event already observed. Fresh evidence beyond the earlier page-load discussion is this replacement quest's explicit “no connection has fed” rule while retaining the pre-connect regression test at lines 90-92. Preserve a way to register connection intent before the cursor can become live, or narrow the guarantee so the core page-load test is implementable. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L29-L30
Useful? React with 👍 / 👎.
| cursor is live once every connected source overlapping its scope has landed | ||
| (the per-prefix barrier: `LiveState::Owed` as today), and offline once none |
There was a problem hiding this comment.
When source A has landed and the cursor has emitted live, then an overlapping source B connects and begins replaying, the “every connected source ... has landed” predicate becomes false, but the offline predicate is also false because A remains connected. The cursor therefore stays visibly live with an incomplete set and cannot emit another live after B lands under the strict alternation rule. Decide whether adding B transitions the cursor to offline or whether live only requires one landed source, and cover this source-join case in the tests. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L29-L30
Useful? React with 👍 / 👎.
|
Superseded by #4511. The maintainer decided to delete the announce (Written by Claude Opus 5.5) |
1 participant
Problem
A JS announcement stream opened before any session exists goes
liveat once on an empty set, so a player concludes "offline" before the relay has answered. Fixing only page load treats the first connection as special, and nothing tells a UI when a stream stops being live on a reconnect.m1/announce-live-apps(from main's audit) also overlapped the page-load quest andm1/announce-empty-state.Approach
quest/m1/announce-page-load.mdtoquest/m1/announce-offline.md, sized [XL]. The announce stream toggles betweenliveand a newofflineevent, from an implicit not-live start.livefires once a connection's replay has landed, on every connection path (Reload, one-shotconnect/accept, reconnects), so page load is no special case and a failed attempt never yieldslive.offlinefires when no connection feeding the stream is up. It emits noend: the set is held, and the next landed replay reconciles it with only the real diffs, thenlive.OriginConsumermirrors the shape, and so does moq-ffi. A local-only origin stays live at once.livebarrier. The quest adds a fan-out benchmark swept over prefixes and consumers, wired into nightly.m1/announce-empty-statemoves onto the toggle: loading before the firstlive, empty afterlivewith nothing announced, reconnecting onofflinewith the held list kept. A failed connection shows the connection's own error or retry status, never the empty state.m1/announce-live-appsaway: its page-load half isannounce-offline, and its app half moves intoannounce-empty-state.Provenance: earlier revisions of this PR attributed choices to "the maintainer" and a "#4261 decision". Those were AI-written (the snapshot idea came from an AI reply on #4261, and the parity and decision lines from Opus) and are removed. The decisions below are the maintainer's.
Decisions
All from the maintainer, 2026-09-29.
#4369: when should JS page-load
Livefire? (superseded by the toggle below)One-shot failure: what does a stream see when a one-shot
connect/acceptfails?Where should Offline live?
On Offline, broadcasts already announced?
Rust mirrors the toggle?
Initial offline event?
JS queue?
Open
rs/moq-net/src/{lite,ietf}/publisher.rs). Holding retractions there keeps advertising a dead route to peers during the gap. The quest recommends egress forwards retractions at once and only app-facing cursors hold.Impact
@moq/net's announceEvent,rs/moq-net'sAnnounceEvent, and the moq-ffi bindings, so it lands ondev.offlineis local connection state; the lite draft carries only the initial-set count (Active Count), no live marker.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code