feat(lint): flag video/img src pointing at an audio file - #3741
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
:large_green_circle: 7001fd034 COMMENTED
Small, well-scoped follow-up to #3609 that closes the provable direction (<img>/<video> pointing at a definitely-audio extension). Element-first gate is right, blast radius is nil, PR body already reasons through the cover-art / container-ambiguity cases correctly. Nothing blocking — a couple of small notes on where the reasoning could live in the code and where a test could discriminate a little harder.
Verified as OK
- Element-first gate is asymmetric the way the description promises.
findMediaSrcKindMismatchFindingsinpackages/lint/src/rules/media.tsearly-returns unlesstag.nameisvideoorimg, so<audio>never reaches the kind check regardless of extension —<audio src="whoosh.mp4">stays silent because the ELEMENT is filtered, not becausesrcKind("whoosh.mp4")returns null. That's the right shape: element filters the tag, kind filters the source. - Widened rule is behaviour-equivalent for the pre-existing image↔video directions. The rewrite around
expected+if (kind === expected) continuecollapses the two old!==guards into one and only opens the door for the newaudiokind.<video src="foo.png">and<img src="foo.mp4">still fire the same way #3609 shipped. srcKind()is module-private. Grepping the wholepackages/tree forsrcKind,AUDIO_SRC_EXT,SRC_KIND_NOUN, andmedia_src_kind_mismatchshows one caller only —findMediaSrcKindMismatchFindings. Widening the return union fromimage | video | nulltoimage | video | audio | nullhas zero downstream reach.data:audio/*handled at the same MIME branch as image/video.srcKindnormalizes with.toLowerCase()after the capture, soData:AUDIO/MP3;codecs="mp4a.40.2"classifies as audio; the[^;,]+group stops at the first parameter delimiter so;codecs=…and;charset=…don't pollute the MIME.blob:and non-audiodata:URIs still bail throughnull.- Cover-art asymmetry is a producer concern, not a lint concern, and the PR correctly keeps it that way. The rule fires on the extension alone;
probeMediaProfile'sisStillImageVisual/hasMovingVideoStreamfilters are not consulted here (and don't need to be —<img src="song.mp3">should fire regardless of whether the mp3 has attached_pic art, because the producer'svisualKind === "none"fail-close will drop it at render either way). Good to keep the lint layer extension-only. - All required CI green at head. Typecheck, Build, Lint, Test, Producer integration/unit, SDK unit+contract+smoke, CLI smoke, Studio load smoke, Render on windows-latest, Analyze (javascript-typescript / python / actions), CodeQL, Preview parity, Fallow audit — all pass at
7001fd034.
Concerns (non-blocking)
- The "why
.oggand.m4aare absent" reasoning lives in the PR body and the test title, not next toAUDIO_SRC_EXT. This is the hostage-note pattern — the next person who tries to widen the set won't see the reasoning at the site. A one-liner above the Set naming the container-ambiguity ("both can carry video, extension proves nothing, this rule iserrorseverity") would keep the constraint at the point of change. - The
.ogg/.m4atest's discriminator is one-sided.it("does not flag .ogg or .m4a…")puts both extensions on<video>elements. That catches the mistake of adding them toAUDIO_SRC_EXT(would fire audio-vs-video), but not the mistake of adding them toVIDEO_SRC_EXT(would silently classify as video and pass the test for the wrong reason). Per the PR's own logic — "the extension proves nothing about the file" — both mis-classifications are wrong. Putting one of the two on<img>(where kind=video would also fire) tightens the discriminator to catch both routes. - Common audio-extension gaps worth considering.
.aif(macOS/Windows short form of aiff),.oga(Ogg audio-only, distinct from.ogg), and arguably.amrare missing..aifis the most common of the three in the wild; a real customer composition with<img src="…theme.aif">would slip past this rule with the current set. Not blocking — extension coverage can grow later — but worth noting the set is not exhaustive.
What I didn't verify
- I did not run the four new vitest cases myself; I read them for shape and traced them against
findMediaSrcKindMismatchFindingsby hand. The PR body claims 552 pass; I trust that + green CI. - I did not walk every non-media rule to check that a new "audio-kind" concept elsewhere in
packages/lintcould collide with this widening. The grep forsrcKind/AUDIO_SRC_EXT/SRC_KIND_NOUN/media_src_kind_mismatchcovers the direct symbol surface; if a sibling rule later starts askingsrcKind()"is this audio?", it will now get an honest answer, which is fine but worth remembering. - I did not audit downstream Studio UI copy for the new message string — the wording change from
"a ${kind}"toSRC_KIND_NOUN[kind]("an audio file"etc.) may or may not be surfaced in snapshot tests I didn't chase.
State at HEAD 7001fd034: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — stamp needs pinning to 7001fd034 and any subsequent push voids it.
— Review by Rames D Jusso
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7001fd0 to
deb3f93
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
:large_green_circle: deb3f9329 COMMENTED (re-pin under require_last_push_approval: true)
Follow-up to the non-blocker (b) I named at 7001fd034 — the .ogg/.m4a fixture pinned only the AUDIO_SRC_EXT route because both entries were on <video> tags; a mistaken addition to VIDEO_SRC_EXT would have silently classified them as video and passed the test. Now closed.
Interdiff scope 7001fd034 → deb3f9329 (verified git diff byte-identical on packages/lint/src/rules/media.ts; delta is test-only):
packages/lint/src/rules/media.test.ts+4/-1. Thedoes not flag .ogg or .m4a, whose containers can carry video toofixture now carries<video id="v1" src="clip.ogg">,<video id="v2" src="clip.m4a">,<img id="i1" src="clip.ogg">,<img id="i2" src="clip.m4a">. Title updated from.ogg, which carries video as well as audioto.ogg or .m4a, whose containers can carry video too.
Discriminator check — traced both mis-classification routes at source against findMediaSrcKindMismatchFindings:
- If
.ogg(or.m4a) added toAUDIO_SRC_EXT:<img>yieldskind === "audio",expected === "image",kind !== expected→ finding fires → assertiontoBeUndefined()fails. ✓ - If
.ogg(or.m4a) added toVIDEO_SRC_EXT:<img>yieldskind === "video",expected === "image",kind !== expected→ finding fires → assertiontoBeUndefined()fails. ✓ (new route this fixture closes) - If
.ogg(or.m4a) added toIMAGE_SRC_EXT:<video>yieldskind === "image",expected === "video",kind !== expected→ finding fires → assertiontoBeUndefined()fails. ✓ (also closed, since fixture retains video rows)
The fixture is now honest across all three mis-additions.
Everything else unchanged from R1 at 7001fd034. srcKind/AUDIO_SRC_EXT/SRC_KIND_NOUN/media_src_kind_mismatch still module-private (single caller). findMediaSrcKindMismatchFindings element-first gate unchanged. data:audio/* MIME branch unchanged. Cover-art asymmetry still correctly at producer layer. All prior "Verified as OK" bullets still hold.
What I didn't verify — the four new vitest cases from R1 haven't been rerun by me (still not running local vitest). CI hasn't settled at HEAD yet: Preflight, Typecheck, Build, Test, Test: runtime contract, CLI smoke, Studio: load smoke, Producer: unit/integration, Format, and the CodeQL/Analyze suites are pending on run 34074069580. Since the delta is a test-only change under an existing describe block, CI red on any unrelated required context would be a pre-existing flake, not a new regression.
Non-blockers (a) and (c) from R1 explicitly deferred per author's call: .ogg/.m4a container-ambiguity reasoning stays in PR body ("this repo's rationale goes in the PR body, not the source"), and extension coverage widening (.aif/.oga/.amr) is a scope call for a separate PR. Both stances are reasonable — noting for the record that (a) is a repo convention I'll respect going forward on HF-OSS.
State at HEAD deb3f9329: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED, one commit (refactor(lint): drop m4a from the audio src kinds — force-pushed amend of the same commit at 7001fd034, kept parent e374f260). hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA and any push voids it.
— Review by Rames D Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at deb3f9329cd22342ab83c4c4b9108eecb42ffb3b. CI is green across all six workflow runs at this head (CI, Windows render verification, CodeQL, regression, Player perf, preview-regression -- all success). The skipped lanes are path-filtered, and Tests on windows-latest is a gate job whose "Require all Windows test lanes" step is skipped for the same reason, so its 2s green is vacuous rather than a signal.
I read the rule rather than riding the prior review. srcKind is well-behaved on the inputs that usually break this kind of check: data: URIs are classified by MIME rather than by a nonexistent extension, blob: returns null, query strings and fragments are stripped for both the absolute and the relative form, and the new URL() / decodeURIComponent pair is wrapped so a malformed absolute URL degrades to the same split instead of throwing. The element-first gate (tag.name !== "video" && tag.name !== "img") means <audio> never reaches the comparison, which is right.
One thing on the record, not a blocker.
.ogg, .m4a, .oga and .aif are in none of the three sets, so srcKind returns null and they are silently tolerated on <img> as well as on <video>. media.test.ts:897 pins that: it asserts <img id="i2" src="clip.m4a"> produces no finding.
The test's stated reason -- "whose containers can carry video too" -- holds for .ogg (Ogg carries Theora) but not for .m4a. .m4a is the audio-only MPEG-4 variant by definition; the video-bearing spellings are .m4v and .mp4, and .m4v is already in VIDEO_SRC_EXT. So an <img> pointed at a .m4a can never render, which is exactly the failure this rule exists to catch, and it is the one case the rule waves through.
Moving .ogg and .m4a into VIDEO_SRC_EXT gets both behaviors you appear to want: <video src="clip.m4a"> stays unflagged (kind === expected), while <img src="clip.m4a"> starts reporting. That is narrower than putting .m4a back into AUDIO_SRC_EXT, which would also flag the <video> case you deliberately dropped in deb3f9329.
Not blocking on it. Every residual gap here fails toward under-reporting, which is the right direction for a lint rule. Your call whether to take it now or leave it.
-- Rames
What
media_src_kind_mismatchnow also fires when a<video>or<img>owns asrcthat is an audio file (mp3,wav,aac,flac,opus,aiff,wma, or adata:audio/*URI).<audio>is still never flagged, including<audio src="sfx.mp4">.Why
#3609 added this rule for the image↔video directions and dropped audio from the type lattice entirely —
srcKind()returnedimage | video | null, so an audio extension fell through asnulland the rule bailed.That was the right call for the
<audio>element:<audio src="clip.mp4">is legitimate, and an extension cannot prove whether that mp4 carries an audio stream. But it was implemented by deleting the audio kind rather than by skipping the audio element, which also dropped a direction that is provable: an<img>or<video>pointing at an mp3 can never satisfy the producer.Found while auditing the
ASSET_MEDIA_TYPE_MISMATCHrenders still failing in prod after #3609 — one is a real<img src=".../brand-assets/music/….mp3?…">in a customer composition.Containers that are excluded, and why
.oggand.m4aare deliberately absent: both are containers that can carry video, so the extension proves nothing about the file and this rule is error severity.The cover-art case is not a false positive
An mp3 with embedded cover art does carry a video stream, so it is worth spelling out why this still cannot fire wrongly. Probed a real one:
In
probeMediaProfile,isStillImageVisualkeys off the container demuxer (STILL_IMAGE_DEMUXERS), andmp3is not one, soisStillImageis false;hasMovingVideoStreamfiltersattached_pic === 1, so that is false too.visualKindlands on"none", which satisfies neitherexpected: imagenorexpected: video— the producer fail-closes either way.Test plan
bunx vitest runinpackages/lint— 552 pass, including four new cases:<img>+mp3,<video>+wav,<audio src="*.mp4">stays clean,.ogg/.m4astay cleanbun run typecheckinpackages/lint