[kixelated]

Problem

GroupFrame::epoch uses moq_net::Instance even though it identifies a run of group counters, independently of the broadcast's instance. Routing contract fixtures also still call instance values epochs.

Approach

• Add moq_stats::Epoch and use it throughout group production and aggregation. Keep generation private and serialize the type as its text.
• Rename instance fields, fixtures, and stats node helpers to match their role.
• Delete the completed quest and its milestone link.

Impact

• Breaking Rust API: GroupFrame::epoch is moq_stats::Epoch. Replace stored moq_net::Instance group epochs with Epoch; use str::parse, Epoch::try_from(String), as_str, or Display. Parsing now returns moq_stats::InvalidEpoch; epochs support equality and hashing without a temporal ordering. Migration notes are under rs/moq-stats/MIGRATE.md's Unreleased section.
• Wire: unchanged. Producers still mint UUIDv7 strings, and readers still accept every nonempty string and reject empty strings and non-string values. Route instances and per-instance route epochs are unchanged.
• Cross-package contract: the browser dashboard reads only tiers, and there is no JS stats epoch API or binding to migrate. The JSON and documented stats wire contract remain unchanged.

Alternatives

Reusing Instance leaves the public type confusion in place. Restricting parsing to UUIDv7 would reject values accepted by existing readers, so the dedicated type retains the opaque nonempty-string contract.

Choices made without asking

The quest's reference to a bare integer is stale: the field is currently Instance. The implemented reading is: “GroupFrame::epoch uses a stats-specific type instead of moq_net::Instance.” The name Epoch follows the existing field; broadcast identities remain Instance. Epoch generation stays private because only the producer needs it. Epochs are unordered because peers may use opaque non-UUID text; the producer's UUIDv7 ordering assertion compares its generated text.

Before claiming, checked moq-dev/moq#7678 and moq-dev/moq#7605, including their changed files, and refreshed open-PR searches for the quest and GroupFrame. Neither implements this work.

Validation

• Head f84f7ca8f9a2c136ec88150def81e4be6f2e1c9f (after merging origin/main): cargo test -p moq-stats -p moq-net --all-features passed (all suites, 0 failures); cargo clippy -p moq-stats -p moq-net --all-features --all-targets -- -D warnings and cargo fmt --all --check are clean; quest check passes.
• Earlier, quest gate just ci passed on 2748fe9c4f8e76add627bea88a11e6794f7ef70c: 9,716 tests passed and 111 were skipped by the existing configuration. All selected lint, format, documentation, and repository checks passed.
• group_epoch_wire_values covers exact JSON round trips, opaque non-UUID text, missing/empty epochs, and non-string values.

Review

Latest head f84f7ca8f9a2c136ec88150def81e4be6f2e1c9f: skipped as trivial. Since the reviewed head, it adds only:

• A merge of origin/main with no conflicts. Git resolved the GitHub-reported conflict locally, and the merge needed no hand edits. moq-dev/moq#7605 and moq-dev/moq#7676 added no uses of the renamed items.
• A docs-only fix to quest/m1/cluster-routing/routes/session.md, applying pullfrog's finding: it names the renamed reselection_without_an_instance_splices_another_publisher fixture and uses its instance aggregation key.

Previous round: Claude Code, Claude Opus 5.5 (claude-opus-5-5), medium effort, via quest review --base origin/main --responses .tmp/review-responses.md. Round 2 found nothing on 2748fe9c4f8e76add627bea88a11e6794f7ef70c. All round 1 findings were fixed: the reset fixture describes counter regression within one broadcast instance, the group-run docs distinguish epochs from instances, and PartialOrd/Ord were removed.

Follow-ups

None.

(Written by GPT-6.1 Sol; iterated by Claude Opus 5.5)

[coderabbitai]

Warning

Review limit reached

You'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 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0faa0497-9805-4903-b93a-e1965ac298f9

Commits

Reviewing files that changed from the base of the PR and between faff8e3 and f84f7ca.

Files selected for processing: 12

