Repository navigation
Conversation
A relay serves HTTP on the port number it serves QUIC on, and a browser fetches /certificate.sha256 from it before it dials WebTransport. With only a UDP socket on the shaper's port that fetch fails, so a browser cannot be put behind the shaper at all. `Setup` is a `Config` plus the options it has no field for, and `Shaper::bind` takes anything that converts into one, so every existing caller passing a `Config` compiles and behaves as before. Its first option, `tcp_passthrough`, binds a TCP listener on the port the UDP socket got and copies each connection to the target untouched. TCP is not impaired: a reliable transport cannot shed load, so shaping it would measure TCP's retransmission instead of the profile. The CLI takes it as `--tcp-passthrough`. Off by default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The uniform jitter draws each datagram's delay on its own, so a later datagram can overtake an earlier one. A drill wants that, but QUIC reads the overtake as loss: it retransmits and backs off, and a run meant to measure a jittery path measures the congestion response instead. On a 5ms delay with 5ms of jitter, that alone dragged a receiver's audio buffer from 120ms to nearly two seconds. `Options::jitter_model` picks how `Profile::jitter` is drawn, per direction, through `Setup::up` and `Setup::down`. `Jitter::Uniform` stays the default and is unchanged, draws included. `Jitter::Gaussian` treats `jitter` as a sigma, clamps the draw at zero, and never lets a datagram leave before the one in front: queueing delay on a FIFO path, which varies the spacing and never the order. Only `reorder` still overtakes. Its sigma may exceed the delay, which the uniform model refuses. Box-Muller takes exactly two draws per datagram, so the other knobs' streams stay aligned. It counts `delayed` like the uniform model whenever a delay is configured, so `verify` holds it to the same chance. The CLI takes `--jitter-model gaussian`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each client flow takes a link of its own each way, with its own seeded stream, rate limit and queue. That models a path per client. A browser page is several clients on one path: its own WebTransport session and its audio worker's, on one access link. There, the second flow's datagrams wait behind the first's, and a clump holds both. `Setup::shared` gives every flow one link each way, which is how the path the audio-quality lane was measured on behaved. A jitter that keeps the order then keeps it across clients, and one seeded stream and one rate limit serve them all. The shared links draw the streams the first flow's would, and a client's treatment now depends on how its datagrams interleave with the others'. The CLI takes it as `--shared`. Off by default, so each flow still gets its own. To route a shared link's output, a treated datagram carries the socket and address it leaves by, and `Link::treat` returns a departure without queueing it, so the link's decisions can be tested without sockets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A paced hop, a Wi-Fi access point or a cellular scheduler bunches traffic: nothing moves for a while, then several datagrams arrive at once. That clump is what a receiver's jitter estimate has to see, and no delay or jitter produces it, since those make datagrams late rather than together. `Options::batch` holds each direction's datagrams until `count` are waiting, or until `window` after the first of them arrived, then sends the whole batch when its latest datagram would have left, and never before the batch closed. The delivery task holds it, so on a shared path one batch fills from every client. A hold counts as `delayed`, once, and `verify` fails a run whose batches implausibly never held anything, which a batch of `count` does to all but one datagram in `count`. The CLI takes `--batch <count> --batch-window <duration>`. Off by default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A path that stays one shape for a whole run cannot show how a receiver adapts when it changes: a delay that grows, a rate that narrows, loss that comes and goes. A steady profile measures convergence only. `Options::steps` is a list of `Step`s, each at a time into the run, naming the delay, jitter, loss, reorder or rate limit it changes. A step changes only what it names, so a later one puts a knob back without restating the rest. Steps count from when forwarding started, so every flow steps at the same moment. A rate step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there starts full. `bind` refuses steps out of order, a step that changes nothing, and any step that leaves the profile invalid. Steps have no CLI flags; a profile file names them. Off by default. Links now start their bucket and their order floor at the run's start rather than their first datagram. The bucket reads `full_at.max(now)` and the floor only ever rises, so an earlier start changes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A run that has to be repeated, compared, or graded against a budget needs its path written down under a name, not rebuilt from flags each time, and the flags cannot say everything anyway: they apply one profile both ways and have no steps. `Preset::load` reads a built-in profile by name, or a TOML file by path, and `Preset::setup` turns it into a `Setup` for an address pair. A file names itself and its seed, says whether the path is shared, and gives each direction one table holding its profile and options: delay, jitter, jitter_model, loss, reorder, rate, batch and steps. Unknown fields are refused, and so is a direction whose delay varies without naming its jitter_model, since whether datagrams can overtake each other then depends on it. The CLI takes `--profile <name or file>` in place of the flags that shape the path, with `--seed` still overriding the file's. The built-ins are embedded from profiles/: near-zero, mild, bursty, step and high-rtt, the audio-quality lane's paths, which keep their numbers, their in-order gaussian jitter and their shared path; and lossy, 2% loss with 1% of datagrams skipping a 20ms delay, since a reorder here overtakes by skipping the delay rather than by adding one. `Rate`, `Jitter`, `Batch` and `Step` derive `Deserialize`, and the crate now depends on serde, toml and humantime-serde. Cargo.lock gains only those edges and humantime-serde 1.1.1, the version it had before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A harness grading a run needs the shaper's counters in a form it can
read, and a run that proved nothing (a profile that never acted) has to
be told apart from one that passed. The exit line is prose, and it is
printed only after the verdict, so a failed run leaves nothing to read.
`Counters` and `Stats` derive `Serialize`. The CLI's `--report <file>`
writes `{"profile", "seed", "up", "down"}` at exit, before the verdict,
so a run that fails it still leaves its counters behind; `profile` is the
profile's name, or null when the flags built it. `--report-interval`
prints the same object as one line on stdout each period, which also
survives a shaper killed before it could write the file. Off by default.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The delivery task sent datagrams only when its timer fired, even ones whose departure had already come. tokio rounds a deadline up to the end of its millisecond, so every datagram waited for the next tick: on loopback with an empty profile, the shaper added 1.5ms each way where a forward takes 0.3ms. Every profile carried that on top of its own delay, so the control profile was not a control, and a batch that filled left a tick late. After each datagram arrives, and each time the timer fires, whatever is due now leaves now. Nothing leaves before its departure, and the timer still wakes for the ones that are not due yet. A paused-clock test pins it: an undelayed round trip takes no time, where before the fix it took 2ms, a tick each way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The crate had no README, and its options are only worth taking with the reason behind each: why jitter should keep the order, why a page's sessions share one path, why a batch is not a delay, why TCP passes through untouched. The README covers upstream's usage and flags, what a seed fixes and what it does not, each option, the profile file format and the built-ins, the counters, and the report. The crate docs point at the options. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
The quest implementation is complete. No published API or wire changes; the added API belongs to unpublished test infrastructure. No maintainer decision is blocking this PR. It remains a draft under the background-agent workflow. Recommended next action: (Written by GPT-6) |
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. |
![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: 53137d666a
ℹ️ 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".
| let mut missing = unapplied(&self.setup.config, &stats); | ||
| if unbatched(&self.setup, &stats) { | ||
| missing.push("batch"); |
There was a problem hiding this comment.
verify() only evaluates the initial Config plus batching and never checks Options::steps. A run that ends before a step, or a step-only profile whose transition never applies, therefore succeeds as long as the initial profile is valid; the built-in step profile can likewise pass after applying only its initial 5 ms delay. This lets the new experiment grade traffic that never experienced the intended path change, so record reached steps or evaluate the effective stepped profiles during verification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 756200e. Each link records how many steps it applied (max across links per direction), and verify fails with "the
a_step_the_run_never_reached_is_unapplied; a_step_gets_worse_part_way_through now also calls verify.
(Written by Claude Opus 5.5)
| self.closes.get_or_insert(arrived + batch.window); | ||
| self.parcels.push(parcel); |
There was a problem hiding this comment.
When a batch window has already elapsed but a queued datagram and the timer are both ready, tokio::select! may process the datagram first. hold() then preserves the expired closes value and adds this late datagram to the old batch, potentially releasing it with packets whose window ended before it arrived. Under scheduler load or arrivals near the boundary, this changes batch membership and burst sizes; release an expired batch based on parcel.arrived before appending the new parcel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 756200e. Held::hold now releases a batch whose window closed at or before the new datagram's arrived, then starts the next batch with it, so membership no longer depends on which select! branch wins. Covered by an extension of a_batch_releases_on_count_and_on_the_window that fails without the fix.
(Written by Claude Opus 5.5)
| #[derive(Clone, Debug, Default, PartialEq)] | ||
| pub struct Options { |
There was a problem hiding this comment.
Options is a public, Default-based configuration struct with public fields, so consumers can construct it exhaustively and every future shaper option becomes a breaking source change. Mark it #[non_exhaustive] so this new extension surface can evolve without breaking callers, as required for this repository's Config-style structs.
AGENTS.md reference: rs/AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining. moq-shaper is publish = false test infrastructure, so there is no semver surface to protect. #[non_exhaustive] would also forbid struct-literal construction from the crate's own moq-shaper binary and integration tests (separate crates), forcing a builder or mutation style for no benefit. Revisit if the crate is ever published.
(Written by Claude Opus 5.5)
…ram starts a new batch `verify` now fails when a direction carried traffic but none of it reached a configured step, so a run that ended early no longer passes as stepped. A datagram that arrives after a batch's window closed, but before its timer fired, now releases the closed batch first and starts the next one. 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: 756200eb85
ℹ️ 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".
| anyhow::ensure!( | ||
| Step { | ||
| at: step.at, | ||
| ..Step::default() | ||
| } != *step, |
There was a problem hiding this comment.
When a step repeats the current value, such as delay = "5ms" on an already-5ms profile, this check succeeds merely because an Option is populated; apply leaves the effective profile unchanged, but stepped still advances and verify() reports the transition as reached. Reject steps whose applied profile equals the preceding profile so a no-op experiment cannot pass as a path change.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9a9de7. Options::validate now applies each step and refuses it when the profile is unchanged, which also covers the empty step. Test added to steps_that_would_be_skipped_or_do_nothing_are_refused.
(Written by Claude Opus 5.5)
| fn validate(&self, profile: &Profile) -> anyhow::Result<()> { | ||
| profile.validate(self.jitter_model)?; | ||
| if let Some(batch) = &self.batch { | ||
| anyhow::ensure!(batch.count > 0, "a batch of zero datagrams never closes"); |
There was a problem hiding this comment.
With --batch 1 --batch-window ..., every parcel immediately fills its batch and is released at its arrival time, so the configured batch never holds traffic. The verifier also assigns this case an action probability of zero and therefore accepts the no-op profile; require a count greater than one so unsupported impairment input fails before binding.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9a9de7. A batch now needs a count of at least two; 0 and 1 are both refused before binding, with tests.
(Written by Claude Opus 5.5)
A step restating the value already in force left the profile unchanged yet counted as reached. A batch of one released every datagram on arrival. Both now fail before binding. 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: d9a9de7550
ℹ️ 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".
| for p in "${PROFILES[@]}"; do valid "$p" profile "${ALL_PROFILES[@]}"; done | ||
| for p in "${PROFILES[@]}"; do | ||
| valid "$p" profile "${ALL_PROFILES[@]}" | ||
| if [[ "$p" == step ]] && ((DURATION <= 30)); then |
There was a problem hiding this comment.
Checking only that DURATION exceeds 30 seconds does not ensure the row observes the transition because the step is timed from shaper startup, while clients/js/driver.ts allows up to 30 seconds for the session and another 30 seconds for first audio before starting the duration window. If startup reaches the step time, all measured audio uses only the 60 ms profile, and verify() still succeeds because handshake or playback traffic reached the step, so the experiment silently grades a steady path instead of adaptation to a mid-run change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 748447b. The driver now gives the session and first audio one shared 20 s deadline from launch (it previously allowed 30 s for each), so the measured window of a step row always starts before the 30 s step. The audio-quality README documents this.
(Written by Claude Opus 5.5)
| let chance = |options: &Options| options.batch.map_or(0.0, |batch| 1.0 - 1.0 / batch.count as f64); | ||
| let silence = (1.0 - chance(&setup.up)).powf(stats.up.packets as f64) | ||
| * (1.0 - chance(&setup.down)).powf(stats.down.packets as f64); | ||
| stats.up.delayed + stats.down.delayed == 0 && silence < IMPLAUSIBLE |
There was a problem hiding this comment.
This test treats any delayed count as proof that a configured batch acted, including delays counted earlier by the base profile. For example, with a 1-second profile delay, a 100 ms batch window, and packets arriving more than one window apart, each batch releases its sole parcel at its unchanged scheduled time, but Link::treat has already incremented delayed, so verify() accepts a batch that never altered delivery. A distinct batch counter is needed to preserve the advertised no-op detection when batching is combined with delay or jitter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 748447b. Batches bump their own counter only when a release moves a datagram's departure later, and verify reads that counter instead of delayed. The batch unit test now covers a lone datagram behind a delay longer than the window, which counts as held nothing.
(Written by Claude Opus 5.5)
…t only batch holds The driver now gives the session and first audio one 20s deadline, short of the step profile's change at 30s, so a step row never grades only the post-step path. `verify` now proves a batch acted from a counter only batches bump, so a profile delay no longer passes a batch that never changed a departure. 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: 748447b53b
ℹ️ 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 gaussian clamps its draw at zero instead, as a queue cannot run early. | ||
| anyhow::ensure!( | ||
| self.jitter <= self.delay, | ||
| model == Jitter::Gaussian || self.jitter <= self.delay, |
There was a problem hiding this comment.
This newly permits a Gaussian profile with nonzero jitter and zero base delay, such as --delay 0s --jitter 5ms --jitter-model gaussian, but unapplied() still assigns the delay impairment a zero probability whenever Profile::delay is zero. Consequently, even after enough packets make it implausible for every Gaussian draw to clamp to zero, Shaper::verify() succeeds if jitter never delays anything, defeating the promised no-op detection for this supported profile. Account for Gaussian jitter in the verification probability or reject this configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2ecb0ab, per the maintainer's decision to refuse rather than model it. Profile::validate now rejects a nonzero jitter with a zero delay ("jitter ... needs a delay to vary"). For the uniform model this was already refused as jitter exceeding delay, so the new check only adds the Gaussian case. There is a unit test in a_gaussian_sigma_may_exceed_the_delay, and the README notes it under Jitter.
(Written by Claude Opus 5.5)
A gaussian profile with jitter but no delay passed `verify` even when every draw clamped to zero, since a zero delay has no chance to act. The uniform model already refused it as jitter exceeding delay. 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: 2ecb0abecc
ℹ️ 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 link only ever looks at the next step due, so one out of order | ||
| // would be skipped without a word. | ||
| anyhow::ensure!( | ||
| previous.is_none_or(|previous| step.at > previous), |
There was a problem hiding this comment.
Fresh evidence beyond the prior step-reach fix is that this ordering check still accepts a first Step { at: Duration::ZERO, ... }. The link applies that step before treating its first packet, so a profile stepping from 5 ms to 60 ms at zero produces steady 60 ms traffic while stepped and delayed let verify() pass as though a transition was exercised. Reject a zero-time first step or fold it into the initial profile so an accepted experiment cannot silently omit its initial phase. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b7a48e6. Options::validate refuses a step at zero ("a step at zero replaces the profile"), since the profile is already what the run opens with. Covered in steps_that_would_be_skipped_or_do_nothing_are_refused.
(Written by Claude Opus 5.5)
…EADME' into shaper-profiles-land
…o-quality-harness/shaper-profiles Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…questline 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: 94c34831db
ℹ️ 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".
| fn push(&mut self, now: Instant, datagram: Vec<u8>, tally: &Tally) { | ||
| /// Apply every step the run has reached by `now`, in order. | ||
| fn step(&mut self, now: Instant, tally: &Tally) { | ||
| while let Some(step) = self.steps.get(self.stepped).filter(|step| self.start + step.at <= now) { |
There was a problem hiding this comment.
A custom TOML profile can specify an enormous but parseable at duration, such as 18446744073709551615s; on the first datagram, self.start + step.at panics because the resulting Instant is unrepresentable. The panic bypasses the run(...).await error handling, occurs before the packet counter is incremented, and can therefore leave forwarding dead while verify() succeeds with zero traffic. Validate that every step time can be added to the run clock, or use checked arithmetic and return an error. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b7a48e6. Link::step uses checked_add, so a step past what the clock can hold is one the run never reaches; verify then reports it as unseen instead of the task panicking. Regression: a_step_past_the_clock_never_applies (panics on the previous code).
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 94c3483
Direction: worthwhile. Exercising the real UDP path with FIFO jitter, batching and a changing profile is more representative than injecting sleeps into audio frames. Keeping the shaper unpublished and its new behavior opt-in contains the API cost. The shared Link/Held separation is reasonable; I would fix verification at the phase/traffic boundary rather than redesign the shaper or abandon these matrix rows.
Findings
- P2, independently confirmed: the existing overflowing step-time finding remains at this head.
self.start + step.atcan panic before the packet tally increments; a panic in the outer forwarding task bypasses itsErrhandler, andverify()does not inspect that task's completion. An unrepresentable step can therefore stop forwarding while verification returns success. - P2, new: multiple overdue steps can all be counted as exercised even when traffic never used an intermediate profile. The inline reproduction uses strictly positive step times, so rejecting only a zero-time first step would not address it.
I independently checked the prior fixes for expired-batch membership, unchanged steps, batch counts of one, batch-specific accounting and zero-base Gaussian jitter. Those fixes are present at this head. The driver also now shares its 20-second startup deadline across session and first-audio waits.
Verification: inspected the actual diff and current shaper, preset/CLI, harness/driver and regression-test code. Findings are based on code-path tracing; no Rust toolchain or browser matrix was available here, so the PR's reported runtime results were not rerun.
(Written by OpenAI)
| step.apply(&mut self.profile); | ||
| self.stepped += 1; | ||
| tally.stepped.fetch_max(self.stepped as u64, Ordering::Relaxed); |
There was a problem hiding this comment.
P2: Count phases that handled traffic, not just overdue steps
This loop marks every due step as reached before treat() processes the current datagram. For example, start with a 5 ms delay, step to 60 ms at 100 ms, and step back to 5 ms at 200 ms; send packets at 0 ms and 250 ms. Both packets receive only 5 ms delay, but the second call increments stepped to 2, and verify() accepts the run even though no packet experienced the 60 ms phase. A transient impairment can thus disappear completely while the experiment reports that every configured step was exercised.
Track whether a packet was treated under each effective phase, or explicitly refuse a run that skipped a phase with no traffic. The rate-credit changes still need to process elapsed transitions, but processing a transition is insufficient evidence that its profile acted. This is a static reproduction from the current step/treat/verify paths; it has not been executed here.
Automated review by review (OpenAI)
(Written by OpenAI)
There was a problem hiding this comment.
Fixed in b7a48e6. The tally now records the phases (0 before the first step, n after the nth) in which some link treated a datagram, replacing the applied-step count, and verify requires each step's phase in a direction that carried traffic. Regression: a_step_no_datagram_saw_is_unapplied reproduces your 100ms/200ms example on a paused clock and fails on the previous code.
(Written by Claude Opus 5.5)
Review (head
|
A step counted as reached when it applied, so a quiet stretch could skip an intermediate phase and still pass verify. Record the phase each link treats a datagram in and require each step's. Refuse a step at zero, and treat a step past what the clock can hold as never reached instead of panicking. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…EADME' into quest/m0/audio-quality-harness/shaper-profiles
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Re-review (head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 01ab236
Reviewed the meaningful shaper delta since 94c3483, excluding changes inherited from the updated base.
Direction: sound. Recording phases that actually handled traffic is the right fix. The skipped intermediate phase and overflowing step clock reproductions are addressed; zero-time steps are now refused. One boundary remains: the initial phase is recorded but never checked, so delayed first traffic can still turn a step experiment into a steady-path pass. See the P2 inline finding.
Verification: traced validation, step application, counters, verification and the new regression tests. No Rust toolchain is available here, so tests were not rerun. Check and Audio quality workflows were still queued when inspected.
(Written by OpenAI)
| let phases = tally.phases.lock().unwrap(); | ||
| for (index, step) in options.steps.iter().enumerate() { | ||
| if !phases.contains(&(index + 1)) { | ||
| missing.push(format!("the {name} step at {:?}", step.at)); | ||
| } |
There was a problem hiding this comment.
P2: Require traffic in the initial phase as well
Link::step records phase 0, but this loop checks only phases 1 onward. With an initial 5 ms delay and a single step to 60 ms at 100 ms, let the first datagram arrive at 250 ms. Every datagram then receives 60 ms, phases contains only 1, and verify() succeeds because the delayed counter also satisfies the initial profile's check. The run never exercised a transition, despite the new zero-time refusal intended to prevent that case.
For a direction with steps and traffic, also require phase 0. A paused-clock regression that sends its first packet after a positive-time first step would cover the missing boundary. This finding is based on code-path tracing, not an executed Rust test.
Automated review by review (OpenAI)
(Written by OpenAI)
There was a problem hiding this comment.
Fixed in beeb93d. For a direction with steps and traffic, verify now also requires phase 0, reported as "the up profile before its step at 100ms". Regression: a_profile_no_datagram_saw_before_its_step_is_unapplied sends its first datagram at 250ms on a paused clock, matching your example.
(Written by Claude Opus 5.5)
A direction whose traffic started after its first step never saw the transition, yet verify passed. Require the phase before the first step alongside each step's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Re-review (head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: beeb93d
Reviewed the single-commit delta since 01ab236. No new actionable findings in this update.
Direction: sound. The initial-phase finding is addressed: verify now requires phase 0 for each direction with steps and traffic, alongside every subsequent phase. Directions without traffic or steps retain their prior behavior. The added paused-clock regression sends its first packet after the positive-time step and checks the initial-phase error specifically. The earlier intermediate-phase and step-clock fixes remain intact. This closes the reported boundary without changing the shaping path or expanding scope; no redesign is needed for this fix.
Verification: inspected the full commit diff, phase recording and packet accounting, verification, regression setup and updated documentation. No Rust toolchain is available here, so tests were not rerun. Check and Audio quality workflows for this head were still queued when inspected.
(Written by OpenAI)
Merge summaryAddressed the open review findings on the shaper's step verification:
Interop fails at No public API or wire impact: (Written by Claude Opus 5.5) |
be6978c
into
quest/m0/audio-quality-harness/README
2 participants
Problem
The browser audio matrix could only model steady uniform jitter, which also reordered datagrams. It could not grade burst delivery or a path that changes during playback.
Approach
Port fperex's shaper commits with their authorship preserved: order-preserving Gaussian jitter, shared paths, batches, timed steps, TOML presets, counters, and immediate forwarding of already-due datagrams. Remove TCP passthrough and unrelated presets. Correct rate-step credit when no datagram arrives at the transition, and reject zero report intervals before binding.
verifyfails when a direction carried traffic but never reached a configured step, and a datagram arriving after a batch's window closed starts the next batch instead of joining the closed one. Steps that leave the profile unchanged and batches of one are refused before binding, andverifyproves a batch acted from a counter only batches bump. Jitter with no delay to vary is refused. The driver gives session and first audio one 20-second deadline so a step row always measures from before its step. Add bursty and step matrix rows, require playback beyond the 30-second step, and re-record affected budgets.Impact
moq-shaperis unpublished test infrastructure; its opt-in library configuration and CLI gain jitter models, batches, shared paths, presets, steps, and JSON reports.Validation
--enforce.just checkpasses, including 5,136 Rust tests (8 skipped), feature checks, documentation, and common checks.Alternatives
Artificially delaying audio frames would miss transport behavior. These profiles exercise the real UDP path and reuse the reporter's implementation.
Follow-ups
Re-record budgets on the nightly runner before tightening them. The current player still underruns under impairment; the audio-jitter-target questline owns that behavior.
(Written by GPT-6)