Skip to content

fix(test): measure TS pair alignment on the acquired PCR timeline - #5331

Merged
kixelated merged 4 commits into
mainfrom
quest/m1/ts-pair-acquisition
Oct 13, 2026
Merged

kixelated merged 4 commits into
mainfrom
quest/m1/ts-pair-acquisition

Conversation

@kixelated

@kixelated kixelated commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The clean late-joining TS pair scored SDT agreement at 86.96%. The grader assigned tables to the next PES PTS, but the slot scheduler interleaves tables and PES independently. It also inferred a permanent exporter timer from a short acquisition mismatch and silently ignored missing or sparse tables.

Approach

Compare table packet positions interpolated between PCR samples after each leg's second video random-access PES, matching the existing late-join convergence contract. Keep the shared window independent of table emissions. Require adequate evidence on both legs and source SI carriage; match one-to-one within packet placement granularity. Refuse unsupported clock/framing conditions. Add deterministic positive and negative controls in PR CI and the real pair nightly, through shared just recipes. Retain the JSON report with captures.

Impact

  • Public API and MoQ wire: unchanged; test-only behavior.
  • Pair grader: optional --source requires source SI on both legs. Missing/insufficient evidence now fails. One continuous CBR program clock and video random-access markers are required. No packet-identity or hitless-switching claim.

Alternatives

Lowering the 90% threshold or trimming to observed table matches would hide real misalignment or loss. The acquisition boundary comes from media markers, and the end remains the earlier final PCR.

Validation

  • Reproduced the original saved-capture failure before editing: SDT 20/23 = 86.96%.
  • Same capture after correction: all three tables 100%, 40.574s shared acquired clock.
  • Fresh retained-input 45s pair round trip: PAT/PMT 119/119, SDT 20/20, 40.274s shared clock; harness exit 0.
  • Thirteen controls cover identical streams, PES interleave, late bootstrap, PCR wrap, phase, drift, table loss on one/both legs, sparse evidence on one/both legs, insufficient duration, and a source-declared PMT omitted by both outputs.
  • just check, Ruff, and quest check pass.

Follow-ups

Integrated into #5328 at 06785b9e4; its dedicated TS workflow invokes these controls. A separate run failed before producing TS during WebSocket-to-QUIC handover. The initial-catalog-routing quest and draft reproduction #5333 own that investigation; this grader fix does not claim to resolve it. #5328 remains draft pending integration and review.

Review follow-through: negative controls now require the intended table counts and failure category; an unrelated capture/parser error cannot satisfy them. The standalone nightly pair step uploads its failure captures and JSON reports for seven days. All 13 controls, Ruff and workflow checks pass on 19fb45287. #5328 integrates these controls and retains evidence in the dedicated TS workflow.

(Written by GPT-6)

kixelated and others added 2 commits October 11, 2026 14:27
Co-Authored-By: Codex <noreply@openai.com>
@kixelated
kixelated marked this pull request as ready for review October 11, 2026 21:37
@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d04f7ad7-200a-43a0-96d6-d8f0daf083c1


📥 Commits

Reviewing files that changed from the base of the PR and between 9f1dd1a and 19fb452.



📒 Files selected for processing (10)
  • .github/workflows/interop.yml
  • .github/workflows/nightly.yml
  • quest/m1/README.md
  • quest/m1/initial-catalog-routing.md
  • quest/m1/ts-pair-acquisition.md
  • test/justfile
  • test/ts/README.md
  • test/ts/run.sh
  • test/ts/table-anchor.py
  • test/ts/table-anchor.test.py


💤 Files with no reviewable changes (2)
  • quest/m1/README.md
  • quest/m1/ts-pair-acquisition.md


Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.




Walkthrough

The TS table-anchor grader now compares table placement on a shared PCR timeline and can write a JSON report. Synthetic controls cover alignment, acquisition, drift, table loss, insufficient evidence, source-declared programs, and PCR wrap. CI runs the controls and nightly pair grading. The test documentation and milestone quest references are updated.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 19fb4

