chore(quest): audit the quest tree - #4393
Conversation
# Conflicts: # quest/m1/quic/README.md # quest/m1/quic/bbr-app-limited.md # quest/m1/quic/bbr-release.md # quest/m3/libmoq-cmake-lib.md # quest/m3/libmoq-fetch.md # quest/m3/libmoq-hidden.md # quest/m3/libmoq-shutdown.md
…ties 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. |
Recommendation: MERGEPositive improvement. This is quest-tree hygiene only (193 markdown files under Worth the complexity. A one-shot audit that makes the tree agree with code and itself is cheaper than leaving ~100 stale Required/Closes/symbol refs and done quests in place. New quest docs are concrete (symbols, land-on-dev, tests); README reorder puts blockers above dependents. Author reports Different approach? Not really for this class of change—piecemeal deletions would leave cross-links broken longer. Follow-ups called out in the PR (merge main into line branches, retarget qos/wildcard, delete leftover dead branches) correctly stay out of this PR. Safe to merge once required checks finish; nothing here blocks runtime. 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: 01b8374f9b
ℹ️ 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".
| - An encoder restart (a PTS rewind or a signalled time-base discontinuity) is | ||
| a new epoch, not a forward re-anchor: the importer ends the broadcast with | ||
| an error, and the caller republishes, which the broadcast epoch line turns | ||
| into a fresh `@<uuidv7>`. Until that line lands, a restart fails loud. |
There was a problem hiding this comment.
Move Broadcast epochs from Related to Required, or remove the epoch behavior from this quest's goal. The plan says the separate broadcast-epoch line is what turns a restart into a fresh @<uuidv7>, but related quests do not block execution, so this quest can currently be completed with every encoder restart merely failing. That does not deliver the stated outcome that restarts start a new epoch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed the goal overclaimed. Kept broadcast epochs as Related rather than Required so this quest isn't blocked behind that whole line, and reworded the goal: a restart ends the broadcast with an error here, and the epoch line owns turning the republish into a new epoch. Fixed in 16fcfb6.
(Written by Claude Opus 5.5)
| (`rs/moq-net/benches/origin.rs`). A suffix cannot walk that trie, so a naive | ||
| match costs the whole announce table on every announcement and every new | ||
| cursor. Benchmark first: extend `rs/moq-net/benches/origin.rs` with suffix | ||
| cursors swept over publishers and subscribers. The slope decides between a | ||
| reversed-segment index and abandoning the quest. |
There was a problem hiding this comment.
Extend the plan beyond matching announcements to suffix cursors. When the only claim is **/transcode.pro and a client subscribes or fetches foo/transcode.pro, the current request_broadcast/best_server path still searches the prefix trie and cannot select that publisher; the proposed benchmark and reversed index only address announcement fan-out. The quest needs to scope route insertion and SUBSCRIBE/FETCH resolution for suffix advertisements, with the routing lookup benchmarked across both table axes.
AGENTS.md reference: AGENTS.md:L35-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The plan now covers inserting suffix advertisements where request_broadcast / best_route resolve SUBSCRIBE and FETCH, with the same specificity rules, and benchmarks suffix route lookups as well as announce cursors, both swept over publishers and subscribers. Fixed in 16fcfb6.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging after maintainer confirmation. The PR lands the audit's cleanup and the decisions from the /quest-plan interview listed in the description. Review: Codex's two P2s are addressed in 16fcfb6. remove-live now scopes the epoch outcome to the broadcast-epoch line, and suffix-announce covers SUBSCRIBE/FETCH route resolution and its benchmark. Once this lands, follow-up agents will merge main into each line branch, move the wildcard line to m0, and retarget the qos line to dev. (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: 16fcfb6a40
ℹ️ 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".
| as `route_order` in `rs/moq-net/src/model/origin.rs` (cost, chain length, a | ||
| deterministic tiebreak), including `Cost::UNKNOWN` for an announce that |
There was a problem hiding this comment.
Do not delete this quest as completed while equal routes still resolve differently across implementations. For two routes with equal costs and hop counts, preferredEntry in js/net/src/origin.ts retains the newest entry, while Rust's route_order uses a deterministic prefix-and-chain hash before recency. That leaves the deterministic parity required here unfinished and can make Rust and JS origins select different providers; retain this work or explicitly fold it into the cluster-routing quest.
Useful? React with 👍 / 👎.
| The wire field is version-gated like `hidden`. Update `js/net` and | ||
| `drafts/draft-lcurley-moq-lite.md` in the same PR. Token patterns |
There was a problem hiding this comment.
Add doc/concept/moq-lite.md to the planned updates. It currently states that routes are always prefixes on every wire version, so implementing this plan as written would leave the user-facing protocol documentation directly contradicting the new suffix wire behavior. Repository sync rules also require doc/concept updates alongside rs/moq-net wire/API changes.
AGENTS.md reference: AGENTS.md:L99-L103
Useful? React with 👍 / 👎.
The release shipped (#4393 removed its quest), so the systemd unit fix stands alone in m0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Summary
A full
/quest-auditof 483 quests on main and 38origin/quest/*branches, followed by/quest-plandecisions from the maintainer. The tree now agrees with the code, with dev, and with itself.quest check: 446 documents ok.Deleted: already done
compareRoutes), m0/wildcard/{resolve,demand} (done on the line), lite-count-settle (folded into track-tail-interop).Maintainer decisions
suffix-announce, covers it as a moq-lite-only feature. Processors adopt wildcard's service-prefix layout, and advertise-auth becomes prefix-only.127-p.remove-live: importers droplive()and publish stream timestamps verbatim. It replaces gateway-live-clock.flate-binary: moq-binary folds into moq-flate.frame-slot-charge: re-planned from the closed chore(quest): add frame slot charge #3546.audio-quality-native: moved out of the m0 harness line.Plan fixes
Around 100 quests had stale symbol names, line refs, Required links, and Closes entries for issues that are already closed. The full evidence is in the audit report, grouped by quest.
Not in this PR (follow-ups)
## Requiredrename, soquest readytreats every line as ready).quest/m0/wildcard/READMEand retarget quest(wildcard): Wildcard advertisements #4037. quest(wildcard): block the line on the #4279 datagram, origin, and benchmark findings #4386 and docs(quest): drop suffix-based routing from the plans #4382 must follow the m0 path.quest/m0/frame-slot-chargeandquest/m0/3477-watch-auto-latencyonce this PR and the jitter line land. Fourteen other dead branches were already deleted.flush_onecomment inrs/moq-uring/src/quic/noq/connection.rs("Ignores noq's pacing hint"). The ring does pace throughpoll_timeout.Public API / wire
None. This PR changes quest files only.
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)