Fix test-media.js loudness measurement + confirm test-posthog-funnel was a local process fluke - #801
Merged
Merged
Conversation
… and its direction assumption was wrong
Two real bugs, unrelated to the CSP work, found while investigating the
"pre-existing" test-media.js failure flagged after that PR shipped.
1. measureLufs() used execFileSync, which only exposes captured stderr
via a thrown Error's .stderr field - but `ffmpeg ... -f null -` with
the ebur128 filter exits 0 on success (verified consistently on this
machine's ffmpeg build), so the try/catch never fired and the
function silently returned null on every successful measurement, for
both the before and after readings. Only the "after" reading was ever
asserted non-null, so the loudness proof this test exists for had
never actually run. Fixed by switching to spawnSync, which returns
{stdout, stderr, status} regardless of exit code.
2. Once real measurements were flowing, a second bug surfaced: the test
asserted normalization must make the audio QUIETER (afterLufs must be
< beforeLufs), on the assumption a default-amplitude sine wave always
measures around -3 LUFS ("very loud"). ffmpeg's lavfi sine source has
no amplitude parameter (verified via -h filter=sine) and its actual
default output measures -21.8 LUFS on this build - quieter than the
-16 LUFS target, so normalizing UP to target correctly makes it
LOUDER, the opposite of what the old assertion required. Replaced the
directional assumption with the real, portable claim: normalization
moves loudness closer to the target than the input was, regardless of
which side of the target the raw input happens to fall on.
Verified: passes consistently across repeated runs: real measurements
now visible in the output (input -21.8 LUFS -> output -16.2 LUFS,
target -16). Mutation-tested by temporarily disabling the loudnorm
filter in src/tools/media-kit.js (swapped for a no-op volume filter) -
confirmed the test correctly fails with "did NOT hit target", then
reverted (clean, zero diff on the source file). Checked every other
execFileSync use in scripts/ for the same bug shape (relying on stderr
from a caught exception rather than the success return value) - none of
the other four call sites have it; all read stdout from the normal
return path.
Also traced the earlier "test-posthog-funnel.js has 8 pre-existing
failures" finding to its root cause: a stray local server process left
bound to port 3081 from earlier work in the same session. Killed the
stray process; the test now passes cleanly and consistently (53/53).
No code changes were needed there - the earlier "confirmed pre-existing
via git stash" conclusion was itself a false negative, since a stashed
git diff does not affect an already-running background process bound to
the same port both runs happened to hit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MikeyPetrillo
temporarily deployed
to
agent402 / production
August 17, 2026 01:26 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
MikeyPetrillo
marked this pull request as ready for review
August 17, 2026 01:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #799, fixing the two test issues flagged as "pre-existing" after that PR shipped.
test-media.js— real bug, fixed.measureLufs()usedexecFileSync, which only exposes captured stderr via a thrownError's.stderrfield — butffmpeg ... -f null -with theebur128filter exits 0 on success, so the try/catch never fired and the function silently returnednullon every successful measurement. The loudness proof this test exists for had never actually run. Fixed by switching tospawnSync(returns{stdout, stderr, status}regardless of exit code). Once real measurements were flowing, a second bug surfaced: the test assumed a default-amplitude sine wave always measures around -3 LUFS ("very loud"), but ffmpeg'ssinesource has no amplitude parameter and this build's default output measures -21.8 LUFS — quieter than the -16 LUFS target, so normalizing up to target correctly makes it louder. Replaced the directional assumption with the portable claim: normalization moves loudness closer to the target than the input was, regardless of which side of the target the input falls on.test-posthog-funnel.js— false alarm, no code change. Traced to a stray local server process left bound to port 3081 from earlier work in the same session (the same process that also causedtest-leaderboard-envelope.jsto fail during a full local sweep). Killed the process; the test now passes cleanly and consistently (53/53). The earlier "confirmed pre-existing viagit stash" conclusion was itself a false negative — stashing the working tree doesn't affect an already-running background process bound to the same port.Test plan
test-media.jspasses consistently across repeated runs, with real measurements visible (input -21.8 LUFS → output -16.2 LUFS, target -16)loudnormfilter insrc/tools/media-kit.js(swapped for a no-op), confirmed the test correctly fails with "did NOT hit target", then reverted (clean, zero diff)execFileSynccall sites inscripts/for the same bug shape (relying on stderr from a caught exception rather than the success return value) — none of the other four have ittest-posthog-funnel.js53/53 passing after killing the stray processtest-ci-coverage.jsclean (4/4)🤖 Generated with Claude Code