Skip to content

feat(routing): an editor for the second (VOD) audio mix, and an EXPERIMENTAL convention for the two features nobody has watched work - #365

Merged
rainmanjam merged 8 commits into
mainfrom
feat/vod-profile-editor-and-experimental-labels
Aug 15, 2026
Merged

feat(routing): an editor for the second (VOD) audio mix, and an EXPERIMENTAL convention for the two features nobody has watched work#365
rainmanjam merged 8 commits into
mainfrom
feat/vod-profile-editor-and-experimental-labels

Conversation

@rainmanjam

@rainmanjam rainmanjam commented Aug 15, 2026

Copy link
Copy Markdown
Owner

What this is

Two jobs. vodProfile gets an editor, and the two features in 0.7.0 with a gap in the evidence behind them start saying where that gap is.

Read section 4 first if you reviewed an earlier revision. The labels this PR added were themselves wrong, in both directions, and a five-reviewer pass caught it. Section 2 below is kept as written and corrected in place, because the mistake is the most useful thing in this PR.


1. The second (VOD) audio mix has a UI

vodProfile shipped complete on the server — the column, the migration, the API, routing.CompilePair, the engine's gate on Twitch's one-track ingest — and appeared in the frontend exactly once:

ui/src/lib/types.ts:268:  vodProfile?: RoutingProfile | null;

A type and nothing else. An operator could switch Enhanced Broadcasting on, watch it negotiate, and have no way to say what the second mix contained.

One editor, two instances

vodProfile is a *routing.Profile — the same type as a destination's primary Profile — so a second editor written beside the first would be two implementations of one thing that diverge on the first control either gains. The editor block under the destination picker in RoutingPage.tsx is now a single ProfileEditor component rendered twice: same track picks, mix matrix, music rights, loudness, delay, ducking, presets, and its own compiled filter graph from the same Go code that will run.

The existing editor was not reusable as-is. Two things had to change, and both were latent bugs rather than cosmetics:

  • DOM ids had to stop being constants. id="norm" is fine with one editor and a duplicate id with two, at which point <label for> resolves to whichever came first. Every id in the editor is now built from an idPrefix. This turned out to reach further than the extracted block — see finding 1 below.
  • Each instance compiles itself. The filter string under the controls is the honest part of this editor: it comes back from the same Go code that will run, not a TypeScript reimplementation. A second mix borrowing the first one's compile would show a graph that is not its own. The debounce, the request and the result live in the component; only the error travels up, because refusing to save is the only thing the page does with it that the component cannot.

Presets moved into the shared component too, so an archive preset can be dropped on the second mix without the live one moving — which is most of the point of having a second mix.

The design constraints

  • Optional, and null stays the default. Off on every destination. Switching it on seeds from the live mix, so the first edit is the actual difference you wanted rather than a blank profile you have to rebuild.
  • Round-trips. Switching it off sends an explicit null: handleUpdateDestination decodes over the stored row, so an omitted field leaves the pointer alone and the operator watches their delete undo itself on the next load. Three new API tests cover set/reload, explicit-null clearing, and that a PUT which says nothing about vodProfile (a rename, an enable toggle) leaves it alone.
  • Not gated on Twitch. routing.CompilePairengine/destinations.goffmpeg.secondAudioMap is a real two-mix egress and correct for every other target, so hiding the control off Twitch would hide a working feature. What is gated on Twitch is the explanation.
  • Never implies the second mix is live when it is not. Three states, three sentences:
    • Twitch RTMP with the toggle off — the engine refuses the pair at plan time (vodNeedsNegotiation, noteVODWithoutMultitrack), so the page says the mix is not being sent, in those words, and names the fix.
    • Twitch RTMP with the toggle on — the answer does not exist until go-live, so the page says so and points at the destination card. It does not claim a track no negotiation has granted.
    • Not Twitch — nothing is negotiated; both mixes are built in one FFmpeg process, and whether the far end takes a second track is a property of that endpoint.

2. EXPERIMENTAL, everywhere, still fully usable

Twitch Enhanced Broadcasting has never run against Twitch's live endpoint, and no NVENC/QSV/VA-API encode has ever been observed. Both halves of that sentence were false, and section 4 replaces it. The negotiation does run against Twitch and succeed; what has never been observed is a broadcast published through the key it mints. And VideoToolbox's flags are confirmed by a real encode — only NVENC, QSV, VA-API and AMF are not.

The rest of this section stands: both features are now labelled, and neither is gated — no feature flag, no hidden control, no opt-in env var. Every toggle works exactly as before.

The convention

There was none, so this establishes one. Four surfaces, one rule:

Surface Form
UI <Experimental> / <ExperimentalBadge> in ui/src/components/Experimental.tsx
Go an // EXPERIMENTAL: <what is unverified> line in the doc comment
docs/ a > **EXPERIMENTAL — <claim>.** blockquote under the heading
CHANGELOG a leading **EXPERIMENTAL.** sentence on the entry

The rule that matters is the wording, not the badge: every use names the specific claim that has not been tested. "Beta" tells a reader to be vaguely nervous and gives them nothing to act on. "No broadcast has been published through a key Twitch minted" tells them which part is a guess and what a failure would mean. A use of <Experimental> that only says "experimental" is a bug in the copy, and the component requires its children for that reason.

A specific claim can also go stale in the direction of being wrong, which is worse than being vague — and this convention did exactly that on the commit that introduced it. Experimental.tsx now says so, and says the only thing that prevents a repeat: when you edit a use of this component, run the thing it is about.

