fix(hyperframes-media): surface the real reason a TTS line failed [P2]#1999
Conversation
9fef490 to
7e4e2dd
Compare
terencecho
left a comment
There was a problem hiding this comment.
LGTM on the code — needs a rebase (currently dirty) and a CodeQL triage before merge.
Cross-checks
- Secret leak grep.
heygenJSONthrow paths only include the truncated response body (detail.slice(0, 300)), NOT request headers → no API-key / OAuth-token leak viae.message. The presignedaudio_urlalso isn't included in the fetch-error path (only HTTP status). - Test-actually-exercises the failure paths. All 4 new tests hit distinct branches (thrown catch, non-ok fetch, missing
audio_url, non-zero subprocess exit); assertions check error content, not justok:false. - Regression-safe shape.
synthResultreturns{ok:true, words:null}on success (matches old shape);depsarg defaults to real impls;audio.mjshandles a missingerrorfield (${error ?(${error}): ""}) so an older caller / stubbed dep won't NPE. - Diff-scope. Base
80271863= realorigin/mainmerge-base; 4 files, +94/-17; no stack drift.
Findings
- [note / not a merge-blocker] CI CodeQL flagged 3 new alerts — 1 high "Insecure temporary file" @
tts.mjs:320(writeFileSync(wavAbs, bytes)) and 2 medium "Network data written to file" @ 203 / 292. Line 320'swriteFileSyncalready existed pre-PR (the.mp3else-branch); alerts likely surface now becausesynthesizeHeygenis newly exported and reachable from tests. Overall CI conclusion is success (CodeQL is advisory), but worth Miguel triaging the dashboard. - [nit]
tts.mjs:315—"wav transcode failed (ffmpeg — see output above)"implies stderr is above, buttranscodeToWavcalls ffmpeg withstdio: "ignore"(line 205), so no ffmpeg output actually appears anywhere. Consider dropping the "see output above" clause or capturing stderr through the same shape as the new error surfacing. - [nit] No test for the
wav-transcode-fail path (the injectedtranscodeToWavreturning false). Trivial to add given the deps injection. - [blocker before merge]
mergeStateStatus: DIRTY— has merge conflicts against main. Needs rebase.
— Review by tai (pr-review)
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: 🟢 LGTM at 7e4e2dd
Concurring with @terencecho on the code. Fix is right-layer: synthesizeHeygen()'s bare catch {} and three { ok:false } short-circuits (missing audio_url, non-ok audio fetch, failed transcode) all now propagate { error } strings, and audio.mjs:158 appends them to the anomaly text. The deps injectable arg is scoped to failure-path testability with real network/ffmpeg impls as default. synthResult unifies the subprocess providers so per-provider failures name why (missing wav, non-zero exit, spawn error).
Endorsing Terence's secret-leak check — heygenJSON truncates error bodies to 300 chars via detail.slice(0,300), no request headers or presigned URLs in the propagated error, so no OAuth/API-key surface in the CLI diagnostic log. HeyGen's /voices/speech error bodies carry HTTP status + code (plan_upgrade_required etc.), not account identifiers.
Ship-blocker to flag: PR is currently mergeable state DIRTY per gh pr view — needs a rebase before merge. Terence also flagged CodeQL triage.
R1 by Via
synthesizeHeygen() swallowed every failure into a bare { ok:false }: a thrown
HTTP error (e.g. 402 plan_upgrade_required from heygenJSON) was caught and
discarded, a missing audio_url / failed audio fetch / failed transcode all
returned nothing. audio.mjs then logged 'TTS failed — omitted' for every line
with zero detail, so the actual cause took a hand-rolled repro to find.
Each failure path now returns an { error } string (the caught message, the HTTP
status, or the specific stage that failed), and audio.mjs appends it to the
anomaly. The subprocess providers (elevenlabs/kokoro) get the same treatment via
a shared synthResult() helper. synthesizeHeygen takes an injectable deps arg so
the failure paths are unit-tested (thrown 402, non-ok fetch, missing audio_url).
7e4e2dd to
52a4335
Compare
Problem
synthesizeHeygen()swallowed every failure into a bare{ ok: false }:heygenJSON) was caught and discarded by a barecatch {},audio_url, a failed audio fetch, and a failed transcode each returned{ ok:false }with no reason.audio.mjsthen loggedline N: TTS failed — omittedfor every line with zero diagnostic detail, so the actual cause (a plan/billing error, an auth problem, etc.) took a hand-rolled repro script to find. Reported from real usage: "surface e.message in the anomaly text."Fix
Every failure path now returns an
{ error }string stating why — the caught exception message, the HTTP status, the missingaudio_url, or the failed transcode stage — andaudio.mjsappends it to the anomaly (TTS failed — omitted (<error>)). The subprocess providers (elevenlabs / kokoro) get the same treatment via a sharedsynthResult()helper (naming the non-zero exit or the missing wav).synthesizeHeygentakes an optional injectabledepsarg (defaults to the real network/ffmpeg impls) purely so the failure paths are unit-testable.Verification
tts.test.mjs(node:test) — 4 new tests, 9 total pass: a thrown 402 surfaces inerror(contains 402 + plan_upgrade_required); a non-ok audio fetch surfaces its HTTP status; a missingaudio_urlis named;synthResultnames a non-zero subprocess exit. No behavior change on the success path.