fix(producer): accept partial color metadata in plan v2 - #2814
Conversation
|
Dev rollout confirmation + one adjacent edge case: I reproduced this exact sentinel failure through an ordinary One adjacent gap worth covering before merge: when Local synthetic proof of that strengthened shape is green on the Plan v2 + PLAN_TOO_LARGE suites; the exact-mode failure itself is already covered by this PR and its live GCP smoke. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Found one blocking contract gap at exact head 01b448ce128841593aea75aac59131a0f6610e71.
- [P2] Validate and omit
.hf-completein the full-source fallback too —packages/producer/src/services/distributed/planV2.ts:505. Whenmeta/videos.jsonis absent,buildVideoChunkDependencies()returnsdependencies: nullbeforelistVideoFramePaths()runs. The publication loop then classifies everyvideo-frames/**file aschunks: "all", so a valid.hf-completemarker is published to every chunk and a non-empty marker bypasses the new zero-byte validation entirely. This makes sentinel handling depend on metadata mode even though the extraction-cache ownership marker exists independently of that metadata. Validate the exactvideo-frames/<videoId>/.hf-completepath during publication in both modes, omit only a zero-byte regular marker, and add a metadata-absent regression; keep unknown files fail-closed/over-included per the existing policy.
The partial-color parser change itself is well scoped: only the three ffprobe-normalized color components accept empty strings, while all other string fields retain the non-empty contract. Exact-mode sentinel validation and the focused malformed-input tests are also solid. Core/Windows checks are green; regression shards are still running and have no failures.
— Magi
Verdict: REQUEST CHANGES
Reasoning: The partial-color fix is correct, but Plan v2 still bypasses the sentinel validation/omission contract in its documented full-source fallback.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 01b448ce128841593aea75aac59131a0f6610e71.
Two adjacent Plan v2 issues cleanly rolled into one PR, both root-caused correctly:
Partial color metadata
The engine's normalization contract at ffprobe.ts is "any color component present → emit the object with all three fields, missing ones as empty strings." Plan v1's reader tolerated that; Plan v2's generic readString rejected the empty ones. The fix — new readColorComponent at planV2.ts:678-683 that accepts empty strings for the three color fields specifically — matches the engine's contract exactly. Other string fields on the same shape (videoCodec, videoId, srcPath, framePattern) still go through readString and still reject empty; the four continues to reject an empty ${name} regression tests at planV2.test.ts:373-403 lock that non-loosening down. New engine ffprobe test normalizes omitted video color components to empty strings at ffprobe.test.ts:319-353 pins the upstream contract this fix depends on — good defensive coupling.
The 12-case rejection matrix (3 components × 4 invalid values: missing/null/number/object) is exhaustive coverage of the "not a string" boundary. The 3 partial-color happy paths (matrix-only / transfer-only / primaries-only) cover each single-component-present shape.
Fixture plan-v2-partial-color — real regression at render level, two-chunk config exercises the materialize-into-chunk path. Correct.
.hf-complete extraction-cache sentinel
listVideoFramePaths() iterates the video-frames/<id>/ directory. The engine's content-addressed extraction cache drops .hf-complete as a zero-byte ownership marker beside the numbered frames; distributed plan() materializes that whole directory tree, so the marker rides along into what v2 scans as "frames." Pre-fix, the frame-index regex /(\d+)(?=\.[^.]+$)/ on .hf-complete would not match, and depending on downstream handling this either errored or (worse) fell through as an unnumbered frame.
The fix — explicit sentinel handling at planV2.ts:250-259 — is fail-loud in exactly the right way:
- Recognize by exact filename match.
lstatSync(symlink-safe) →.isFile() && .size === 0→ skip.- Any other shape (non-empty file, symlink, directory) throws
PlanV2IntegrityErrorwith a clear message. Tests atplanV2.test.ts:225-259cover the valid case + both malformed shapes.
Choosing lstatSync over statSync is the right call — if someone symlinks the sentinel to something else, isFile() returns false on the symlink stat and the throw fires. Fail-closed.
Cross-repo check — EF Pydantic contract
Per your ask on the delegation memo re: EF#43345 tightening extra="forbid" to non-retryable: checked EF's heygen/temporal/model/product/hyperframes_distributed_activity_model.py. The wire boundary between HF sidecar and EF Temporal activities uses _WIRE_CONFIG = ConfigDict(extra="ignore") (line 226), and the Plan v2 shape stays entirely inside the HF Cloud Run boundary (materialized locally by workers, never crosses to Python). Grep for colorSpace/color_space/plan_v2 in experiment-framework/heygen/ returns no matches on the Pydantic model side. No EF-side model changes needed for this PR.
Small notes
-
readColorComponentis named after its use site but the ONLY behavior difference fromreadStringis allowing empty strings. If a future field needs the same shape, either rename to something likereadOptionalStringComponentor keep it color-specific with a docstring — probably fine as-is. -
Test-suite naming: the two malformed-sentinel tests use
for (const malformedSentinel of ["non-empty file", "directory"])— the "directory" branch creates the dir with a child file, which is worth doing (a well-crafted.hf-complete/directory shouldn't slip through as an empty dir), but the test description doesn't call that out. Non-blocking.
Nothing else. LGTM.
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 786b523f244053dfa19d6e6cfbc6e3d9ff043cc6. The R1 blocker is closed.
planV2.ts:217-249 now separates the contract into two explicit operations: validate every canonical video-frames/<videoId>/.hf-complete as a zero-byte regular file, then recognize only that exact three-segment path for omission. buildPlanV2Publication() invokes validation before buildVideoChunkDependencies() and skips the canonical sentinel before artifact targeting/hashing, so both exact-dependency and missing-meta/videos.json full-source modes share the same behavior. Both public publication paths (createPlanV2FromV1 and publishPlanV2FromV1) flow through this function.
The failure boundary remains conservative. A nested/unknown .hf-complete-shaped path is retained and assigned to all chunks in full-source mode, while canonical non-empty files, directories, and symlinks fail closed in both dependency modes. planV2.test.ts:239-299 pins the metadata-absent happy path, unknown-path over-inclusion, and the full 2 modes × 3 malformed shapes matrix. Exact-mode frame pruning still excludes unrendered numbered frames.
The previously approved partial-color work remains intact: only the three ffprobe-normalized color components accept empty strings; unrelated identifiers/codecs/paths stay non-empty, with positive and non-loosening coverage.
Local verification: focused Plan v2 suite passes 51/51 (107 assertions); oxlint reports 0 warnings/errors; oxfmt check passes. Exact-head build, typecheck, producer unit/integration, Windows tests/render, CodeQL, lint/format, and regression preflight are green. Long regression shards are still running with no failures, so merge should continue to wait for required CI settlement.
— Magi
Verdict: APPROVE
Reasoning: Sentinel validation and omission now happen at the shared publication boundary before dependency-mode selection, closing the full-source bypass without weakening unknown-artifact handling; focused tests and completed exact-head CI are green.
Summary
.hf-completeextraction-cache sentinel from exact video-frame dependenciesCloses #2807.
Root cause
The engine returns a non-null color object whenever any ffprobe color component is present, normalizing omitted components to empty strings. Plan v1 accepts that contract, but Plan v2 parsed all three components with its generic non-empty-string reader. A valid H.264 stream with only
color_space=bt709therefore failed planning on the omittedcolor_transfer.The real GCP smoke also exposed an adjacent Plan v2 incompatibility: distributed
plan()materializes the extraction cache's.hf-completemarker beside numbered frames, and the v2 dependency scanner attempted to parse it as a frame filename.Validation
bun test packages/producer/src/services/distributed/planV2.test.ts— 45 passedhyperframes-dev/us-east1Cloud Run + Workflows smoke:971eef11fbca0d6641c1b18ecbb89bce530f9c69dfee2ff200fc92cab8e2c90aReview
Independent implementation review completed with no outstanding findings.