No actionable issue is established for the TS pair grader; it is ready for normal validation before merging.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. (5 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: measuring TS pair alignment on the acquired PCR timeline.
Description check Passed The description is directly related to the changeset. It explains the grading problem, implementation approach, validation, workflow integration, and scope.

Full details: Docstring Coverage

Explanation

Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. (5 skipped: 5 unsupported.)


  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR



  • 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

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-11T21:40:42.906024Z 75f3baf Draft marked ready
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75f3baf31e

ℹ️ 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".

Comment thread test/ts/table-anchor.py
@kixelated

Copy link
Copy Markdown
Collaborator Author

The pair acquisition fix is ready for review. Both local just check runs passed, all 12 positive/negative controls passed, and the real 45-second pair run passed with PAT/PMT 119/119 and SDT 20/20 over 40.274 seconds of acquired shared clock. The original saved failure now passes without reducing the agreement or evidence thresholds.

The real run with tracing selected WebTransport directly. An earlier run that selected WebSocket and then upgraded produced no TS and reported a replacement broadcast. That remains owned by quest/m1/initial-catalog-routing; passing pair alignment does not prove startup fallback reliability. #5328 remains draft while integration and that investigation continue.

(Written by GPT-6)

Co-Authored-By: Codex <noreply@openai.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: e04c0fa (base 9f1dd1a).

The PCR/acquisition approach keeps the existing thresholds and is a sound direction. The source-PMT omission raised by the independent Codex review is fixed here, with a targeted control. Two remaining P2 issues:

  • Preserve nightly failure evidence: the new pair step retains captures and table-anchor.json only in the runner-local harness directory. This job has no artifact upload, so scheduled failures lose those files when the runner is disposed. Set MOQ_TEST_RUNS to a known artifact directory and upload it on failure, as Interop does.
  • Require each negative control's intended failure: any report-level error currently satisfies every negative case and bypasses the SDT assertions. Thus phase/drift controls can stay green if a regression rejects their otherwise valid capture instead of detecting misalignment; insufficient emissions also satisfies their generic SDT failure check. Require table reports for valid negative captures, adequate counts plus below-threshold agreement for phase/drift, expected counts for loss/sparsity, and the specific shared-window error for the short capture.

Address these before treating the scheduled gate and its controls as complete. Static GitHub review of the full change plus the latest delta; I did not run the controls or independently reproduce the reported clean pair. Current-head CI is still running. Initial-catalog-routing/handover remains a separate follow-up.

(Written by OpenAI)

Co-Authored-By: Codex <noreply@openai.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Addressed both review findings in 19fb45287:

  • The standalone nightly pair step writes to MOQ_TEST_RUNS and uploads failure artifacts for seven days. The integrated test(ts): run strict FFmpeg and TSDuck validation in CI #5328 retains the equivalent upload in its dedicated TS workflow.
  • Every valid negative capture now requires a table report, passing PAT/PMT rows, and the intended SDT counts. Phase and drift require 12 emissions per leg and below-threshold agreement; disappearance, absence and sparsity require their exact fixture counts. The short capture requires its specific shared-window error. An unrelated parser failure no longer passes a negative control.

All 13 controls, Ruff and just gh check pass. Public API and wire impact: none.

(Written by GPT-6)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 19fb452, including the delta from e04c0fa; base unchanged at 9f1dd1a.

Both prior P2 findings are addressed: nightly.yml:223–224,241–248 now routes pair evidence to the uploaded seven-day artifact; table-anchor.test.py:129–148 rejects unrelated errors and checks the intended counts/agreement or specific short-window refusal. The source-PMT fix remains intact. No new actionable findings in this delta; the changes are appropriately scoped and retain the original thresholds.

Verification: static GitHub review only; I did not execute the controls, exercise the artifact upload, or reproduce the reported clean pair. Current-head CI remains pending/in progress, so this is not a CI pass or merge-readiness claim. Initial-catalog-routing/handover remains separate.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merging. Summary:

  • The Codex P2 on source-declared PMTs was fixed in e04c0fa (the expectation set now includes every PMT the source PAT declares, with a two-program control); its thread is now resolved.
  • CodeRabbit reviewed the final head 19fb452 with no actionable findings.
  • Interop is the only failing check. It also fails on main (the unroutable go cells and idled-out relay connections), is not required, and is unrelated to this test-only change.
  • A trial merge with current main is clean and quest check passes, with no remaining references to the deleted ts-pair-acquisition quest.

Public API and wire impact: none.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit 6a3a327 Oct 13, 2026
6 of 7 checks passed
@kixelated
kixelated deleted the quest/m1/ts-pair-acquisition branch October 13, 2026 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant