Repository navigation
quest: plan CARLA control, DTLS timeout, HLS source, and CLI catalog follow-ups - #7751
Conversation
…a typed DTLS timeout Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 76981b7 against base 151ed38.
No actionable finding in the three quest plans. The CARLA plans describe the observed handoff and stuck-detector paths, retain the socket migration context, and leave the brake choice and diagnosis explicit. The DTLS plan preserves the distinction from ICE timeout and accounts for the eventual vendored-stack transition.
Static GitHub-only review; I did not reproduce the public-demo behavior or run CARLA or DTLS tests.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The stuck-truck quest's recommended fix cannot explain the 0 km/h observation it is based on, and the DTLS quest's typed handover to the vendoring quest is not recorded where the vendoring PR will see it.
Reviewed changes
- Neutral-pedal takeover (m1, XS):
takeoverStateandDrive.synchronizecurrently copy the autopilot's measured pedals, so the plan keeps steering and gear but starts the pedals neutral, clamps a claiming command on the worker too, and updates the sim/web tests and theworld.pydocstring. Paths, symbols, and test names check out against the code. - Stuck respawn after handback (m1, S): the
StuckDetectorresets on every tick with a driver, e-stop,not autonomous, or a parked world. The plan reproduces the observed wedge, lists two suspects, and recommends arming for driverless un-e-stopped trucks regardless of autonomy while keeping held and e-stopped trucks exempt. - Typed DTLS timeout (m2/webrtc, XS): adds a variant to
moq_ice::Errorandmoq_rtc::Errorfor the post-ICE handshake timeout currently flattened by str0m intoError::Rtc("rtc error: DTLS error: timeout: handshake"), withsession::endlogging it likeIceTimeout. The exact string, thedrainmapping, and the moq-tokiowebrtcmapping were verified. - Index and note updates: the m1 and WebRTC READMEs list the new quests, and
dtls-after-ice.mdallows the typed variant in its post-ICE failure assertion.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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. View limit details
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. |
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 22abc2d against base 151ed38.
P2 — Account for predecessor sources when exposing cache identity (quest/m2/hls-rendition-source.md:5–8,17–24). A rendition's current binding does not necessarily identify every segment in its playlist: rs/moq-hls/src/export/rendition.rs:593–625 retains rows from predecessor renditions, and :1323–1330 dispatches an inherited segment to the predecessor, whose own binding authorizes and supplies its bytes. After a replacement, a playlist can therefore list old and new source instances together. Returning only the current source on a playlist or on every segment response can mis-key retained bytes and make a cache or authorization layer treat the predecessor's content as the replacement's. Specify identity for the actual binding supplying each resource (and how a mixed-source playlist is represented), preserve per-viewer authorization, and test a replacement while predecessor rows still drain.
Static GitHub-only review of the new plan and current HLS binding/segment paths. I did not run tests or exercise a cache.
There was a problem hiding this comment.
ℹ️ No critical issues — one plan-accuracy point inline.
Reviewed changes
- Added the
moq-hlsrendition-source quest and itsm2README entry: the export would report each rendition's bound source so a downstream HLS cache can share entries across viewer grants.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 4c024d4 against base b5f4bca.
The new CLI catalog-update quest is scoped to an external update mechanism, preserves the one-catalog default, and covers a subscriber and a late joiner. I found no new actionable issue in that addition.
The earlier HLS mixed-source playlist finding remains open: the HLS quest is unchanged and still calls for the current rendition binding without specifying the predecessor bindings supplying retained segment rows.
Static GitHub-only review of the new plan and the prior finding; I did not run the CLI or HLS tests.
There was a problem hiding this comment.
ℹ️ No critical issues — one plan-accuracy point inline.
Reviewed changes
- Added
quest/m2/cli-catalog-updates.mdand itsm2README entry: a plan for the CLI to publish catalog updates mid-stream so catalog recovery replays need no hand-built clients.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd CLI catalog plans The stuck quest diagnoses before fixing: a stalled handback cannot hold the detector off at 0 km/h, so the latched e-stop, a parked world, and a rocking truck are the suspects. The vendoring quest now owns the typed DTLS timeout if it lands second. The HLS quest keys on path and instance and reports each resource's actual source, predecessors included. The CLI quest names the current `moq import` verb. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the delta since the prior Pullfrog review (4c024d4), namely 6169567, which implements the earlier plan-accuracy findings.
- Rewrote the CARLA stuck-handback plan to rule out a stalled handback and an abandoned lease from the code, put the latched e-stop first, and leave the driverless-latched respawn policy to the maintainer with a recommendation.
- Recorded the DTLS/vendoring handover in
vendor.md, and madedtls-timeout.mdcarry dimpl's typed timeout when vendoring lands first instead of adding a throwaway text match. - Switched the HLS source identity to
path+instance(with the hidden route epoch explained) and specified per-resource supplier reporting, including a predecessor's retained rows, so a mixed-source playlist is covered. - Fixed the CLI catalog quest to name the current
moq importverb.
The rewritten code claims (handback release and WALKING_PACE, the 1 s command watchdog, STUCK_DISTANCE re-anchoring, and Rendition::after rows carrying their instance) check out against the source.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 6169567.
The prior HLS mixed-source finding is addressed: segments report the source actually supplying their bytes, playlists represent every listed source, and a replacement test retains the predecessor rows. The updated plans preserve per-viewer authorization and put the DTLS text-match replacement with the vendoring work. No additional actionable finding in this delta. Static GitHub inspection only; I did not run HLS, CARLA, or DTLS tests.
(Written by review (OpenAI))
Plans five quests from a 2026-11-03 planning session with the maintainer.
Rtc(String), since str0m flattens dimpl's typedTimeoutError::Handshake. Add a typed variant that session end logs like an ICE timeout.moq importpublishes one catalog per broadcast.Ranking and placement (my choices):
demo/carla; the sockets epic only owns the CARLA socket port, which both link as Related.vendor.mdnow owns replacing that match with dimpl's typed variant if it lands second.dtls-after-ice.md's test note now allows the new variant for a post-ICE DTLS failure.main: no duplicate with the HLS media grid quest, which covers segmentation, or with the planning PRs moq-dev/moq#7747 and moq-dev/moq#7748.Public API: none (plan only). Wire: none.
Review
Codex
gpt-6.1-sol, medium effort, viaquest review --base origin/main --new, round 1 on head6169567485125f148117d0f1e20c8904db19baa3: no findings.Online review findings on the earlier heads, all fixed in
6169567485:vendor.mdowns it.moq import. Fixed.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code