feat(lint): validate audio group membership and timing - #3447
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 759c3e2d. No blockers — the three rules are correct as written, and the PR-body claim I could falsify holds at source. Everything below is test coverage, plus one message nit.
Verified rather than read
"align membership checks with the audio-only runtime/render model" is true. resolveAudioGroups collects members with root.querySelectorAll(`audio[${HF_AUDIO_GROUP_ATTR}]`) (packages/core/src/audioGroups.ts:124, with HF_AUDIO_GROUP_ATTR = "data-audio-group" at :17), so the rule's tag.name === "audio" plus readAttr(…, "data-audio-group") is an exact match for what actually resolves at runtime. Worth saying out loud because the two volume rules immediately above these reach for isMediaTag() — the divergence here is deliberate and right, not a slip.
Also checked and clean: hasAttrName's regex does not prefix-match, so a bus carrying only data-start-offset does not trip data-start; uppercase and self-closing <HF-AUDIO-GROUP …/> both resolve; a bus inside a <template> shell is reached. And there is no lint-code catalogue to register the three new codes in — docs/packages/lint.mdx names no codes at all and HyperframeLintFinding.code is a bare string.
How the rest was checked
I ran the shipped media.ts at this head against the real buildLintContext and utils.ts, with htmlparser2 pinned to the package's own ^10.1.0. Two edits only: the @hyperframes/parsers colour-grading import stubbed (no audio rule calls it) and an export line for the three functions. The baseline reproduces all 12 of your new assertions green, so the mutants below are measured against a harness that demonstrably works.
| mutation | your suite |
|---|---|
drop if (mayHaveCrossFileMembers) continue; |
killed |
drop data-track-index from AUDIO_GROUP_TIMING_ATTRS |
killed |
drop the hf-audio-group guard in the carve rule |
killed |
drop if (!elementId) continue; |
survives |
drop if (memberGroupIds.size === 0) return []; |
survives |
widen tag.name === "audio" to isMediaTag(tag.name) |
survives |
1. stays quiet for a bus with no id passes for the other guard's reason
Its fixture is a lone id-less bus, so memberGroupIds is empty and the function returns at the first guard. Delete if (!elementId) continue; and the test stays green. Give the fixture a member and the mutant is loud:
<hf-audio-group data-label="Nameless"></hf-audio-group>
<audio id="a" … data-audio-group="sfx"></audio>
shipped -> (quiet)
guard deleted -> audio_group_no_members, elementId: null, "#null is an audio group…"
One data-audio-group added to that fixture pins it.
2. Same for stays quiet in a file that declares no members at all
That test's comment is the clearest statement anywhere of why the empty-members early return exists — but its fixture also carries a data-composition-src host, so mayHaveCrossFileMembers covers it and the early return is never the reason it passes. The early return's own live case:
flat file, one bus, no members, no composition host
shipped -> (quiet)
early return deleted -> audio_group_no_members on #orphan
Dropping the host from that fixture pins the guard the comment is actually about.
3. The audio-only narrowing is unpinned, and it fails toward silence
This is the one I would add, because the PR body makes it the contract. Nothing goes red if the member filter widens:
<hf-audio-group id="voiceover"></hf-audio-group>
<video id="v" … data-audio-group="voiceover"></video>
<audio id="a" … data-audio-group="sfx"></audio>
shipped -> audio_group_no_members on #voiceover (correct — resolveAudioGroups ignores the video)
widened -> (quiet)
A <video> claiming membership is precisely the case where the bus really is dropped at runtime, so it is the case an author most needs to hear about. One test on that fixture keeps the alignment from drifting back.
4. nearby names every other group in the file, not the near misses
Executed against a file with one healthy bus and one typo:
#voiceover is an audio group no clip belongs to … Clips in this file name "music", "voiceovr" instead.
music is the correctly-matched sibling bus's own id, offered as a fix for a typo it has nothing to do with. With four groups in a file the suffix is just the file's group list. Either rank by edit distance and keep the closest one or two, or drop ids that already match some other bus.
CI
Same stack position as #3444-#3446: of main's 8 required contexts, only regression is present at 759c3e2d, and it is green. Build, Test, Typecheck, Test: runtime contract, both windows jobs and Semantic PR title have never run, because ci.yml filters branches: [main]. That self-repairs on retarget — edited is in its types: with a comment explaining exactly this — so no action, but it does mean the 142 new test lines here have not executed in CI at any SHA yet. Perf: ${{ matrix.shard }} showing SKIPPED with the matrix variable unexpanded is a job-level if: that evaluated false, not a job still queued.
— Review by Rames (pr-review), James's assistant
84add19 to
beb41a0
Compare
759c3e2 to
22296f5
Compare
|
Addressed the review follow-ups:
The media lint suite passes (57 tests), the lint package typechecks, and the full build is green. |
9294964 to
16b543f
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 16b543f221. My earlier review (5003551802) was pinned to 759c3e2dc5, which the restack replaced; the branch is now de61cf21 + 16b543f2 "pin audio group membership guards".
All three surviving mutants are dead, and the new fix is pinned too. I re-ran the harness rather than reading the new tests.
Method
Rebuilt the harness against this head: the shipped packages/lint/src/rules/media.ts byte-for-byte except three lines (relative import specifiers, the @hyperframes/parsers colour-grading import stubbed — no audio rule calls it — and an export line), driven by the real buildLintContext with htmlparser2 at the package's own ^10.1.0. The test corpus is your suite transcribed, including the two new cases and the two you changed. Baseline reproduces all 16 assertions green, so the mutant results are measured against a harness that demonstrably works, and every mutant was diffed after editing to confirm the edit landed — a no-op reads exactly like a survivor otherwise.
| mutation | round 1 | this head |
|---|---|---|
drop if (!elementId) continue; |
survives | killed — stays quiet for a bus with no id |
drop if (memberGroupIds.size === 0) return []; |
survives | killed — stays quiet in a file that declares no members at all |
widen tag.name === "audio" to isMediaTag(tag.name) |
survives | killed — does not count video as group membership |
drop if (mayHaveCrossFileMembers) continue; |
killed | killed |
drop data-track-index from AUDIO_GROUP_TIMING_ATTRS |
killed | killed |
drop the hf-audio-group guard in the carve rule |
killed | killed |
revert nearby to [...memberGroupIds] (new) |
— | killed — suggests only unmatched member ids… |
Each of the three now dies to exactly the test that should own it, which is the part that matters: the two fixture changes were minimal and surgical — one member added to the id-less-bus fixture, the composition host dropped from the no-members fixture — and each moved the test off the guard it was accidentally exercising and onto the one it is named for.
The message nit is fixed, and the fix is itself pinned
const unmatchedMemberGroupIds = [...memberGroupIds].filter((id) => !declaredGroupIds.has(id));so a healthy sibling bus can no longer be offered as the typo correction. Reverting that one line turns suggests only unmatched member ids, not a healthy sibling group red, so the new behaviour is pinned rather than merely present — which is the thing that was missing from the three guards in the first place.
Nothing open from me on this rung.
— Review by Rames (pr-review), James's assistant
16b543f to
e8672b3
Compare
4ed7032 to
a678fed
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at e8672b31ca (prior review 5003703323 @ 16b543f221).
Restack only — both files in the PR's own file list are blob-identical across 16b543f221 → e8672b31ca, checked by blob sha per path, so the prior review transfers exactly.
The findings it closed stand: all three previously-unpinned guards now die to the test that owns them. I verified that by execution rather than by reading the added fixtures — replaying the shipped rule module against the real lint context, 16 assertions green as a baseline, then 7 of 7 mutants killed, including one reverting the nearby fix so that fix is pinned too.
No blockers.
— Review by Rames (pr-review), James's assistant
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at e8672b31ca, the exact head of my re-review above — that review is the audit; this is the stamp <@vanceingalls> asked for in Slack.
No blockers on the code. The one nit I raised on #3445 (the mock gain defaulting to 1, so the toBe(1) solo assertions are weaker than they read) is non-blocking and that code is deleted at #3454.
Scope of this approval, stated so it is checkable: it covers e8672b31cabca5e23045c39a77f74d8655002ed2 only. main runs require_last_push_approval: true, so if this head moves the PR returns to REVIEW_REQUIRED rather than carrying my stamp forward — re-ping me and I will re-review at the new head.
— Review by Rames (pr-review), James's assistant
miguel-heygen
left a comment
There was a problem hiding this comment.
Independent exact-head pass at e8672b3.\n\nThe three rules match the runtime contract rather than a broader HTML guess: only audio tags count as members, timing attributes are rejected on a composition-clock bus, and carve remains a clip-only attribute. The no-members rule correctly fails closed only when the current file can establish emptiness, excluding cross-file hosts and files with no local membership evidence. The revised fixtures isolate each guard, including the audio-only and unmatched-nearby cases.\n\nAll current required checks are green.\n\nVerdict: APPROVE\nReasoning: The lint surface mirrors the shipped audio-group model and the previously weak guards now have direct, mutation-sensitive witnesses.\n\n— Magi
Part 4 of 12 replacing #3439. Base:
wa-26c-engine-group-render.Why
The authoring contract should reject invalid bus timing and carve attributes without flagging valid cross-file or element-less groups.
What
<hf-audio-group>Verification
fallow audit --base main --fail-on-issuesThe final stack tip preserves the verified #3439 replacement and includes the review fixes landed across the stack.
Stack: #3446 ← #3447 → #3448