Review caught that the same anti-pattern was still live in the Studio binding:
canaryEventProperties destructured only `enabled` and dropped the reason. Its
own doc comment promised 'identical shape to the CLI, so a rollout spanning
both reads as one flag' — which the CLI-only fix had just made false.
This matters beyond symmetry. A CLI-launched Studio adopts the CLI's decisions
and shares its bucket seed, so a cohort flip can surface on either surface.
Emitting attribution on only one leaves Studio-observed flips unattributable
and makes the two flip counts irreconcilable — and Studio is the surface most
likely to expose a shared-seed-with-diverging-id pattern, which is the open
question the reason exists to answer.
Also adds the no_unit_id emission test the CLI side advertised but never
asserted, and a Studio pair pinning that a URL override and a cohort roll
produce the same assignment with different reasons.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The calibration contract deferred this until the stability check came back
dirty. It did: the first fleet read found 304 installs (1.08%) reporting both
values for a canary whose percentage never moved, and the genuinely anomalous
ones could not be separated from a developer toggling HF_CANARY_*, because the
assignment alone is identical in both cases.
resolveCanary has always computed the reason and canaryEventProperties dropped
it. Now every canary emits canary_reason_<name> beside its assignment.
Deliberately outside the $feature/ namespace: PostHog treats those as flag
values, and a non-boolean there would corrupt the flag's own breakdowns.
Two of the six wire values are immediately useful beyond override attribution.
'excluded' identifies CI installs, which today have to be dropped by joining
on is_ci — conflating them with out_of_cohort is what made the first accuracy
read look like a significant failure (9.22% against a 10% target) when it was
not. 'no_unit_id' surfaces the fails-closed corner.
The reason is optional on the core helper so existing callers are unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(cli): classify identity persistence on every telemetry event
Install-grain metrics currently trust every anonymousId equally, but
ephemeral/isolated-HOME workloads mint a fresh id per run — one machine
produced 2,956 rotating render identities since Jul 30 (94.4% seen on a
single render command), inflating acquisition and diluting per-install
penetration while looking like real product usage.
Every event now carries:
- identity_persistence: durable (id loaded from a preexisting config —
proven to survive a process boundary) | unknown (minted+persisted this
run; an ephemeral HOME is indistinguishable from a genuine first run
from inside one process) | process_only (persist failed). Sticky per
process so a fresh install re-reading its own write cannot self-promote.
- config_write_outcome: ok | ok_unmirrored | failed for the identity-
establishing write; absent when the id came from disk.
- invocation_id: random uuid per CLI process, so one invocation's events
group even when the install identity is untrustworthy (unlike run_id,
which needs an orchestrator to set HYPERFRAMES_RUN_ID).
Install metrics can then count only durable identities, and a daily
churn monitor can alert on the unknown share.
* fix(cli): require the anonymousId to come off disk before classifying durable
Review finding: materializeConfig mints a replacement anonymousId when a
hand-edited/image-baked config lacks one. That replacement only reaches
disk when the bucket-seed backfill happens to write; with a seed present
the read path performs no write at all, so the install re-mints a fresh
id every run while the unconditional durable branch stamped each of them
with the one label durable-only counting is allowed to trust.
durable now requires parseNonEmptyString(parsed.anonymousId): a minted
replacement classifies like a fresh mint — by the backfill write outcome
when that path runs (unknown/process_only), and process_only on the
no-write path where the id provably dies with the process. Two tests pin
both shapes.
carriedSectionsFrom() decided whether a ## Usage section was generated by matching
its first line against a list of historical opener phrases — so a hand-written
Usage section that happened to open that way was classified as generated and
silently deleted on regeneration. Ownership is now purely set membership: a
section is generated iff its heading is one the template emits, and ambiguous
'usage' is no longer in that set (the template never emits it), so any ## Usage is
carried. Exported carriedSectionsFrom behind an entrypoint guard and added two
executable preservation fixtures. Flagged by Magi (#5).
- edit-operations: dispatch() does not consult can() (session.ts:608 -> applyOp
with no validation; a no-timeline addGsapTween/addLabel is a no-op but a missing
target still writes via selector fallback). Use Rames's wording: call can() first
and skip on failure — no false 'applies nothing' guarantee.
- timing-and-animation: the second E_NO_GSAP_TIMELINE site — gated setGsapTween on
an error it cannot return and called shipped parser code 'a later phase'. Rewrote
to gate addGsapTween (which can return it); dropped the stale can() comment in
types.ts:595.
- html-schema: data-playback-start is read by runtime, Studio and CLI (Studio also
writes it, timelineEditingHelpers.ts); only the compile path is media-start only.
Document the layered precedence instead of calling it runtime-only.
Round-2 findings from Magi (#2/#3/#4) and Rames.
My generator rebuild replaced the '## Related topics' section (still required by
docs/AGENTS.md) with the provenance footer, dropping it from all 168 generated
Catalog pages. Emit it again as the final section so pages end with it, and stop
carriedSectionsFrom() breaking at the footer marker so a human section appended
below the generated tail survives a regeneration. Adds a per-page regeneration
assertion so a future drop fails CI. Flagged by Magi (P1) and Rames.
A source that answers with a JSON error body still reached ffprobe and
produced `moov atom not found`. Replicate returns
`{"detail": "requested file not found"}` for a dead asset, and a gateway
in front of it can relay that body with a success status.
The sniff now treats `<`, `{`, or `[` as the opening byte of a text
document. No supported container starts with any of them, so this is the
same trade as before: three bytes instead of an allowlist that grows one
entry per payload shape observed in production.
Renamed accordingly, since the class now covers JSON as well as markup:
MARKUP_NOT_MEDIA -> NOT_MEDIA_PAYLOAD, MarkupNotMediaError ->
NotMediaPayloadError, markupPayload.ts -> notMediaPayload.ts. Registry
entries in the Lambda name map, the CDK and SAM plan lists, the Cloud Run
set, and SAFE_RENDER_ERROR_CODES move with it.
Also documents the reachability boundary on the error class: only a 2xx
response gets here. `downloadToTemp` rejects 404/410 as `http_not_found`
before writing a byte, and every ffprobe input is local because
videoFrameExtractor downloads http srcs first. So the shapes this
classifies are soft-404 and interstitial HTML, S3/CloudFront error
documents, and JSON API error bodies -- each served with a success
status. A genuine 404 surfaces as a download failure, not as this error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes#3012
## What
Fixes `HeadlessExperimental.beginFrame` hanging at 60fps and other high frame rates.
## Why
Warmup always advances Chrome in 33ms steps, but the first capture timestamp was calculated from the output fps.
At 60fps, the last warmup tick is 1947ms, while the next commit tick used to jump back to about 1067ms. Chrome sees time moving backwards and `beginFrame` can stall.
## How
Keep the old timestamps when they are already safe. If the commit tick would go backwards, move the capture baseline past warmup first.
The init commit and both producer probes now use the same timestamp helpers, so they cannot calculate different values.
## Test plan
- [x] Added tests for 24, 30, 31, 32, 33, 60, 120, 240, and 59.94fps
- [x] Added coverage for the actual session baseline, commit parameters, and both producer probe paths
- [x] Engine tests: 1383 passed, 3 skipped
- [x] Producer unit tests passed
- [x] Engine and producer typechecks and builds passed
- [ ] Manual render test
Review follow-up on the STUDIO-5433 defense.
Correctness
- The sniff ran above the documented video/audio failure split, so an
<audio> src that resolved to an HTML payload aborted the whole render
instead of degrading to duration 0. It now runs inside the same try, so
video surfaces the typed error while audio still drops out, with a
warning naming the element.
- Raw fs errors (EISDIR on a directory src, EACCES, the existsSync->open
ENOENT race, EMFILE) escaped and failed the compile with an unclassified
error carrying an unredacted temp path. The sniff is now a classifier that
never throws: an unreadable file reports "not markup" and the real probe
produces the real error.
- Elements whose duration the compiler never resolves (a data-end video, a
looping audio) skipped the sniff entirely, so the original ffprobe error
still escaped, and looping audio was reported as owner "system" after
every frame had been captured. Video is now caught in the asset preflight,
which sees every local src regardless of authored timing; audio is
classified per-element in audioMixer as source/invalid_media/owner "user",
keeping audio failures non-fatal as they already were.
- Detection is a byte-level check for a leading "<" (BOM-, whitespace- and
NUL-tolerant, looped read) instead of a <!doctype|<html|<?xml string
prefix, which missed a NUL-prefixed payload, >256B of leading whitespace,
UTF-16-encoded HTML, and a prolog-less <svg. No supported container starts
with "<", so the allowlist no longer grows per payload shape.
- finally { await fh.close() } could replace the in-flight typed error with
the close error.
Routing and privacy
- MARKUP_NOT_MEDIA is now in SAFE_RENDER_ERROR_CODES, the Lambda terminal
name map, the CDK and SAM non-retryable plan lists, and the Cloud Run
non-retryable set, and the class carries owner/retryable. Previously the
API emitted errorCode: undefined and a deterministic authoring bug burned
the full distributed retry budget.
- The message no longer carries 32 raw payload bytes or the src.
redactTelemetryString preserves host and path for HTTP srcs, so
per-tenant CDN paths reached a message the server forwards to clients.
Correlation is a sha256 element fingerprint, matching
AssetMediaTypeMismatchError.
- The message names both causes (unresolved nested-composition URL, or an
HTML/XML error page served as 200) rather than misdiagnosing an S3 403
body as an authoring bug.
Tests
- Byte-level detection is unit-tested in engine: markup shapes, BOMs,
UTF-16, nine container signatures, unreadable inputs.
- Replaced the tautological assertions. The old checks for "html" in and
"moov" absent from a fixed message template could not fail for any input.
- New coverage for audio degradation, the audioMixer classification, the
preflight video/image/audio split, and the API error metadata.
- The sibling htmlCompiler.mediaType failure was a vitest-under-bun runner
mismatch, not a missing ffmpeg binary. It passes, including the 4-wide
probe-semaphore invariant the sniff now runs inside.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
STUDIO-5433 defense: when the downloaded media file begins with
<!DOCTYPE, <html, or <?xml, throw a typed HtmlNotVideoError naming
the offending src instead of letting ffprobe emit an inscrutable
moov-atom-not-found on a plain HTML page.
Complements #3033 diagnosability layer. Root-cause EF fix ships
separately.
Signed-off-by: Via <vance@heygen.com>
A probe that could not run is no evidence about the GPU, so pointing the
operator at GPU passthrough hid broken Chrome installs behind a phantom
problem. Carry a cause off the probe and emit the matching remediation.
Also un-exports buildUnverifiedHardwareGpuWarning (Fallow: engine test
files are not audit entry points, so a test-only import would not have
counted as a consumer) and covers the non-linux branch via the spy.
Chrome's hardware GL args are advisory: with no usable GPU it silently
falls back to software WebGL and the capture runs at CPU speed. Run the
existing WebGL probe for explicit hardware mode too and warn loudly with
the platform's remediation. The requested mode is still honoured.
Fixes#2967
Three R3 findings.
The redactor's segment classes were ASCII `\w`, so `/数据/客户/秘密视频.mp4` and
`/data/客户/secret.mp4` went out verbatim — and the generic redactor also feeds
CLI telemetry and producer observation messages, where no known-path list
compensates. Segments are now defined by their delimiters instead of an
alphabet, which is correct for every script by construction rather than
requiring Unicode classes to be kept correct. The bare-relative lookbehind had
the same ASCII assumption and let a match start mid-token, redacting
`客户/秘密/视频.mp4` to `客户[path]`; it is now a token boundary, and
bare-relative runs before absolute so it claims the whole token.
sanitizeProbeFailure cast the rejection reason to Error and read `.message`.
An injected probe can reject with anything, so `Promise.reject("failed")` gave
`undefined` and threw inside the redactor — converting a returned failure
result into a rejected promise. Normalized at the boundary, and
redactKnownPaths no longer throws on a non-string.
The contract only admitted .ts/.js/.mjs/.cjs, so it missed shipped shell and
Python callers. frame_strip.sh passed a user-controlled path as ffprobe's last
positional with no terminator; render-and-composite.sh had four more. Both
fixed, and the sweep now covers .py/.sh. Python list argvs are bracket
literals so they get the same position check; shell command lines get a
separate presence check, because checking position there needs a shell parser
— stated as the weaker guarantee it is rather than implied to be equal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeQL flagged js/redos on the all-literal argv matcher. It was right: the
`(?:"[^"]*"\s*,?\s*)+` form nests a quantifier inside a quantifier with an
optional separator, so whitespace can be matched two ways and a long
non-matching argv backtracks exponentially.
Replaced with a linear scan — find the spawn head, slice to the closing
bracket, and check the entries — plus small named helpers. Same behaviour: an
all-literal argv is treated as taking no input, an argv with a bare identifier
still has to be understood (verified by adding one and watching the guard fail).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI runs the PR merged with main, so it saw a caller my branch predated:
`spawnSync("ffprobe", ["-version"])` in engine/src/utils/ffprobe.test.ts. That
is a capability check with no runtime path, so there is nothing to terminate,
but the unclassified guard flagged it as a caller it could not parse.
An argv whose entries are all string literals carries no input by
construction. Those are dropped before the check; an argv with a bare
identifier still has to be understood, verified by adding one and watching the
guard fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract test only walked packages/*/src and only .ts, so it could not see
the shipped agent tools under skills/**, which are .mjs/.cjs. 19 call sites
there and in package tests were still missing `--` immediately before the
input while the suite reported the bug class closed — a dash-prefixed filename
is parsed as an option and fails the same way.
Sweeps packages/, skills/ and scripts/ now, including .mjs/.cjs and test
files (dither.test.mjs was one of the broken sites). Excludes only the
contract test itself, which documents the contract with example argvs
including a deliberately misordered one.
Two guards were fixed while widening: the terminator must never be inserted
after `-i`, which consumes the next token (a blind pass hit an ffmpeg input
and a base64 -i), and comment prose describing a spawn is not a spawn.
Also routes every audioPadTrim probe failure through one sanitizer at the
boundary. runFfprobeJson scrubbed its own stderr, but
defaultProbeVideoFrameInfo threw `no video stream in ${videoPath}` raw into
the public PadTrimAudioResult.error, and an injected probe can throw anything.
The redaction unit tests all passed with the caller wiring deleted; the new
public-path regressions fail without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generic scrub still missed a relative path with no `./` prefix:
`customer/acme-secret/video.mp4` and `assets/bgm.mp3` reached telemetry
completely unredacted, because the absolute rule needs a leading slash and
the `./` rule needs the dot. Adds a rule for them that still leaves `N/A`,
`24/1` and `48000/1001` alone.
Shape matching is a net with holes by construction, so audioPadTrim now
also redacts the exact path it put in the argv, plus its basename, before
the generic scrub runs. It built the argv, so it does not have to guess.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
redactTelemetryString enumerated roots — /Users, /home, /opt, /tmp and a
handful more — so a project on /data, /Volumes, an NFS mount or any root a
user invented reached telemetry verbatim. Relative paths and bare basenames
were never redacted at all, and audioPadTrim routes raw ffprobe stderr
through this on every probe failure.
Now redacts by shape: absolute paths under any root (two or more segments,
so N/A and a 24/1 frame rate are not mistaken for one), relative paths
including dash-prefixed ones, and bare basenames with an asset extension.
URLs still keep their host and drop only the query.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prior contract test scanned a hardcoded file list for a format flag
followed by a bare identifier, so it only matched the shape it was written
against. Mutation testing showed removing `--` from engine/utils/ffprobe.ts
and cli/commands/init.ts did not fail it.
Now walks packages/*/src and finds callers itself, checks that `--` is
immediately BEFORE the input rather than merely present, and compares
discovery against a manifest so a regex regression cannot silently stop
checking a known caller. A separate guard fails on any file that spawns a
probe binary but builds an argv this test cannot parse.
All 11 seams mutation-tested for both removal and misordering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit claimed "all nine now terminate their options". That
was false: `producer/src/utils/audioRegression.ts:307` still passed the
path bare, and it is production source used by the regression harness.
A repo-wide audit found two more in studio-server
(`mediaValidation.ts`, `mediaMetadata.ts`) — their current callers pass
absolute paths, so they were defence-in-depth rather than live bugs, but
the exhaustiveness claim should be true rather than narrowed.
Eleven sites total, all terminated.
Adds a SOURCE-level contract test, which is the gap that let this
happen twice. #2740 fixed one of ten sites and shipped a regression
asserting the argv of that single site, so CI reported the class closed
while nine invocations still parsed `-intro.mp4` as an option. A
per-site unit test has the same blind spot for site twelve; scanning the
tree does not. The test also asserts its own coverage list has not
shrunk.
Verification: engine 1300, lint 511, core 1431, studio-server 398, cli
init/webmAlphaCheck/whisper 146, producer utils 51, audioPadTrim 18.
Removing any single terminator fails the contract test by name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2740 added `--` to one of nine independent ffprobe invocations, so the
bug class it closed stayed open everywhere else while CI reported it
fixed — the regression test asserts the argv of that single site.
Reproduced on ffprobe 8.1.1: an asset named `-intro.mp4` probes fine
through extractMediaMetadata but fails with "Missing argument for option
'intro.mp4'" in audio pad/trim (mid-render), `hyperframes init`, whisper
duration probing and webmAlphaCheck. hevcPreviewLint catches and returns
false, so a dash-prefixed HEVC preview silently passes the lint rule.
Terminated at all of them:
producer/services/render/audioPadTrim.ts (x2)
producer/plan-parity-analysis.ts
cli/commands/init.ts
cli/utils/webmAlphaCheck.ts
cli/whisper/transcribe.ts (x2)
core/mediaGradeAnalyzer.ts
lint/hevcPreviewLint.ts
audioPadTrim's runFfprobeJson is a near-verbatim clone of the engine's
runFfprobe and structurally cannot add the terminator itself, because
callers bake the input path into `args`. It now asserts the terminator
is present rather than letting a dash-prefixed path through, takes the
same stdio ["ignore", ...] as the engine helper, and redacts its stderr
— it was throwing raw ffprobe output, which echoes the input path, into
logs and telemetry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Vance Ingalls