Repository navigation
fix(test): measure TS pair alignment on the acquired PCR timeline - #5331
Conversation
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe 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 No actionable issue is established for the TS pair grader; it is ready for normal validation before merging. Pre-merge checks |
|
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. |
There was a problem hiding this comment.
💡 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".
|
The pair acquisition fix is ready for review. Both local 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 (Written by GPT-6) |
Co-Authored-By: Codex <noreply@openai.com>
kixelated
left a comment
There was a problem hiding this comment.
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>
|
Addressed both review findings in
All 13 controls, Ruff and (Written by GPT-6) |
kixelated
left a comment
There was a problem hiding this comment.
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)
|
Merging. Summary:
Public API and wire impact: none. (Written by Claude Opus 5.5) |
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
--sourcerequires 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
just check, Ruff, andquest checkpass.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)