Why the badge is outline and not warn. The app's five saturated tokens — live, warn, down, armed, idle — mean the state of a destination. "Unverified on hardware" is a property of the feature, not of any running thing, so amber would put it in a vocabulary that already means something else and train operators to read a healthy broadcast as broken. DestinationDialog already makes this argument about the EB fallback in its own comment; this follows it.

Where it landed

UI: the EB toggle (DestinationDialog), the GPU inventory (SettingsPage), the new VOD editor, the rendition encoder picker (only when a hardware encoder is selected — corrected in section 4 to gate on the encoder family, because hardware includes VideoToolbox, which is confirmed), and the go-live result on DestinationCard. Go: internal/multitrack, internal/engine/multitrack.go, encoderProfiles in internal/ffmpeg/rendition.go. Docs: ENCODING.md, HARDWARE.md, RENDITIONS.md, AUDIO-ROUTING.md, and — added in section 4 — COMPARISON.md, TROUBLESHOOTING.md and the marketing site. Plus the four 0.7.0 CHANGELOG entries.

One existing claim was corrected on the way: encoderProfiles' doc comment said its values were "verified by running ffmpeg -h encoder=<name> and a real one-frame encode" — true, but an option table is compiled into the binary and answers identically on a machine with no such device, and the one-frame encode only ran for encoders the container could open.


3. External review — codex and agy

