fix: support Loom transcript extraction#355
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a0c54c3ce
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84288c945
ℹ️ 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".
|
@codex review Please re-review the current head ( Since the previous review on
Do not merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d084d2b51
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Please re-review the current head ( Since
Do not merge. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Codex review: needs maintainer review before merge. Reviewed July 10, 2026, 8:35 AM ET / 12:35 UTC. Summary Reproducibility: yes. at source level: current main lacks Loom media-host classification, and #353 provides a concrete CLI command that reaches Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Ship Loom through the existing generic yt-dlp/transcriber pipeline with the current fail-closed URL safeguards, while retaining the shared audio selector only if maintainers are comfortable that supported extractors expose reliable audio-codec metadata; otherwise scope that selector change to Loom. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main lacks Loom media-host classification, and #353 provides a concrete CLI command that reaches Is this the best way to solve the issue? Yes. Extending the existing generic yt-dlp and transcription chain is narrower and more maintainable than bespoke Loom API or scraping code, and the branch now preserves guarded daemon behavior and rejects foreign redirect content. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 206a6f844764. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles)
|
… into fix/loom-transcript-extraction
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Pushed HEAD Polish since last push:
Open Codex P2 threads (caption finalUrl, auto YouTube, cache-before-HTML) replied + resolved. Live transcript proof still blocked — no transcription API key / whisper.cpp in this environment. Do not treat as ClawSweeper-ready for live success until Kartik can supply a key (or run extract with a configured provider). Code quality gates ( Do not merge from this comment. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Live Loom → whisper.cpp proof + Firecrawl fail-closed@clawsweeper re-review Live transcript proof (required)
Firecrawl P1/P2 from prior review
Do not merge from this comment. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
--video-mode transcriptLoom failures as a real CLI/non-zero failure (not empty successful extract content with the error only in diagnostics)shouldPreferUrlMode(...)policy used by Chrome/daemon auto routingvideo.kind: "direct"(prefer/transcript-only still does, for yt-dlp identity)htmlBaseUrlfor relative caption resolution; cross-ID / conflicting metadata discards foreign HTML (and Firecrawl markdown)Root cause
generic/not_implementedfor public Loom pages.bestfallbacks. For hosts with split HLS (including Loom), that can resolve to a video-only rendition and fail withMedia has no audio stream.shouldPreferUrlModefailures as best-effort URL-only empty successes. Because Loom was briefly prefer-url-mode, CLI prefer failures were swallowed into empty successful extracts with the real error buried in diagnostics.resolveTranscriptForLink(url, null, ...), so embedded<track kind="captions|subtitles">discovery never ran on the real prefer path.resolveTranscriptForLinkwithout disabling embedded YouTube substitution allowed incidental YouTube iframes to replace the Loom URL before the generic provider ran.shouldPreferUrlMode(...)helper hard-routed Chrome/daemon requests into URL/video extraction even though guarded daemon URL flows intentionally disable yt-dlp for unresolved URLs.{ kind: "direct", url: loomShareUrl }made thin Loom pages look like downloadable media (isVideoOnly/loadRemoteAsset/ slides).Implementation
isLoomVideoUrl/isSameLoomRecordingwith structured URL parsing forhttp(s)://loom.com|www.loom.comshare/embedpaths and a lowercase 32-hex video id.autoandprefer, always with the original Loom URL (mediaKind: "video",service: "generic").TRANSCRIPT_ONLY_STRATEGIES: cache-before-HTML, same-IDhtmlBaseUrl,embeddedMediaUrl: null, failures throw.htmlBaseUrl/ discard foreign HTML on cross-ID redirect, disable YouTube substitution, setvideo: null.htmlBaseUrlfrom same-ID metadata (url/ogUrlpreferred oversourceURL); conflicting Loom IDs discard HTML/markdown/metadata text.inputMode: "video".DEFAULT_AUDIO_FORMATso every combinedbestfallback requires[acodec!=none].Verification
Live behaviour proof
Public fixture:
https://www.loom.com/share/ef3224a48a084371bd6d766ee81f083f(~51 min)yt-dlp extractor / audio-capable selection (earlier on this PR):
Built CLI: explicit prefer failure is a real reject (no transcription provider):
Live end-to-end transcription: SUCCESS (2026-07-10, local whisper.cpp)
Branch tip at proof run base:
2ca44d4b(Firecrawl non-Loom fail-closed landed after asb0e49833; Loom→whisper path unchanged).Transcript excerpt (public fixture; no redaction):
Also since ClawSweeper's Firecrawl non-Loom final-URL finding:
b0e49833fails closed when authoritativeurl/ogUrlis outside the same Loom recording (regression intests/link-preview.loom-auto-youtube.test.ts).pnpm -s checkpassed on that tip.Security
Raw Loom URLs were not allowed to bypass the daemon’s guarded fetch/external-downloader boundary. Firecrawl metadata URLs used as
htmlBaseUrlmust passisSameLoomRecording(Loom host + same 32-hex id). Cross-ID / conflicting metadata discards foreign HTML. AUTOvideo: nullavoids treating share HTML as direct media.Fixes #353