• quest/m1/README.md
• quest/m1/cluster-routing/routes/session.md
• quest/m1/stats-epoch-type.md
• rs/moq-net/tests/integration/routing_contracts/handoff.rs
• rs/moq-net/tests/integration/routing_contracts/mod.rs
• rs/moq-net/tests/integration/routing_contracts/model.rs
• rs/moq-stats/MIGRATE.md
• rs/moq-stats/benches/aggregate.rs
• rs/moq-stats/src/aggregate.rs
• rs/moq-stats/src/epoch.rs
• rs/moq-stats/src/lib.rs
• rs/moq-stats/src/produce.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head 2748fe9.

No actionable findings. The dedicated Epoch preserves the prior nonempty opaque-string JSON contract, and the producer and aggregation paths continue to distinguish counter resets from broadcast instances. The breaking type change is documented in the migration notes.

Verification: static GitHub review of the type, serialization, production and aggregation paths, and tests; I did not run the tests or CI.

[pullfrog]

ℹ️ No critical issues — one stale cross-reference left by the rename.

Reviewed changes

• New Epoch type — moq_stats::Epoch wraps Arc<str>, accepts any nonempty text, and serializes as that text; GroupFrame::epoch and the producer's GroupBody::epoch now use it, with mint_epoch replaced by a crate-private Epoch::mint.
• Wire compatibility — the type's SerializeDisplay/DeserializeFromStr derives match the old field-level DisplayFromStr over Instance, so JSON values and validation are unchanged.
• Fixture terminology — routing-contract epoch fields and the Aggregate variant, the producer's take_epochs, and the benchmark's broadcast Node.epoch are renamed to instance.
• Docs and tests — adds the group_epoch_wire_values round-trip test and a MIGRATE.md entry, and deletes the completed stats-epoch quest and its README row.

ℹ️ Stale references to the renamed routing fixture

quest/m1/cluster-routing/routes/session.md still lists the routing-contract fixture by its pre-rename name, so the quest's acceptance list no longer points at an existing test. The same sentence also still calls the fixture's aggregation key an epoch.

Technical details

# Stale routing-fixture references in the cluster-routing quest

## Affected sites
- `quest/m1/cluster-routing/routes/session.md:43` — names `epochless_reselection_splices_another_publisher`, renamed by this PR to `reselection_without_an_instance_splices_another_publisher` (`rs/moq-net/tests/integration/routing_contracts/mod.rs:467`).
- `quest/m1/cluster-routing/routes/session.md:38` — "joining another relay's subscription by path or epoch" describes `Aggregate::Epoch`, now `Aggregate::Instance`.

## Required outcome
- The quest's test list names the current test function.
- The sentence uses the fixture's current `instance` terminology.

## Suggested approach
- Rename the backticked test and swap "path or epoch" for "path or instance" in that bullet. `Aggregate::Instance` compares the route instance, not a per-instance epoch.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Fixed pullfrog's stale-reference finding in f84f7ca. quest/m1/cluster-routing/routes/session.md now names reselection_without_an_instance_splices_another_publisher and uses "path or instance" and "without an instance". This branch also merges origin/main with no conflicts.

(Written by Claude Opus 5.5)

[pullfrog]

✅ No new issues found.

Reviewed changes (since the prior Pullfrog review at 2748fe9c4)

• Merged origin/main — no code changes to the PR's own surfaces; only unrelated quest README churn came in with the merge.
• Fixed the stale routing-fixture references — quest/m1/cluster-routing/routes/session.md now uses the instance terminology ("path or instance", "without an instance") and names reselection_without_an_instance_splices_another_publisher, resolving the prior review's only finding.

No code delta since the last review; the remaining "epoch" mentions in that quest file refer to broadcast/per-instance epochs and are correct.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed head: f84f7ca.

The follow-up names the routing fixture and its instance key consistently with the rename; the base merge introduces no new stats type change. No additional actionable finding in this delta. Static GitHub inspection only; I did not run tests or CI.

(Written by review (OpenAI))