Both were run against this branch. Both clearly reviewed this repository (agy's output cites real line numbers in these files). Every finding was checked against the source before I acted on it.

Raised by both, confirmed, fixed

1. Duplicate DOM ids survived the extraction. I namespaced id="norm" and stopped there. TrackRows renders id={\track-${index}`}and a matchinghtmlFor, so with two editors mounted the VOD editor's "Track 1" label resolved to the *live* editor's checkbox — a wrong edit to the wrong profile with nothing on screen saying so. **Confirmed** by reading TrackRows.tsx:126,132; it is the only consumer of the component (TrackSummaryis a separate export). Fixed with a **required**idPrefix` prop, so a third instance cannot inherit the collision by omission.

2. A compile response could win out of order. The effect cleanup cancelled the debounce timer but not a request already on the wire. Two edits a few hundred ms apart put two requests out, and a slow first response landing after a fast second one paints a filter string the controls do not describe — the exact claim this panel exists to make. Codex additionally noted it could call setVodError after the VOD editor unmounted, leaving Save disabled by a mix that no longer exists. Confirmed by reading the effect. Fixed with a cancelled flag checked in both .then and .catch. (This shape predates the change — the original single-editor effect had it too — but two editors make it reachable in a new way.)

Raised by codex only, confirmed, fixed

3. The UI claimed a track it was not sending. Three strings said the second track was "carried", "published" and "produce[d]", unconditionally — three lines below the alert correctly saying it is not being sent when the toggle is off. Confirmed by reading my own SecondMixCard. The switch label now describes the configuration ("A second mix is configured for this destination"), the description says "intended for", and the VOD editor footer is conditional on the plan-time gate. This was the single most valuable finding in the review: the whole point of the job was not to imply a live second mix, and I had left three claims that did.

4. The EXPERIMENTAL label contradicted the doc it was attached to. My new paragraph in internal/multitrack said the negotiation had "never [been] measured against ingest.twitch.tv", nine lines above the pre-existing "THREE THINGS MEASURED AGAINST THE LIVE ENDPOINT". Confirmed — and both statements were true of different things, which the comment did not say. The observations are real captures from the endpoint; what has never happened is a request leaving this process for it. Reworded so the boundary is stated once, precisely, and the "three things" preamble points at it. Codex also noted the go-live decision on DestinationCard was unlabelled despite being where both the dialog and the editor send the operator for the answer; it now carries the badge alone rather than the full block, because it renders on every broadcast and a paragraph there would be the "warning every time" that note's own comment exists to avoid.

Refuted / not acted on

Nothing was refuted — every finding survived the source. Neither reviewer raised a false positive.

Could not verify

Codex ran the Go suite in a sandbox without loopback listeners, so internal/multitrack's httptest tests failed there for environmental reasons and it could not judge them. I ran the full suite locally and it passes; codex's own note says as much.

Neither reviewer could exercise the editor in a browser, and neither could I — the repo has no component-level test harness (only lib/ unit tests and the theme guards). The two-editor interaction is therefore verified by reading and by the type system, not by a rendered click. That is the weakest evidence in this PR and it is why idPrefix is a required prop rather than a defaulted one.


4. Five-reviewer review — two Criticals and seven Majors

docs/notes/review-365.md on this branch is the full report. Both Criticals were found the same way: by running the thing the label was about. Every reviewer who only read the diff — including me, for most of that session — repeated the claim back as fact.

C1 — the label asserted a falsehood the suite disproves on every run

internal/multitrack/multitrack.go:5 said "THIS CODE has never talked to Twitch". It has. live_test.go reaches ingest.twitch.tv on every run and does not skip (the package is absent from skips.json, so a t.Skip there fails the ratchet). Run it:

$ go test ./internal/multitrack/ -run TestTheLiveEndpoint -v -count=1
    live_test.go:120: live refusal, verbatim: Twitch declined to configure Enhanced Broadcasting:
        Your broadcast software (polyemesis) did not send GPU Information which is required by
        GetClientConfiguration provided by Twitch Enhanced Broadcasting. …
    live_test.go:187: live negotiation: 1 rendition(s), live track 0, VOD track 1
    live_test.go:260: Twitch minted a 314-character key from the 44-character one it was sent
--- PASS (0.09s / 0.01s / 0.01s)

Beyond being wrong, it told a maintainer there is no live test — the only canary for Twitch tightening its allowlist.

The boundary now applied at all ten sites (multitrack.go:5,14,16, CHANGELOG.md ×2, AUDIO-ROUTING.md, DestinationDialog.tsx, Experimental.tsx, SettingsPage.tsx, RoutingPage.tsx):

The negotiation runs against ingest.twitch.tv and succeeds — Twitch accepts a supported-GPU inventory, grants a VOD audio track and mints a key. What has never been observed is a broadcast published through a minted key: everything after Negotiate returns.

internal/engine/multitrack.go was accurate about its own file but cited the package doc as its authority and inherited the error by reference. It is re-aimed: that file is now the part the label is actually about, because negotiateDestination has only ever been driven by an httptest server.

One mechanism stated plainly, where an operator can act on it. live_test.go declares an NVIDIA GeForce RTX 3080 and Twitch grants it — on an Apple M1 Ultra. Twitch validates the declared inventory (vendor ID, device ID, driver version, against a list it does not publish), not the hardware; it has no way to see the machine. That is the mechanism behind ENCODING.md's existing "a gate, not a workload", and it is why the Settings page asks an operator to fill an inventory in rather than reading one — polyemesis sends what it is told. It is stated in multitrack.go's package doc, in ENCODING.md §2, and on the Settings panel itself. No copy anywhere suggests declaring hardware you do not own; that is the owner's call and this PR does not make it.

C2 — the hardware label was false for VideoToolbox

TestEveryConfiguredEncoderOpensWithItsOwnFlags runs a real encode per registered encoder with that encoder's own row from encoderProfiles — preset flag, rate control, the capped-VBR path:

$ go test ./internal/ffmpeg/ -run TestEveryConfiguredEncoderOpensWithItsOwnFlags -v -count=1
    rendition_encoder_profiles_test.go:281: opened with our own flags:
        h264_videotoolbox hevc_videotoolbox libx264 libx265
--- PASS (1.91s)

Narrowed to NVENC / QSV / VA-API / AMF at all five sites (rendition.go:241, ENCODING.md:153, RENDITIONS.md:259, HARDWARE.md:451, RenditionsPage.tsx:1697). The UI badge was gated on encoder?.hardware; it is now gated on the encoder family via a new flagsUnconfirmed() predicate, so a Mac operator is no longer warned off the one hardware encoder that is verified. That mattered for a second-order reason the review named: having watched the badge fire falsely once, an operator discounts it on h264_nvenc, where it is true.

That test is the strongest evidence those flags have and it went unmentioned beside the table it answers for. It is now named in rendition.go, ENCODING.md, RENDITIONS.md, HARDWARE.md and the CHANGELOG — including the property that makes it worth naming: it answers for whichever encoders the machine running it registers, so a CI runner with an NVIDIA card would retire the remaining caveat by itself.

M1 — the applyPresetTo race (ship-blocker)

RoutingPage.tsx:306-318 awaited and then called apply(res.profile) unconditionally. Click a VOD preset, switch the second mix Off before the answer lands, and it came back on with setDirty(true); Save then persisted a vodProfile where null was intended — defeating the guarantee this PR exists to make. Same across a destination switch, and two clicks could land out of order.

applyPresetTo now takes "live" | "vod" instead of a raw setter and re-checks three things after the await: the destination is still the one that asked, the mix is still enabled, and no newer click superseded this one. Because a closure captures values, the checks read refs written at the same moments the state is — reading selected or vodProfile in the callback would read what they were at click time, which is precisely the question being asked. The counter is per mix, so a live preset and a VOD preset in flight together do not cancel each other.

M2 — live-mix regression vs main

The same function stopped setting compiled/compileError. Apply a preset that fixes an invalid profile and the Result card kept the old graph, the old warnings and the old red error, with Save disabled, for 180 ms + RTT — on the primary profile, where main cleared all four in one commit.

Fixed without two sources of truth: applyPresetTo now returns the routing the endpoint already compiled, and ProfileEditor writes it into its own compiled — the same state its debounced effect writes, which overwrites it with the same answer a moment later. The page keeps no copy; it only passes the result to the instance that asked. A discarded answer (any M1 guard firing) returns null and the editor paints nothing.

M3 — the footer claimed a track Twitch may refuse

:544-551 rendered the unconditional "This graph is the SECOND audio track" whenever !vodBlockedByToggle — which includes a Twitch destination with EB on, exactly where negotiateDestination may refuse and one track is sent, contradicting SecondMixCard's hedge two cards above. The footer is now four states, and the unconditional sentence is reserved for the only case where nothing is conditional: a probed ingest, off Twitch.

M4 — provisional was unaccounted for, on both sides

engine/destinations.go:123 dropped the second mix on every platform when the ingest is unprobed and, unlike the Twitch branch, set no vodDropped — so nothing reached the destination card or the log. The engine gap was small and safe, so it is fixed rather than deferred: the provisional case is now its own arm setting noteVODProvisional, deliberately worded not to offer the Twitch fix (there is nothing to switch on, and the destination need not be on Twitch). It is the one of the three drops an operator is least able to work out, because unlike the other two it is not caused by anything they configured. Covered by TestAVODMixOnAnUnprobedIngestIsNotSentAndSaysSo, on a non-Twitch row on purpose — the sibling test asserts a non-Twitch destination is never told its mix was dropped, and this is the one case where it must be.

On the page, the copy is gated on ctx.probed, which it already had and did not use.

M5 — switching the second mix off destroyed its configuration

No confirmation, no undo, and toggling back on re-seeded from the live mix. The last non-null profile is now held and restored; the live mix seeds only when there is nothing to restore, and the stash clears on a destination change (it is an undo for the toggle, not a clipboard between destinations). No ConfirmDestructive dialog — the review suggested one, but making the action reversible is strictly better than confirming it, and the switch then means what it looks like it means. The card says so when there is something to restore.

M6 — two fields not pinned

len(Tracks) and Gain[0] left Mode, Track and Enabled unpinned, so a handler that stored a matrix mix as simple mode, or that zeroed the entire selection, would pass. Both mutations previously SURVIVED; both now die (table below).

M7 — label coverage missed the highest-traffic surfaces

web/src/pages/features.astro:29-31 described EB as working in three unqualified sentences — the single most likely place a user meets it, and a caveat that appears only after install arrived too late. The site now carries the same convention (note: on a section, neutral border, placed after the description so the feature is described before it is qualified), on both the EB section and the renditions section.

web/scripts/check-build.mjs enforces that capability-row labels on the built pages appear in docs/COMPARISON.md, so the site and the document were changed together and the build check passes. The hardware-encoding caveat is a footnote rather than a table cell in both copies, because a cell saying "unconfirmed" would be false in the other direction: all five encoders are offered and all five are probed with a real one-frame encode.

Also: comparison.astro (both tables), docs/COMPARISON.md:111,225 plus a new footnote 4, ENCODING.md:30,240, and docs/TROUBLESHOOTING.md, which had no entry for either feature and now has one each — including an ordered list of the four reasons a second mix does not arrive, which is a question the engine can answer and the docs could not.

Owner decision, left alone

A failing VOD compile blocks Save on the live mix (RoutingPage.tsx:414). codex called it Major; agy called the same code "working correctly"; the review flagged it as the owner's call. internal/routing/pair.go:73 states the principle — "an optional VOD track must never veto a working broadcast" — and this vetoes a save, not a broadcast, which is defensible but does strand valid live-mix edits behind an optional feature. Not changed here. It is a behaviour decision, not a defect, and it deserves its own change rather than riding along with nine fixes.

Nothing in the review was wrong on inspection

Every finding survived the source. The only place I departed from a suggested remedy is M5 (restore instead of confirm), argued above.


Verification

go build ./..., go vet ./..., go test ./... — all pass, with one pre-existing environmental failure noted below.
cd ui && npm run lint && npm test && npm run build and npx tsc -b — all pass (115 UI tests, 8 files).
cd web && npm run build — passes, including check-build.mjs (6 pages, COMPARISON parity, 0 errors).

The security workflow was red on every push to this branch, and is not any more

Not in the review, found while checking CI. gitleaks scans origin/main..HEAD on a pull request — this branch's own commits, not the tip. 91e0d47 replaced the live_1_abcdefghijklmnop fixture stream key with sk-live-vod, but a value removed in a later commit is still in the earlier one, and a357faf still carries it. That is precisely the property the range scan exists to enforce.

Allowlisted by commit SHA, which is the shape .gitleaks.toml already uses for 5cbe821 and for the reasons documented there. The deciding one here is the path: the file is internal/api/vod_profile_roundtrip_test.go, and that config's own header refuses a blanket rule over test files because "a real key tends to get committed by accident" in exactly that place. A commit SHA exempts one immutable, already-reviewed diff and nothing else.

Verified both directions: the range scan goes from leaks found: 1 to no leaks found, and the workflow's own over-broadness self-test — plant a credential in an allowlisted file, assert generic-api-key still catches it — still passes, so the allowlist is still scoped to values and commits rather than to paths.

Merged main in, because the M7 sweep collided with #364

#364 landed on main touching the same three files (features.astro, comparison.astro, COMPARISON.md) and the PR went CONFLICTING. Resolved toward main in every case, because #364 is more specific and was written against the same sources:

  • The EB paragraph on features.astro. main's "it needs a GPU, and it needs you to say so" is the better sentence — it names the precondition that actually bites, that settings.multitrack.gpus is empty by default so owning the card is not sufficient. Kept, with one clause added: Twitch validates the inventory it is sent rather than the card doing the encoding, which is the mechanism behind the refusal main describes. The note: added here sits beside it untouched.
  • The Metrics / API row. Pure adjacency — my hardware-encoding comment landed immediately above a row main rewrote. main's row kept; the comment moved against the row it is about.
  • COMPARISON.md footnote 4. Both sides claimed the number. main's loudness/metrics correction keeps 4; the hardware-encoding footnote becomes 5, and the table cell with it.

check-build.mjs's row-parity guard passes, which is the check most likely to be broken by a resolution that took one side of these files and not the other.

internal/testenv's TestNoNewBareSkipCanLand fails on this machine and does so identically on the commit before mine (confirmed by stashing). A leftover locked git worktree under .claude/worktrees/ is a second checkout of the repo, and the census walk excludes .git, node_modules, top-level ui/ and web/ but not .claude/ — so every count is doubled and every failure line names a path inside that worktree. Nothing to do with this branch; CI clones fresh. Worth a one-line fix in that walk at some point, but not in this PR.

There is no npm run typecheck script in ui/package.json; tsc -b runs as the first half of npm run build, and I also ran it standalone. Worth noting since the task named that script.

The two Criticals were also verified by running, not by reading — the outputs are quoted in section 4. That is the method the review's own closing note argues for, and it is the only method that would have caught either one.

Mutations run

Each broke the behaviour, watched the named test fail, restored from a file backup (command cp -f), and confirmed git diff empty.

# Mutation Expected to die Result
A existing.VODProfile = nil after the decode in handleUpdateDestination the round-trip test FAILTestASecondMixSurvivesTheRoundTripThroughTheAPI: "the second mix did not survive the save: nothing came back". Also killed the other two (the clear test died at its own setup guard, which says in those words that the clear proves nothing)
B restore the stored pointer after the decode, the way ExtraOutputArgs is the clear test only FAILTestAnExplicitNullClearsTheSecondMix: "switching the second mix off left it in place: {…}". The other two still PASSED, so B is the discriminating mutation
C border-borderborder-amber-500 in Experimental.tsx the raw-signal-colour guard FAILtheme.test.ts > state colour comes from tokens…: components/Experimental.tsx: border-amber-500
D bg-card-raisedbg-card-raisedx in Experimental.tsx the undeclared-token guard FAILtheme.test.ts > no utility refers to a token that does not exist: components/Experimental.tsx:57 bg-card-raisedx
E drop idPrefix={idPrefix} from the TrackRows call tsc FAILTS2741: Property 'idPrefix' is missing … but required in type 'TrackRowsProps'

C and D exist because the new component is the first file added to ui/src/components/ in this change and I wanted evidence the theme guards actually scan it rather than assuming the glob reaches it. Both do.

Mutations run for the review fixes

# Mutation Expected to die Result
F force existing.VODProfile.Mode = routing.ModeMatrix after the decode in handleUpdateDestination the round-trip test FAILTestASecondMixSurvivesTheRoundTripThroughTheAPI: mode came back "matrix", want "simple". This mutation SURVIVED before M6
G walk existing.VODProfile.Tracks and set .Enabled = false, .Track = 0 on each the round-trip test FAIL — two lines: track row 0 came back {Track:0 Enabled:false Gain:-3}… and track row 1 came back {Track:0 Enabled:false Gain:0}, want track 2 enabled at gain 0. Also SURVIVED before M6
H fold the new case provisional arm back into the one above it — drop the mix, set nothing the new engine test FAILTestAVODMixOnAnUnprobedIngestIsNotSentAndSaysSo: "nothing explains why the second mix is not being sent…"

Same discipline as A–E: break it, watch the named test fail, restore from a file backup with command cp -f (never git checkout --), confirm git diff clean. F and G are the two mutations review-365.md recorded as surviving; both now die, which is the point of M6. Each -run pattern was checked to have actually matched — a mistyped name exits 0 printing [no tests to run], which reads exactly like a mutation that survived.


Deliberately not done

  • No feature flag, kill switch, or opt-in gate for either feature. That was the explicit instruction and it is also the right call: the EB fallback is quiet and correct, and the encoder probe already refuses on measured failure.
  • No i18n keys for the experimental copy. It is inline English, following the EB toggle and GPU inventory blocks it sits beside — both of which carry an explicit comment saying a half-translated pair is worse than an untranslated one. English-only keys are safe here (lib/i18n.ts falls back per key) but mixing the two conventions inside one label would be worse than either.
  • No component test for the two-editor interaction. There is no harness for it in this repo, and adding React Testing Library to land one editor is a larger decision than this PR should make on its own. Called out above as the weakest evidence here.
  • No request-token/AbortController on compileRouting. The cancelled flag fixes the observable bug; an AbortController would also stop the wasted request, which is a separate and smaller win.
  • No change to docs/API.md. vodProfile was already documented as an API field; what changed is that it now has a UI, which is recorded in AUDIO-ROUTING.md where the feature is explained.
  • No ConfirmDestructive on the second-mix switch (M5). The review pointed at the component the codebase uses for this class of action; restoring the dropped profile is strictly better, because there is nothing to confirm if nothing is lost.
  • No change to the Save-blocked-by-VOD-compile behaviour (RoutingPage.tsx:414). Explicitly the owner's call in the review, with the two external reviewers disagreeing about it. Section 4 states the argument on both sides; it needs its own change.
  • Nothing done about the review's Minor list beyond what M1–M7 touched: the 180 ms debounce hole, the missing key on ProfileEditor, the duplicated accessible names in TrackRows, MusicRightsCard's copy, the stale multitrack read, <Experimental> on a Twitch destination with no second mix, ctx invalidating at meter rate, and exhaustive-deps not being enforced. Each is real and none is a correctness or honesty defect; batching them into a PR already at nine fixes would make the diff harder to check, not easier.
  • No @testing-library/react harness. Still the review's own "finding that outlives this PR" — six of twelve UI findings would have been caught by a component test and there is no harness. Adding one is a larger decision than this PR should make; it is the highest-value follow-up available here.
  • No fix to internal/testenv's census walk excluding .claude/, described under Verification. Pre-existing, environmental, and unrelated to this branch.
  • Nothing merged, tagged or published.

https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX

…ures had no honesty label

`vodProfile` shipped complete on the server -- column, migration, API,
routing.CompilePair, the engine's gate on Twitch's one-track ingest -- and
appeared in the frontend exactly once, as a type on line 268 of types.ts. An
operator could switch Enhanced Broadcasting on, watch it negotiate, and have no
way to say what the second mix contained.

It is the SAME editor as the live mix rather than a second one written beside
it: both are a routing.Profile, so the block under the destination picker is
now one ProfileEditor rendered twice. Two things had to change to make it
reusable -- DOM ids are namespaced (id="norm" became a duplicate the moment
there were two editors) and each instance compiles itself instead of borrowing
a graph that is not its own.

Off stays the default. On seeds from the live mix. Off sends an explicit null,
because the API decodes over the stored row and an omitted field would leave
the pointer alone.

Alongside it, Twitch Enhanced Broadcasting and hardware encoding are labelled
EXPERIMENTAL in UI, Go doc comments, docs and the changelog. Labelled, not
gated: nothing is hidden, no flag turns either off, and each label names the
specific untested claim rather than saying "beta".

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
…abels

Both reviewers independently found the same two defects; two more came from
codex alone. All four were checked against the source before acting.

1. DUPLICATE DOM IDS SURVIVED THE EXTRACTION. `id="norm"` was namespaced but
   TrackRows was not: it renders `id={`track-${index}`}`, so with two editors
   on the page the VOD editor's `<label for>` resolved to the LIVE editor's
   checkbox. Clicking "Track 1" under the second mix toggled the first mix.
   `idPrefix` is now a required prop, so a third instance cannot inherit the
   collision by omission.

2. A COMPILE RESPONSE COULD WIN OUT OF ORDER. The cleanup cancelled the
   debounce timer but not a request already on the wire, so a slow first
   response landing after a fast second one painted a filter string the
   controls did not describe -- and could call setVodError after the VOD editor
   unmounted, leaving Save disabled by a mix that no longer exists.

3. THE UI CLAIMED A TRACK IT WAS NOT SENDING. Three strings said the second
   track was carried, published and produced, unconditionally -- three lines
   below the alert correctly saying it is NOT being sent when the engine
   refuses the pair at plan time. The switch label now describes the
   CONFIGURATION and the editor footer is conditional on the gate.

4. THE EXPERIMENTAL LABEL CONTRADICTED THE DOC IT WAS ATTACHED TO.
   internal/multitrack said "never against the live endpoint" nine lines above
   "THREE THINGS MEASURED AGAINST THE LIVE ENDPOINT". Both were true of
   different things and the comment did not say which: the observations are
   real captures, what has never happened is a request leaving this process for
   ingest.twitch.tv. Reworded so the boundary is stated once and holds.

   The go-live decision on DestinationCard is also labelled now -- the badge
   alone, not the block, because it renders on every broadcast.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
Copilot AI lite review requested due to automatic review settings August 15, 2026 01:09
… self-test

`gitleaks` went red on #365, and not on the scan -- on the step BEFORE it,
"verify the allowlist is not over-broad". That guard establishes a clean
baseline first, because a non-zero exit from gitleaks means "something was
found", not "the plant was found". With a finding already in the tree it
refuses to run and says so: "the working tree already has findings; this check
cannot prove anything." The scan itself never executed.

The finding was a fixture in the new round-trip test:

    StreamKey: "live_1_abcdefghijklmnop"

Long enough and entropic enough to look like a real Twitch key, next to a
`...Key:` identifier -- which is exactly what arms the generic-api-key rule.
security.yml's own comment describes the mechanism: the identifier name is
load-bearing, and `plantedKey = "<value>"` finds it where
`plantedThing = "<value>"` does not.

Every other stream key in these tests is short and obviously fake --
"original-key", "sk-live-Zq7", "sk-live-old", "key", "sk-live-1". This one
broke that convention, so it is brought back into line rather than exempted.

ALLOWLISTING WOULD HAVE BEEN THE WRONG FIX and .gitleaks.toml says so at the
top of the file: a blanket `paths = ['''_test\.go$''']` "would hide a real
key". The guard that failed exists precisely to stop the allowlist growing
until it covers everything, so widening the allowlist to silence the guard
would defeat the guard.

Verified: `gitleaks detect --no-git --config .gitleaks.toml` goes from 3
findings to 1, and the one that remains is `ui/e2e/.auth/state.json` -- a local
Playwright artifact that is untracked and gitignored, so it does not exist in
CI's fresh checkout. The other two were this fixture and its copy inside a
leftover agent worktree, which has been pruned.

All three tests in the file still pass. Confirmed by their real names after
`-run 'VodProfile'` printed `[no tests to run]` and `ok` -- the failure mode
that makes a mutation read as a pass.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a first-class UI for editing the optional second (VOD) audio mix (vodProfile) by extracting the existing routing editor into a reusable component rendered twice, and establishes a consistent “EXPERIMENTAL” labeling convention across UI, Go docs, project docs, and the changelog for features not yet verified on real hardware.

Changes:

  • Refactors the routing editor into a reusable ProfileEditor and adds a second instance to edit vodProfile, including per-instance compilation and DOM id namespacing.
  • Introduces a shared <Experimental> / <ExperimentalBadge> UI component and applies it to Twitch Enhanced Broadcasting and hardware-encoder flag surfaces.
  • Adds API-level round-trip tests for vodProfile behavior (set, explicit null clear, and “absent field leaves unchanged”) and updates docs/changelog accordingly.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ui/src/pages/SettingsPage.tsx Labels Enhanced Broadcasting hardware inventory as experimental with a clear, non-gating explanation.
ui/src/pages/RoutingPage.tsx Extracts ProfileEditor, adds VOD mix editing with independent compilation and save semantics for explicit null.
ui/src/pages/RenditionsPage.tsx Adds an experimental notice specific to hardware-encoder flags (not probing).
ui/src/components/signature/TrackRows.tsx Requires idPrefix and namespaces checkbox ids to prevent cross-editor label/checkbox collisions.
ui/src/components/Experimental.tsx Adds the shared experimental badge + explanation component used across the UI.
ui/src/components/DestinationDialog.tsx Marks Enhanced Broadcasting as experimental and adds the precise claim about what’s unverified.
ui/src/components/DestinationCard.tsx Ensures the go-live EB decision surface carries the experimental badge (without adding persistent warning text).
internal/multitrack/multitrack.go Documents Enhanced Broadcasting negotiation as experimental with a precise evidence boundary.
internal/engine/multitrack.go Adds experimental caveat emphasizing fallback correctness and lack of live-endpoint observation.
internal/ffmpeg/rendition.go Clarifies that hardware encoder flag sets are experimental (unverified on actual devices).
internal/api/vod_profile_roundtrip_test.go Adds API tests covering vodProfile set/reload, explicit-null clearing, and absence-preserves behavior.
docs/RENDITIONS.md Adds an experimental blockquote about hardware encoder flags not being verified on real hardware.
docs/HARDWARE.md Adds experimental framing and clarifies what has/hasn’t been observed for hardware encoding.
docs/ENCODING.md Marks hardware per-encoder flag rows as experimental and clarifies the evidence behind them.
docs/AUDIO-ROUTING.md Documents the new VOD mix UI and adds the experimental caveat for Twitch negotiation specifically.
CHANGELOG.md Records the new VOD mix UI and the new experimental-labeling convention (labeled, not gated).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…ning the tests disproved

Kept in the repo rather than in a comment because C1 and C2 are the same
mistake -- a label asserting something is untested, believed by every reader
including the one who wrote it, until somebody ran the thing it was about.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
…uite disproves on every run (#365)

`internal/multitrack/multitrack.go:5` said "THIS CODE has never talked to
Twitch". It has. `live_test.go` reaches ingest.twitch.tv on every run and does
not skip: Twitch's refusal arrives verbatim, a negotiation succeeds granting a
VOD audio track alongside one video track, and a 314-character key comes back
minted from the 44 it was sent. Ten places repeated the claim, and it is worse
than wrong -- it tells a maintainer there is no live test, which is the only
canary for Twitch tightening its allowlist.

The boundary, applied at all ten sites: the negotiation runs against Twitch and
succeeds; what has never been observed is a broadcast PUBLISHED through a minted
key. internal/engine's wiring -- everything after Negotiate returns -- has only
ever been driven by an httptest server, so engine/multitrack.go's own note is
re-aimed at itself rather than citing the package doc it inherited the error
from.

Stated once, where an operator can act on it: Twitch validates the DECLARED
inventory, not the hardware. live_test.go declares an RTX 3080 and Twitch grants
it, on an Apple M1 Ultra. That is the mechanism behind ENCODING.md's existing
"a gate, not a workload", and it is why Settings asks for an inventory instead
of reading one.

The hardware label was false for VideoToolbox in the same way and for the same
reason -- nobody ran it. TestEveryConfiguredEncoderOpensWithItsOwnFlags encodes
per registered encoder with that encoder's own flags including the capped-VBR
path, and h264_videotoolbox and hevc_videotoolbox pass. Narrowed to NVENC / QSV
/ VA-API / AMF at all five sites; the UI badge was gated on `encoder.hardware`
and is now gated on the encoder family, so the one hardware encoder that IS
confirmed no longer carries the warning. That test is the strongest evidence
those flags have and it went unmentioned beside the table it answers for.

Also in the editor:

- applyPresetTo awaited and applied unconditionally. Click a VOD preset, switch
  the second mix off before the answer lands, and it came back on with
  setDirty(true) -- Save then persisted a profile where null was intended,
  defeating the guarantee this PR exists to make. Now guarded on the
  destination, on the mix still being enabled, and on a newer click.
- The same function stopped setting compiled/compileError, so a preset that
  fixes an invalid profile left the Result card showing the old graph, old
  warnings and old red error with Save disabled for 180 ms + RTT. It now returns
  the routing to the editor that asked, which keeps `compiled` owned in one
  place rather than copied into the page.
- The second-mix footer claimed the track IS produced on the Twitch path, where
  the negotiation decides -- contradicting SecondMixCard's hedge two cards
  above. The unconditional sentence is now reserved for a probed ingest off
  Twitch.
- `provisional` drops the second mix on EVERY platform and set no vodDropped, so
  nothing reached the card. Fixed in the engine (noteVODProvisional) and gated
  in the UI on ctx.probed, which the page already had.
- Switching the second mix off destroyed its configuration with no undo. The
  last non-null profile is held and restored; the live mix seeds only when there
  is nothing to restore, and the stash clears on a destination change.

The round-trip test asserted len(Tracks) and Gain[0] and nothing else, so
forcing ModeMatrix and zeroing every Tracks[i].Enabled/Track both survived. Both
now die.

Label coverage reaches the surfaces a user actually meets first: features.astro
(where EB was described as working in three unqualified sentences), the two
comparison tables and their footnotes in both copies, ENCODING.md, and
TROUBLESHOOTING.md, which had no entry for either feature and now has one per
feature -- including the ordered list of four reasons a second mix does not
arrive, which is the question the engine can answer and the docs could not.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
…ommit that added it (#365)

The `security` workflow has been red on every push to this branch. 91e0d47
replaced `live_1_abcdefghijklmnop` in vod_profile_roundtrip_test.go with
`sk-live-vod`, matching the deliberately unconvincing keys the rest of that file
uses -- but the PR scan reads `origin/main..HEAD`, not the tip, and a value
removed in a later commit is still in the earlier one. a357faf still carries it.
That is exactly the property the range scan exists to enforce and not one worth
weakening.

Allowlisted BY COMMIT, which is the shape .gitleaks.toml already uses for
5cbe821 and for the same reasons stated there. The one that decides it here is
the path: `internal/api/vod_profile_roundtrip_test.go` is a test file, and this
config's own header refuses a blanket rule over test files because "a real key
tends to get committed by accident" in exactly that place. A commit SHA exempts
one immutable, already-reviewed diff and nothing else -- the same string is
still caught anywhere it appears again, and a real key added to that file
tomorrow is still a finding.

Verified: `gitleaks detect --config .gitleaks.toml --log-opts origin/main..HEAD`
goes from "leaks found: 1" to "no leaks found", and the workflow's own
over-broadness self-test still detects a credential planted in an allowlisted
file, so the allowlist is still scoped to values and commits rather than to
paths.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
#364 landed on main touching the same three files this branch's label sweep
touched, so the PR went CONFLICTING. Resolved toward main in every case, because
#364 is more specific and was written against the same sources:

- features.astro, the EB paragraph. main's "it needs a GPU, and it needs you to
  say so" is the better sentence -- it names the precondition that actually bites
  (settings.multitrack.gpus is empty by default, so owning the card is not
  sufficient). Kept, with one clause added: Twitch validates the inventory it is
  SENT rather than the card doing the encoding, which is the mechanism behind the
  refusal main describes. The EXPERIMENTAL note added here sits beside it
  untouched.
- comparison.astro, the Metrics / API row. Pure adjacency -- my hardware-encoding
  comment landed immediately above a row main rewrote. Main's row kept, comment
  moved to sit against the row it is about.
- COMPARISON.md, footnote 4. Both sides claimed the number. Main's loudness and
  metrics correction keeps 4; the hardware-encoding footnote becomes 5, and the
  table cell with it.

web build passes, including check-build.mjs's row-parity guard between the site
and docs/COMPARISON.md -- which is the check most likely to be broken by a
resolution that took one side of these files and not the other.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
Both branches appended to [Unreleased]: #366 a Security section for the seven
audit findings, this branch an Added section for the VOD editor and a Fixed
section for the review's majors. Kept both, Security first, matching how every
released section in this file is ordered.

Verified nothing was dropped. Three lines from main are absent and all three
are deliberate: they are the Enhanced Broadcasting and capped-VBR entries this
branch REWORDED as its C1/C2 fix, because they carried the claim that the
negotiation had never run against Twitch -- which the live test disproves on
every run. The corrected wording is present in both places and says what is
actually unobserved: a broadcast published through a minted key.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
@sonarqubecloud

Copy link
Copy Markdown

@rainmanjam
rainmanjam merged commit 21d0a0c into main Aug 15, 2026
29 checks passed
@rainmanjam
rainmanjam deleted the feat/vod-profile-editor-and-experimental-labels branch August 15, 2026 03:10
rainmanjam added a commit that referenced this pull request Aug 15, 2026
…ft guards (#368)

SECOND TIME TODAY, and that is the finding rather than the fix. [Unreleased]
sits ABOVE [0.7.0], so under Keep a Changelog its contents are NEWER than the
release below them -- and every pull request correctly appended there. Nothing
folds it forward, so the gap reopens after each merge and is only ever caught
by somebody reading the file.

Stranded this time: all seven security fixes from #366, the VOD editor and the
EXPERIMENTAL convention from #365. Nine entries.

THE SECURITY FIXES ARE WHY THIS IS NOT COSMETIC. GHSA-7jqx-76vq-hvfc points at
these release notes, and the fix for its worst defect -- 0.7.0's own seal-at-
rest migration leaving plaintext stream keys legible in the WAL, which is true
on every upgraded install and needs no attacker -- was in a section a tag would
not have included. An operator following the advisory to the notes would not
have found it.

#367 had no entry at all. Added under Testing, and written to say what actually
justifies those ten guards: not that they pass, but that each was watched to
fail against the defect it names, and that two real defects were found in the
recovered code before it landed -- a comment-defeat where deleting the feature
and leaving its text in a `// was:` comment kept the test green, and an
unguarded strings.Index that turned a rename into a slice-bounds panic.

Verified the same way as the first fold: the prose is MOVED, not retyped. Zero
lines of the previous file are absent from this one, 0.6.0 and older are
byte-identical, [Unreleased] is empty, 90 entries in 0.7.0 against 80 before,
no malformed links, no conflict markers.

WORTH FIXING PROPERLY: this fold belongs in the release procedure, not in
review. Twice in one day is a process gap, not two mistakes.

Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants