feat(producer): add content-addressed plan v2#2788
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
156cefe to
607a745
Compare
d7a86f3 to
25d95a8
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The content-addressed split is thoughtfully fail-closed: planV2.ts:558-570 verifies selected blobs before publication, and planV2.ts:624-653 re-hashes the materialized bytes immediately before execution. The direct entry points also clean their temporary roots on every exit (planV2Execution.ts:17-24, planV2Execution.ts:37-54).
Blocker
packages/producer/src/services/distributed/planV2.ts:292-297—createPlanV2FromV1()trusts any 64-hexplan.json.planHashand never recomputes the frozen v1 directory before blessing its current bytes into a v2 manifest. If a v1 artifact changes after freeze, conversion content-addresses the changed bytes while retaining the oldsourcePlanV1Hash; the v2 worker then intentionally skips the aggregate v1 check atrenderChunk.ts:436-437. That turns a corrupt/tampered v1 source into an internally valid v2 plan and makes replay/output correlation lie. I reproduced this on the exact head:plan()produced v1 hash0d35af81…5aea; after appending bytes tocompiled/index.html,recomputePlanHashFromPlanDir()returneda87f0f77…e925, yet conversion succeeded and returned the stale first digest assourcePlanV1Hash. This is exposed publicly from both producer entry points, and the test fixture currently proves the gap by converting a directory with the fabricated"1".repeat(64)hash (planV2.test.ts:77-95). Recompute withrecomputePlanHashFromPlanDir()at the conversion boundary, reject a mismatch, and add a mutation/stale-hash regression.
Important
packages/producer/src/services/distributed/planV2.test.ts:99-255— none of the new tests callsplanV2()itself; they all exercisecreatePlanV2FromV1()over a hand-built tiny directory. The PR body specifically claims a pressure regression where capped v1 returnsPLAN_TOO_LARGEand v2 completes the same render, but the committed suite only has the pre-existing v1 cap test (planSizeCap.test.ts:91-140). Please land the motivating end-to-end regression so a future change cannot accidentally restore the cap or bypass only the test helper.packages/producer/src/services/distributed/planV2.ts:237-238,292— the new untrusted-JSON boundaries use bareas PlanVideosJson,as ChunkSliceJson[], andas Record<string, unknown>. That violates the repo's documented hard type-safety rule (CONTRIBUTING.md:47-54) and lets malformed source metadata reach frame-dependency selection under compiler-trusted shapes. Parse asunknownand narrow/validate before using the values.
Verification at exact head 25d95a8acf3408c9b099347f0ca16c2e1d0f4b49: focused producer tests 36/36, existing plan.test.ts 41/41, producer typecheck, AWS typecheck, oxlint on the changed distributed production files, and git diff --check all pass locally. All eight current regression shards and preflight are green. During the final freshness check, #2777 merged and this PR's base moved from its exact parent 607a745 to main@f9f00b0; the #2777 patch IDs are identical, but GitHub now reports this head as conflicting/dirty. The stack needs to be restacked, and the resulting head needs a fresh review.
— Magi
Verdict: REQUEST CHANGES
Reasoning: The v2 transport verifies its own blobs well, but the public v1→v2 conversion boundary can silently canonize a stale/corrupt v1 source while preserving a false source hash. The motivating large-plan path also needs the regression the PR body says was validated.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Head: 25d95a8acf3408c9b099347f0ca16c2e1d0f4b49
Independent R1 pass. I match Magi's blocker and add a few findings on top. The v2 transport itself is careful — content-addressed blobs, manifest integrity hash with a distinct hyperframes-plan-manifest-hash-v2\x00 prefix, verify-before-publish (planV2.ts:588-591), and re-verify at execution (planV2.ts:626-654). The problem is upstream of that: what the transport promises about its source.
Blockers
packages/producer/src/services/distributed/planV2.ts:292-297— stale/tampered v1 source is silently canonized into a v2 manifest.createPlanV2FromV1readsplan.json.planHashand blesses it assourcePlanV1Hashafter only anisSha256shape check. It never callsrecomputePlanHashFromPlanDir(planV1Dir)against the current bytes. In parallel,renderChunk.ts:436-437short-circuits the aggregate-hash gate for v2 withv2Manifest === null ? recomputePlanHashFromPlanDir(planDir) : plan.planHash— a tautology againstplan.planHashfrom the same file. Net effect: a mutation of the v1 staging dir between freeze and v2 conversion produces (a) a v2 that is internally consistent, (b) asourcePlanV1Hashthat no longer describes the bytes that were converted, (c) worker chunks that pass execution-time hash gates because both sides of the comparison come from the same v1plan.json. This is the exact "trust the freeze" invariant #2777 traded away, and it is publicly exported (distributed.ts:56-65). Fix: recompute increatePlanV2FromV1and reject on mismatch; add a mutate-then-convert regression. Full match with Magi.
Important
-
planV2.test.ts:99-255— the headline pressure claim ("capped v1 returns typedPLAN_TOO_LARGE; v2 completes the same render") has no committed test. All new tests exercisecreatePlanV2FromV1on hand-built ~200-byte fixtures.planSizeCap.test.tsstill only covers the v1 throw path. A future refactor could re-close the cap over v2 (e.g., someone adds a v2 staging cap) or bypass only via a test helper, and CI would stay green. Land aplanV2()end-to-end that trips v1's cap and asserts v2 completes on the same input. Match with Magi. -
planV2.ts:237-238, 292— bareascasts at JSON-boundary parses.videos.json,chunks.json, and the v1plan.jsonare pulled in withas PlanVideosJson,as ChunkSliceJson[],as Record<string, unknown>. ContrastparsePlanV2Manifest(planV2.ts:467-526), which is meticulously narrowed. These files are internally-produced today, but the read-boundary is public viacreatePlanV2FromV1— a stale/hand-edited v1 dir slips malformed shapes intobuildVideoChunkDependenciesunder compiler-trusted types. Repo standards (CONTRIBUTING.md rules 47/54/55) want validated narrowing here. Match with Magi. -
renderChunk.ts:210-217— sparse frame-index change is behavior-affecting and undisclosed.framePaths.set(i, ...)becomesframePaths.set(numbered ? Number(numbered[1]) - 1 : i, ...). For standard ffmpeg 1-based names the result is identical, but this is a semantic pivot from "sorted position" to "filename-encoded index". Any pipeline that historically emitted 0-based or non-numeric frame names now reindexes silently. Not in the PR summary; would be worth a one-line disclosure and a v1-side regression asserting the identity case.
Nits
planV2.ts:78-82, 384-399—PlanV2Resultdropslimitations.videoDependencyMode. Callers ofplanV2()can't tell from the return value that video deps fell back tofull-source-pack; they must open the manifest. Cheap observability add.planV2.ts:229-235— thefull-source-packfallback branch has no test. It fires whenvideo-frames/exists butmeta/videos.jsonis absent, marks allvideo-frames/**aschunks: "all", and is your safety net against metadata drift. Untested safety nets tend to bit-rot.planV2.ts:334, 416-421—readSupportedFps(dimensions.fpsNum)treatsfpsNumasfpsand dropsfpsDenentirely from the manifest. Fine under the current invariant thatfpsDen === 1(plan.ts:1038-1039), but the invariant isn't asserted here — a future NTSC-fraction change would land silently in the manifest.
Behavior lenses
- (A) Root-cause fix. Partial.
planV2()bypasses the monolithic 2 GiB cap by forwardingNumber.MAX_SAFE_INTEGERtoplan()and by never emitting a single archive. But the planner still stages the full v1 tree locally (disclosed) andcompiled/**is still shared across every chunk (disclosed). No committed test exercises the "cap trips v1, v2 succeeds" path — see Important #1. - (B) Schema evolution. Explicit opt-in preserved:
CURRENT_PLAN_PROTOCOLstill emits v1;PLAN_PROTOCOL_V2is a distinct descriptor with its ownschemaVersion/artifactLayout/hashSchematriple and distinct hash prefix (planV2.ts:47). Fail-closed on v1 readers hitting a v2 root works (planProtocol.ts:97-111, verified byplanProtocol.test.ts:281-296). - (C) Serialization boundary. sha256 throughout;
assertSafeRelativePathrejects absolute paths and./..segments; POSIX normalization on both read (listFiles) and write (materializePlanV2Target). Path safety is solid. - (D) Chunking / audio isolation. Per-chunk dependencies are derived by walking every rendered frame's
getActiveFramePayloads— exact.audio.aac→{ chunks: [], assembler: true }— hard isolation, verified byplanV2.test.ts:114-125. - (E) Observability & failure attribution.
PlanProtocolUnsupportedErroris typed and terminal;[planV2]-prefixed messages are specific.recomputePlanHashFromPlanDirtiming is preserved viaplanHashMseven when the v2 path skips the actual work — minor: the metric name is misleading in the v2 branch.
Standards + Spec + Precision + Round-trip
- Standards (Miguel's lens): three bare-cast sites at JSON boundaries — see Important #2.
- Spec forward (all four PR-body claims):
- Opt-in v2, v1 default — ✅ (
planProtocol.ts:37-45,plan.tsuntouched at emit). - Split root + verify-before-materialize — ✅ (
planV2.ts:588-591), but the v1→v2 conversion boundary skips its own recompute — see Blocker. - Exact per-chunk video deps + audio assembler-only — ✅ (
planV2.ts:246-256,176-178). - Direct
renderChunkV2/assembleV2, fail-closed on v1 reader — ✅ (planV2Execution.ts,planProtocol.ts:100-111).
- Opt-in v2, v1 default — ✅ (
- Spec reverse (undisclosed diff):
- Hash-recompute short-circuit in
renderChunk.ts:436-437— behavioral change not called out. renderChunk.ts:210-217frame-index derivation change — see Important #3.assemble.ts:44-46,124-125acquiresvalidatePlanV2MaterializedTargetcall — semantically necessary, not disclosed.
- Hash-recompute short-circuit in
- Precision divergence: hash prefix vs
PLAN_V2_HASH_SCHEMAdiffer only by trailing\x00— intentional and consistent.listVideoFramePathsandrebuildExtractedFramesFromPlanDirboth use/(\d+)(?=\.[^.]+$)/with- 1— consistent.fpsNum → fpscollapse — see Nit #3. - Middle-man wrap/unwrap:
resultFromManifestdropslimitations— see Nit #1.
Root-cause verification
Partial. v2 removes the monolithic transport ceiling for the wire payload, but (a) the staging v1 plan still materializes fully at plan time, (b) compiled/** is still all-to-all, and (c) the "big scene now works" outcome is asserted in the PR body only, not by any committed test. The Blocker above additionally lets a corrupted v1 staging dir slip through the conversion boundary while claiming a false sourcePlanV1Hash, which means the v2 transport is content-addressed against bytes it can't prove originated from the hash it advertises. Close the Blocker + land the pressure regression and this becomes a clean root-cause resolution for the transport half.
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 25d95a8acf3408c9b099347f0ca16c2e1d0f4b49.
Full /code-review max workflow — 5 lens-diverse sub-agents (CONTRIBUTING.md compliance, shallow-bug, git-history/blame, prior-PR-comments, code-comment invariants), followed by the HF-specific overlays (peer-producer, sibling-callsite, divergence grep, PR-body promise audit, retry-classifier grep).
Two blockers, both variants of the same class of regression Miguel and Vance blocked on #2777 R1 — deterministic non-healing errors escaping the adapter retry classifier and burning the full 4×15-min retry budget on failures that cannot recover.
Blockers
B1 — 13 new [planV2] deterministic errors thrown as plain new Error(...); zero added to AWS/GCP non-retryable classifiers. Convergent finding from git-history and prior-PR-comment lenses. planV2.ts throws 13 deterministic-only failures (manifest integrity hash mismatch, artifact hash mismatch, materialized artifact hash mismatch, artifact size mismatch, missing content-addressed artifact, materialization target does not match requested execution role, unsafe artifact path, etc.) — every one a bare new Error("[planV2] …") with no code, no dedicated class, .name === "Error". Meanwhile packages/aws-lambda/src/cdk/HyperframesRenderStack.ts and packages/gcp-cloud-run/src/server.ts NON_RETRYABLE_ERROR_NAMES correctly carry PlanProtocolUnsupportedError/PLAN_PROTOCOL_UNSUPPORTED (the #2777 R2 fix Miguel required) but touch nothing about planV2. validatePlanV2MaterializedTarget is invoked from renderChunk.ts:352 and assemble.ts:125, so these errors WILL reach the adapter's retry surface. Every deterministic v2 integrity failure will burn 4×15-min on a chunk that is guaranteed to re-fail. This is worse than the #2777 R1 shape — that error was un-classified but at least typed; these are un-typed too, so the follow-up adapter PR has nothing to classify. Fix: introduce a PlanV2IntegrityError extends Error (with typed code, e.g., PLAN_V2_INTEGRITY_UNRECOVERABLE) analogous to PlanProtocolUnsupportedError, thread it through the planV2.ts throw sites, add both code and name to AWS NON_RETRYABLE_PLAN/CHUNK/ASSEMBLE and GCP NON_RETRYABLE_ERROR_NAMES, pin with the CDK snapshot + GCP HTTP tests. Same recipe as the #2777 R2 fix.
B2 — validatePlanV2MaterializedTarget short-circuits the typed PLAN_HASH_MISMATCH gate in renderChunk.ts. The original PLAN_HASH_MISMATCH gate (blame origin e92700ac) exists specifically to catch content-fingerprint drift and raise a typed RenderChunkValidationError(PLAN_HASH_MISMATCH) — classified non-retryable by BOTH adapters. renderChunk.ts:436-437 now sets recomputedPlanHash = v2Manifest === null ? recomputePlanHashFromPlanDir(planDir) : plan.planHash, making the equality check a tautology in the v2 path. Content-drift is still caught per-artifact via the manifest's sha256s in validatePlanV2MaterializedTarget, but that throws a plain Error("[planV2] materialized artifact hash mismatch: …") — again unclassified. Same retry-budget-burn as B1. Fix: rooted in the same PlanV2IntegrityError proposal — throw the typed error instead of a plain one at planV2.ts:650.
Concerns
Three orange bare as T at untrusted JSON boundaries (planV2.ts:237,238,292) violate CONTRIBUTING.md:47/54 — untrusted-JSON boundary is the exact case the rule targets, requiring either a type guard OR as unknown as T with a one-line justification comment. planV2.ts:237-238 cover videos and chunks which are directly consumed by chunk-worker logic; :292 casts v1Plan to Record<string, unknown> which is thin narrowing, but the rule applies uniformly.
Yellows
v2 path skips recomputePlanHashFromPlanDir (renderChunk.ts:436) — coverage regression: v1's aggregate plan-hash cross-check ("plan.json's declared planHash agrees with a fresh derivation over the tree") is gone in v2 mode. Per-artifact sha256 catches content tampering, but "plan.json is internally inconsistent with its own claim about the rest of the tree" (partial upstream freezePlan, tampered plan.json packaged into v2, producer bug) is now undetected. Consider verifying sourcePlanV1Hash against recomputePlanHashFromPlanDir(planV1Dir) inside createPlanV2FromV1 (freeze-time, once), so the v2 boundary preserves the invariant.
readPlanProtocol return type widened from PlanProtocolV1Descriptor to SupportedPlanProtocolDescriptor union. Public export from @hyperframes/producer and @hyperframes/producer/distributed, so any external adopter that consumed the v1-only signature between #2777 (Jul 25 morning) and this PR (Jul 25 evening) hits a source-level type break. ~10hr gap so likely internal-only, but worth noting. The new strict readPlanProtocolV1 was added and internal callers migrated — the design is right, just the type widening on the shared name is the risk.
Non-null assertions in rebuildExtractedFrames.test.ts:173,176,179,182 on extracted!.framePaths.get(...) where extracted comes from array-destructure with no guard. CONTRIBUTING.md:55 permits ! only after if-check paths. Small — test code — but the rule applies.
Staging-dir planDirSizeLimitBytes: Number.MAX_SAFE_INTEGER in planV2.ts:~1002 disables the typed PLAN_TOO_LARGE gate. PR body concedes "current planner still stages a complete v1 plan locally before converting" — a plan too big to fit /tmp now surfaces as raw ENOSPC from writeBlob/copyFileSync (retryable I/O error) instead of typed PLAN_TOO_LARGE. My #2777 Q2 asked about this — partial answer here. Adapters lose the deterministic typed size signal. Non-blocking because v2 is opt-in transport, but worth confirming planV2() is contractually controller-side (non-Lambda) and documenting.
Questions
-
The
.hyperframes-plan-v2.jsonmarker file is written into the materialized dir atplanV2.ts:600, butHASH_EXCLUDED_PLAN_FILESinfreezePlan.ts:156only excludesplan.json,meta/encoder.json,compiled/index.html. A legacy or third-party v1 chunk executor callingrecomputePlanHashFromPlanDiron the materialized dir would include the marker in the hash and trip PLAN_HASH_MISMATCH. This PR's ownrenderChunkhandles it by branching onv2Manifest === null, but the "v1-compatible execution directory" comment at planV2.ts:573 suggests untouched v1 code should work — is that expected to hold, or is it explicitly "our v2-aware renderChunk only"? -
readSupportedFpsatplanV2.ts:415-419acceptsdimensions.fpsNumin isolation without consideringfpsDen. Elsewhere at line 247 the code correctly uses(frame * fpsDen) / fpsNumfor global-time (honors fractional fps). But the manifest'sfpsscalar is derived fromfpsNumalone — iffpsDen != 1legitimately reaches this check (e.g., 30/2 = 15 fps producer), the manifest'sfps: 30would be a lie. Is there an implicit "v2 requiresfpsDen === 1" contract that should be documented / enforced? -
listFilesatplanV2.ts:~128usesDirent.isFile()/.isDirectory()which return false for symlinks. Any symlink in the v1 planDir (e.g., a shared audio cache) is silently omitted from the v2 manifest. Aware of any adapter path that emits symlinks today, or is v1 guaranteed regular-files-only? -
The sparse-index parse at
renderChunk.ts:210-216derivesframeIndex = Number(match[1]) - 1, hard-coding ffmpeg's 1-based image-sequence convention. If any historical or future extractor starts at00000, the first captured frame maps toframePaths.set(-1, …). Non-live today (verifiedvideoFrameExtractor.ts:356usesi+1), but worth pinning with an assertion — throw if smallest parsed digit is 0, or derive offset dynamically. -
audio.aacfilename hard-coded atplanV2.ts:172. If v1 ever supports a non-AAC container (e.g.,audio.opus), the file falls through to{chunks: "all", assembler: true}and ships to every chunk, butmaterializePlanV2Target.audioPathstaysnull—assembleV2callsassemble(..., null, ...)and silently produces video-only output despite the composition having audio. Extract to a shared constant referenced by bothplan.ts(writer) andplanV2.ts(packager), or derive audio membership fromplan.hasAudio?
Verified clean (adversarial pass didn't find issues)
- Fail-closed on v1 readers receiving a v2 root —
renderChunkandassemblenow callreadPlanProtocolV1, which delegates toreadPlanProtocolthen strict-refs againstCURRENT_PLAN_PROTOCOL. Test atplanProtocol.test.ts:285covers both roles. - Manifest integrity check round-trips deterministically — writer hashes over
base(12 fields, no planHash); reader reconstructs same 12 fields, destructures planHash, hashes payload.canonicalJsonStringifysorts keys so insertion order is irrelevant. Tampering test atplanV2.test.ts:206covers. verifyBlobruns on every selected artifact BEFORE anymkdirSyncormkdtempSync— the corruption test atplanV2.test.ts:191confirmsexistsSync(destination)stays false when any blob mismatches.- Path-traversal defense —
assertSafeRelativePathrejects absolute paths, empty segments,.and..on both Unix and Windows separators, applied inparseArtifactandbuildVideoChunkDependenciesbefore any join/copyFileSync. - Chunk-frame iteration bounds
[chunk.startFrame, chunk.endFrame)match the render-sidecaptureStage.frameRange(renderChunk.ts:644) — no off-by-one. - FrameLookupTable time-monotonic assumption holds — planV2 outer loop iterates chunks in
chunks.jsonorder and frames strictly ascending within each chunk;refreshActiveSetnever rewinds. - Concurrent materialization safe —
materializePlanV2Targetwrites into amkdtempSyncsibling thenrenameSync(atomic publish);writeBlobincreatePlanV2FromV1uses${dst}.tmp-${pid}+renameSyncand short-circuits on existing blob (safe because content-addressed). - Manifest hash domain-separated by
PLAN_V2_HASH_PREFIX— no accidental collision with plan-hash or blob-hash domains. - Chunk retries on same
(planDir, chunkIndex)produce byte-identical chunk sha256 — v2 materialization is deterministic. - ffmpeg 1-based → 0-based frame-index convention consistent between
planV2.ts:210andrenderChunk.ts:214-216(both use/(\d+)(?=\.[^.]+$)/and subtract 1). - Audio-only-for-assembler invariant end-to-end — planV2.ts:176 assigns
audio.aacto{chunks: [], assembler: true}; planV2.ts:617-620 only returns non-null audioPath for assembler; planV2Execution.ts:48 threads it only throughassembleV2.
Meta
Also owed on hyperframes#2777: I posted LGTM at R1 and missed the P1 that Via + Magi both caught (PlanProtocolUnsupportedError documented terminal but not in either adapter's NON_RETRYABLE_*, burning full retry budget). Same lens class as B1/B2 here — when a new typed error is introduced with a "terminal" / "non-retryable" / "cannot heal" comment, grep the AWS NON_RETRYABLE_* and GCP NON_RETRYABLE_ERROR_NAMES lists to confirm classification exists. Now baked into my role playbook.
| const { planHash: _planHash, ...payload } = parsed; | ||
| const expectedHash = computeManifestHash(payload); | ||
| if (expectedHash !== parsed.planHash) { | ||
| throw new Error("[planV2] manifest integrity hash mismatch"); |
There was a problem hiding this comment.
🔴 B1 — 13 new [planV2] deterministic errors thrown as plain Error; adapter retry classifiers have zero coverage
if (recomputed !== manifestHash) {
throw new Error("[planV2] manifest integrity hash mismatch");
}grep 'throw new Error("\[planV2\]' planV2.ts returns 13 hits at lines 296, 305, 417, 424, 467, 474, 477, 479, 488, 494, 514, 633, 640 — plus the ~15 identical throws in validatePlanV2MaterializedTarget / verifyBlob / etc. Every one is a new Error(...) with .name === "Error", no code field, no dedicated class.
Miguel's P1 on #2777 (matched by Vance R1) required PlanProtocolUnsupportedError to be classified in BOTH packages/aws-lambda/src/cdk/HyperframesRenderStack.ts NON_RETRYABLE_{PLAN,CHUNK,ASSEMBLE} and packages/gcp-cloud-run/src/server.ts NON_RETRYABLE_ERROR_NAMES by both its code string AND its class name, pinned by the CDK snapshot + GCP HTTP tests. Verified those lists at HEAD have the #2777 R2 fix (PLAN_PROTOCOL_UNSUPPORTED / PlanProtocolUnsupportedError) but touch nothing about planV2.
validatePlanV2MaterializedTarget is called from renderChunk.ts:352 and assemble.ts:125, so these Errors DO reach the adapter retry surface. Every deterministic v2 integrity failure → 4×15-min retry burn on a chunk that is guaranteed to re-fail.
Recommended fix: introduce PlanV2IntegrityError extends Error in planV2.ts (or a small planV2Errors.ts sibling) with a typed code: typeof PLAN_V2_INTEGRITY_UNRECOVERABLE, replace every throw new Error("[planV2] …") on a deterministic path with throw new PlanV2IntegrityError(...), add BOTH code and name to AWS NON_RETRYABLE_PLAN/CHUNK/ASSEMBLE and GCP NON_RETRYABLE_ERROR_NAMES, pin with CDK snapshot + GCP HTTP tests. Same recipe as the #2777 R2 fix — this PR is the producer-side half; the adapter-side wiring naturally lives with it since the typed carrier is a producer-owned symbol.
| const planHashStarted = Date.now(); | ||
| const recomputedPlanHash = recomputePlanHashFromPlanDir(planDir); | ||
| const recomputedPlanHash = | ||
| v2Manifest === null ? recomputePlanHashFromPlanDir(planDir) : plan.planHash; |
There was a problem hiding this comment.
🔴 B2 — v2 path short-circuits the typed PLAN_HASH_MISMATCH gate with a plain Error in validatePlanV2MaterializedTarget
const recomputedPlanHash =
v2Manifest === null ? recomputePlanHashFromPlanDir(planDir) : plan.planHash;
const planHashMs = Date.now() - planHashStarted;
if (recomputedPlanHash !== plan.planHash) {
throw new RenderChunkValidationError(
PLAN_HASH_MISMATCH,
...In the v2 path, recomputedPlanHash is set to plan.planHash itself, so the equality check is a tautology and always passes — the typed PLAN_HASH_MISMATCH RenderChunkValidationError (classified non-retryable in BOTH adapters) can no longer fire.
The v2 substitute is validatePlanV2MaterializedTarget (planV2.ts:625+), which does verify each artifact's sha256 — but at planV2.ts:650 it throws new Error("[planV2] materialized artifact hash mismatch: ${artifact.path}") — a plain unclassified Error, exactly the B1 problem.
The original PLAN_HASH_MISMATCH gate has an explicit "this is purely content-fingerprint drift" comment (blame origin e92700ac) codifying why it raises a typed error. That contract is broken in v2 mode.
Fix: paired with B1 — throw new PlanV2IntegrityError(...) at planV2.ts:650 instead of a plain Error, so the adapter's classifier catches it. Optionally, restore per-artifact drift → RenderChunkValidationError(PLAN_HASH_MISMATCH, ...) in the v2 path to keep the error-code contract identical between v1 and v2.
| : { mode: "exact-rendered-frames", dependencies: new Map() }; | ||
| } | ||
| const chunksPath = join(planV1Dir, "meta", "chunks.json"); | ||
| const videos = JSON.parse(readFileSync(videosPath, "utf-8")) as PlanVideosJson; |
There was a problem hiding this comment.
🟠 Bare as T at untrusted-JSON boundary — CONTRIBUTING.md:47/54
const videos = JSON.parse(readFileSync(videosPath, "utf-8")) as PlanVideosJson;
const chunks = JSON.parse(readFileSync(chunksPath, "utf-8")) as ChunkSliceJson[];JSON.parse(readFileSync(...)) is the exact untrusted-JSON boundary CONTRIBUTING.md calls out. The rule permits either a type guard / centralized narrowing helper, or as unknown as T with a one-line justification comment. Same pattern repeats at planV2.ts:292 (v1Plan cast to Record<string, unknown> — still a bare cast, still needs the justification form).
Fix: as unknown as PlanVideosJson (with a one-line comment: e.g. // on-disk v1 videos.json is producer-owned and validated upstream), or introduce a parsePlanVideosJson(raw: unknown): PlanVideosJson type-guard reader that throws on shape mismatch (aligns with the v2 fail-closed posture and would also catch producer-side corruption sooner).
| // content-fingerprint drift. | ||
| const planHashStarted = Date.now(); | ||
| const recomputedPlanHash = recomputePlanHashFromPlanDir(planDir); | ||
| const recomputedPlanHash = |
There was a problem hiding this comment.
🟡 v2 path removes the aggregate v1 plan-hash cross-check (coverage regression)
const recomputedPlanHash =
v2Manifest === null ? recomputePlanHashFromPlanDir(planDir) : plan.planHash;In v2 mode the recompute is skipped and the check becomes a tautology. Per-artifact content-addressed sha256 verification catches artifact tampering, but the aggregate invariant "plan.json's declared planHash matches a fresh derivation over the v1 layout" is no longer checked anywhere in the v2 lifecycle — createPlanV2FromV1 reads v1Plan.planHash and forwards it as sourcePlanV1Hash without ever calling recomputePlanHashFromPlanDir(planV1Dir).
Failure modes now undetected in v2: (1) producer bug where freezePlan writes a stale planHash into plan.json but the tree is fresh, (2) partial upstream freezePlan that packages an inconsistent v1 dir into v2, (3) a v1 planDir with a manually-edited plan.json.planHash then packaged.
Fix: either verify sourcePlanV1Hash against recomputePlanHashFromPlanDir(planV1Dir) inside createPlanV2FromV1 (freeze-time check, once — preserves the invariant at the v1→v2 boundary), or stage a v1-shape aggregate over the materialized subset in renderChunk's v2 path. First approach is cheaper and localizes the check.
| // This is the fail-safe policy table for every v1 artifact class. Keeping the | ||
| // branches together makes new artifact classes visibly fall through to both roles. | ||
| // fallow-ignore-next-line complexity | ||
| function artifactTargets( |
There was a problem hiding this comment.
🔵 audio.aac filename hard-coded — non-AAC audio would silently produce video-only output
function artifactTargets(
path: string,
videoDependencies: ReadonlyMap<string, readonly number[]> | null,
): Pick<PlanV2Artifact, "chunks" | "assembler"> {
if (path === "audio.aac") return { chunks: [], assembler: true };Today v1's plan writer only ever emits audio.aac (grepped confirm), so this is safe. But this string is coupled to v1's audio-mux path; if v1 ever supports a non-AAC container (audio.opus for WebM opus passthrough, for example), the file would fall through to the "unknown v1 files" branch, be marked {chunks: "all", assembler: true}, and get shipped to every chunk worker (harmless but wasteful). More importantly, materializePlanV2Target.audioPath would still be null (because it also filename-matches audio.aac), so assembleV2 would call assemble(..., null, ...) and silently produce a video-only output despite the composition having audio.
Fix: extract the audio filename to a shared constant that both plan.ts (writer) and planV2.ts (packager) reference, or derive audio membership from plan.hasAudio rather than filename. hasAudio-based is more future-proof.
| return value as number; | ||
| } | ||
|
|
||
| function readSupportedFps(value: unknown): 24 | 30 | 60 { |
There was a problem hiding this comment.
🔵 readSupportedFps accepts fpsNum in isolation; silently drops fpsDen
function readSupportedFps(value: unknown): 24 | 30 | 60 {
if (value !== 24 && value !== 30 && value !== 60) {
throw new Error("[planV2] dimensions.fpsNum must be 24, 30, or 60");
}
return value;
}Elsewhere at line ~247 the actual global-time computation uses (frame * fpsDen) / fpsNum and correctly honors fractional fps rationals. But the manifest's fps scalar (per the readonly field's type comment fps: 24 | 30 | 60) is derived from fpsNum alone. If fpsDen != 1 ever legitimately reaches this check (e.g., a producer emitting 30/2 = 15 fps), the manifest's fps: 30 would be a lie — every downstream consumer (assembler frame-rate assertion, adapter-side telemetry) sees the wrong value.
If v2 has an implicit "fpsDen must be 1" contract, please assert it explicitly here (if (fpsDen !== 1) throw ...) and document. If v2 supports fractional, fps needs to be the pair not the scalar.
| } | ||
| } | ||
|
|
||
| function listFiles(root: string): Array<{ path: string; absolutePath: string }> { |
There was a problem hiding this comment.
🔵 listFiles silently drops symlinks (Dirent.isFile/isDirectory return false on symlinks without follow)
function listFiles(root: string): Array<{ path: string; absolutePath: string }> {
...
function walk(dir: string): void {
for (const entry of readdirSync(dir, { withFileTypes: true })) {
...
if (entry.isDirectory()) {
walk(absolutePath);
} else if (entry.isFile()) {
files.push({ ... });
}
}
}
}Dirent.isFile() and .isDirectory() return false for symlinks unless the read is done with lstat follow-through. Any symlink in the v1 planDir is silently omitted from the v2 manifest — no error, no artifact — so a v1 layout that legitimately used a symlink (e.g., an assembler audio.aac symlink into a shared cache, a compiled-asset symlink) would produce a v2 tree that's missing that dependency at execution time and only fails deep inside ffmpeg with an obscure error.
I don't think v1 emits symlinks today (couldn't find one in a grep of the freezePlan / distributedPlan paths), but the current shape is silently-tolerant of a class of drift. Consider either following symlinks explicitly (statSync(absolutePath)) or throwing on unknown Dirent kinds so any drift surfaces at v2-create time.
| // chunk's global capture range can request. Preserve ffmpeg's 1-based | ||
| // image-sequence number instead of re-indexing the sparse directory. | ||
| // Legacy/unusual names retain the historical sorted-position fallback. | ||
| const numbered = /(\d+)(?=\.[^.]+$)/.exec(frameName); |
There was a problem hiding this comment.
🔵 Sparse-index parse hard-codes ffmpeg's 1-based image-sequence convention
const numbered = /(\d+)(?=\.[^.]+$)/.exec(frameName);
const frameIndex = numbered ? Number(numbered[1]) - 1 : i;
framePaths.set(frameIndex, join(outputDir, frameName));The new sparse-directory logic parses the trailing digit run and subtracts 1 unconditionally, matching the current extractor at videoFrameExtractor.ts:356 which relies on ffmpeg's default -start_number 1. For v1 dense sequences starting at 00001 this is identical to the old framePaths.set(i, ...) behavior — so no live regression today.
But if any future/historical extractor ever writes frame_00000.<ext> (-start_number 0), the new mapping is framePaths.set(-1, frame_00000) — a silent off-by-one on the first frame at t=0 that would surface as the wrong extracted frame with no obvious error. Old code would have been framePaths.set(0, frame_00000).
Fix: either validate that the smallest parsed digit is 1 (throw if 0) at v2 create time, or derive the offset dynamically from the first observed file so 0-based sequences still map to Map key 0. Cheap belt-and-braces against a class of drift that's hard to detect at runtime.
| }, | ||
| ]); | ||
|
|
||
| expect(extracted!.framePaths.get(20)).toBe( |
There was a problem hiding this comment.
🟡 ! non-null assertions on optional array destructure — CONTRIBUTING.md:55
expect(extracted!.framePaths.get(20)).toBe(...)const [extracted] = rebuildExtractedFramesFromPlanDir(...) destructures the first element of an array; extracted is typed as Foo | undefined and there's no preceding guard (if (!extracted) throw, expect(extracted).toBeDefined(), etc.) before ! is applied. CONTRIBUTING.md:55 permits ! only after if-check paths — this test uses ! cold on an optional. Same pattern repeats at lines 176, 179, 182 (one finding covers all four).
Fix: if (!extracted) throw new Error("expected extracted result"); above the block, or expect(extracted).toBeDefined(); + assert(extracted), or destructure with a strict-first helper.
45afab0 to
7d1c9f6
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function sha256File(path: string): string { | ||
| const hash = createHash("sha256"); | ||
| const buffer = Buffer.allocUnsafe(1024 * 1024); | ||
| const fd = openSync(path, "r"); |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head re-review at 7d1c9f64fb464001a5bcdf4557f1c74a1193a68d.
The R1 issues are closed cleanly: planV2.ts:417-424 now recomputes the frozen v1 fingerprint before content-addressing, planV2.test.ts:147-159 pins post-freeze tampering, planSizeCap.test.ts:142-176 exercises the motivating capped-v1 → successful-v2 path, and the JSON boundaries now narrow/validate unknown values. The sparse-frame behavior is also explicitly split between dense v1 and sparse v2 at renderChunk.ts:188-216, while PlanV2Result.limitations and the full-source fallback are test-locked.
I independently confirm Rames' remaining blocker on this exact head:
blocker — deterministic v2 integrity failures still escape as retryable plain Errors. planV2.ts:653, planV2.ts:700-707, and planV2.ts:772-789 throw plain Error for manifest, blob, marker, and materialized-artifact corruption. Both execution entry points call this validation (renderChunk.ts:351-355, assemble.ts:124-126), but AWS's NON_RETRYABLE_{PLAN,CHUNK,ASSEMBLE} sets (HyperframesRenderStack.ts:196-225) and GCP's NON_RETRYABLE_ERROR_NAMES (server.ts:577-598) have no v2 integrity name/code. The resulting .name === "Error" is retried even though the same bytes cannot heal.
The v2 branch also deliberately makes the legacy aggregate check a tautology at renderChunk.ts:436-445; per-artifact validation is the correct substitute, but its failures must retain the same terminal/non-retryable contract. Introduce a typed v2 integrity error/code, use it for deterministic v2 validation failures, and classify/pin it in both adapters.
Verification:
- producer typecheck — pass
- focused v2 / plan-size / frame-rebuild tests — 26/26 pass
git diff --check— pass- CI unit/integration/typecheck lanes — green; regression/Windows/CodeQL/global-install lanes still running at review time
Verdict: REQUEST CHANGES
Reasoning: the original conversion-integrity and coverage gaps are fixed, but deterministic v2 integrity failures still fall through both distributed retry classifiers and can burn the full retry budget on immutable corruption.
— Magi
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 7d1c9f64fb464001a5bcdf4557f1c74a1193a68d — delta from 25d95a8acf34 (my R1).
LGTM at R2. Author addressed the P1 quartet cleanly and my R1 flags on the JSON-boundary type surface.
Resolved:
- Stale v1 planHash on
createPlanV2FromV1(Magi P1, Via P1) —recomputePlanHashFromPlanDir(planV1Dir)now runs atplanV2.ts:417-424before content-addressing bytes; test atplanV2.test.ts:141-153locks in tamper-after-freeze rejection.renderChunk.ts:437v2 short-circuit stays intact by design (v2 has no whole-planDir hash; the granular integrity gate isvalidatePlanV2MaterializedTarget, called on both chunk and assembler entry paths). - Sparse-index pivot (Via P1) —
rebuildExtractedFramesFromPlanDirnow takes explicitindexMode: "dense-v1" | "sparse-v2"(default"dense-v1"), callsite atrenderChunk.ts:501-505dispatches onv2Manifest === null, comment rewritten to name the two modes. PlanV2Result.limitations.videoDependencyModedropped (Via P1) — restored viaPlanV2Limitationsinterface, roundtripped throughresultFromManifestandparsePlanV2Manifest.full-source-packfallback untested (Via P1) — new test atplanV2.test.ts:185-201.fps/fpsDencollapse (Via P1) — newreadV1PlanFpsatplanV2.ts:556-562fail-closes onfpsDen !== 1, test atplanV2.test.ts:222-226locks in.- Bare
as Tat JSON boundaries (my R1 orange) —planV2.tsnow usesJSON.parse(...): unknownat every entry point (videos.json,chunks.json,v1 plan.json, manifest, marker) withparsePlanVideosJson/parseChunkSlices/isRecordnarrowing. New test atplanV2.test.ts:207-232locks invideos[0].hasAudio must be booleanandendFrame must be greater than startFrameat the JSON boundary. - Pressure regression test committed (Magi P1) —
planSizeCap.test.ts:150-181(lets planV2 complete the same render that trips the v1 transport cap) withplanDirSizeLimitBytes: 1024, asserts v1 throwsPlanTooLargeErrorand v2 completes with schema v2.
Owed correction on my R1 severity call. I framed B1 (PlanV2IntegrityError typed class + AWS/GCP classifier wiring) and B2 (renderChunk.ts:437 short-circuit) as blockers. On re-read, both were overweighted for #2788 specifically:
- The PR body says explicitly: "The AWS Lambda adapter, live AWS parity test, and smoke-test safety work are in the next stacked PR." — adapter classifier wiring properly belongs to #2789 (
packages/aws-lambda/src/cdk/HyperframesRenderStack.tsNON_RETRYABLE_*) and #2790 (packages/gcp-cloud-run/src/server.tsNON_RETRYABLE_ERROR_NAMES), not this protocol-plumbing slice. - B2's v2 short-circuit is architecturally correct — a materialized v2 planDir is a subset of the source v1 tree, so
recomputePlanHashFromPlanDiron it necessarily can't matchplan.planHash(which carriessourcePlanV1Hash). The substitute checkvalidatePlanV2MaterializedTargetverifies per-artifact SHA + size at both entry points (renderChunk.ts:355,assemble.ts:*), which is the right integrity contract for content-addressed subsets.
Both concerns fold into a single carry-forward for #2789: define a PlanV2IntegrityError typed class (with a code constant like PLAN_V2_INTEGRITY_MISMATCH) covering validatePlanV2MaterializedTarget's throws at planV2.ts:707 and :789 plus the createPlanV2FromV1 fingerprint-mismatch at :418-424, then wire that constant + class name into both adapter NON_RETRYABLE_* lists and pin with CDK snapshot + GCP HTTP tests. Same recipe #2777 R2 used for PlanProtocolUnsupportedError. Will lens for this on #2789's review.
One nit — PLAN_V2_MATERIALIZATION_MARKER (.hyperframes-plan-v2.json) is not in HASH_EXCLUDED_PLAN_FILES at freezePlan.ts:156-160. In forward operation this doesn't matter (the marker only exists in materialized v2 planDirs, and v2 mode short-circuits recomputePlanHashFromPlanDir). Rollback scenario is safe too: a pre-#2788 executor reading a v2 materialized planDir fails-closed with a typed PLAN_HASH_MISMATCH (marker file skews the hash), which is a classified non-retryable error → correct terminal behavior. Adding the marker to HASH_EXCLUDED_PLAN_FILES is optional cleanup; leaving it out is defensible as-is.
Fresh CI: all required checks green (Test, Typecheck, Test: runtime contract, Studio: load smoke, Smoke: global install, Format, Preview parity, etc.). Optional regression-shards still running (shard-7 pending at review time), all others green.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Head: 7d1c9f64fb464001a5bcdf4557f1c74a1193a68d
R2 verification pass over the delta since 25d95a8a. Every prior finding has landed, with regression coverage for each. The v1→v2 conversion boundary now enforces content-fingerprint parity before it canonizes anything, and the sparse-index pivot is explicit and mode-scoped.
R2 verification
| # | Prior finding | Status | Evidence at head 7d1c9f64 |
|---|---|---|---|
| Blocker | Stale/tampered v1 canonized into v2; renderChunk.ts:436-437 self-referential |
Addressed | planV2.ts:417-423 calls recomputePlanHashFromPlanDir(planV1Dir) and rejects on mismatch. Materialized subsets are separately guarded by parsePlanV2Manifest (integrity re-hash at planV2.ts:650-654) plus per-artifact rehash in validatePlanV2MaterializedTarget (planV2.ts:783-791). Regression: planV2.test.ts:147-157 mutates compiled/index.html after freeze and asserts throw before publish; planV2.test.ts:262 pins the "v2 subset integrity, not whole-v1 hash" semantics. |
| Important #1 | Pressure claim uncommitted; planSizeCap.test.ts untouched |
Addressed | planSizeCap.test.ts:150-173 — capped plan() throws typed PLAN_TOO_LARGE; planV2() completes the same render on the same input. Matches the PR-body promise verbatim. |
| Important #2 | Bare as casts at JSON boundaries in planV2.ts |
Addressed | parsePlanVideosJson (planV2.ts:280-310), parseChunkSlices (planV2.ts:312-332), and readPlanProtocolV1(v1Plan) (planV2.ts:412) narrow every read boundary. Zero bare casts remain in the new file. Malformed-metadata regression at planV2.test.ts:206-231. |
| Important #3 | rebuildExtractedFramesFromPlanDir sparse-index pivot behavior-affecting and undisclosed |
Addressed | Signature now explicit — `indexMode: "dense-v1" |
| Nit #1 | PlanV2Result drops limitations.videoDependencyMode |
Addressed | PlanV2Result.limitations present on the result type (planV2.ts:99, 105) and set at planV2.ts:524. Result-level assertion at planV2.test.ts:198. |
| Nit #2 | full-source-pack fallback untested |
Addressed | planV2.test.ts:188-204 covers the metadata-absent branch and pins every frame as reachable from every chunk. |
| Nit #3 | fps collapse silently drops fpsDen |
Addressed via fail-closed | readV1PlanFps (planV2.ts:556-561) rejects any fpsDen !== 1 before canonization; regression at planV2.test.ts:233-237. Manifest continues to store fps as an integer, but now provably. |
| Root-cause | Partial — v2 transport bypass without a pressure regression | Newly closed under pressure | planSizeCap.test.ts:150 is the pressure regression, and plan.ts:341-345 doc comment now points at planV2() as the escape hatch instead of "fall back to in-process only". |
New adversarial pass on delta
Read every changed hunk with fresh eyes — nothing worth flagging.
- Standards.
renderChunk.ts:351,366,367,375still hold pre-existing bare casts, but this PR does not modify them, and my R1 scoped that finding to the newplanV2.ts, which is now clean. - Spec forward. All eight PR-body bullets have on-disk backing: opt-in v2 (
planProtocol.ts:37-45,53-57), source-hash verify (planV2.ts:417), boundary parsing (planV2.ts:280-332,412), exact per-chunk deps + audio isolation (planV2.ts:356-378,466, tests atplanV2.test.ts:159-186), sparse-index v2-only (renderChunk.ts:185-217),PlanV2Result.limitations(planV2.ts:99,524), fractional-fps fail-close (planV2.ts:556-561), direct entry points (planV2Execution.ts). - Precision / round-trip. No new drift. Manifest fps stays 24|30|60 with
fpsDen === 1proven; hash prefix vs schema constant intentional trailing\x00preserved;resultFromManifest(planV2.ts:509-527) now round-tripslimitations. - CodeQL alert on
planV2.ts:166. False positive —openSync(path, "r")opens a caller-supplied artifact path for reading (streaming hash), not creating a temp file. Non-blocking.
Full match with Magi's addressed-in-full stance.
— Via
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Corrected exact-head verdict at 7d1c9f64fb464001a5bcdf4557f1c74a1193a68d.
My preceding R2 request over-scoped adapter retry semantics onto this producer-only protocol slice. The PR explicitly defers AWS integration to #2789 and does not enable v2 in either distributed adapter by itself, so plain v2 integrity errors cannot burn an adapter retry budget from this PR alone. The v2 branch's skipped aggregate v1 hash is also intentional: a target-materialized subset cannot reproduce the whole v1 tree hash, and validatePlanV2MaterializedTarget() correctly substitutes per-artifact verification.
All in-scope R1 findings are resolved with regression coverage:
- frozen v1 source bytes are re-hashed before conversion (
planV2.ts:417-424;planV2.test.ts:147-159) - the motivating capped-v1 → successful-v2 pressure case is committed (
planSizeCap.test.ts:150-181) - untrusted JSON boundaries now parse
unknownand validate/narrow - dense v1 vs sparse v2 frame indexing is explicit (
renderChunk.ts:188-216) PlanV2Result.limitations, full-source fallback, and unsupported fractional FPS are test-locked
Carry-forward for #2789: once AWS actually enables v2, deterministic manifest/materialization failures need a terminal adapter error contract; the current #2789 draft should be checked specifically for malformed-manifest and post-materialization corruption classification rather than only S3 digest mismatch.
Verification: producer typecheck passed; focused v2 / plan-size / frame-rebuild tests passed 26/26; diff check passed. Required CI is still finishing, with no failing required check at this review.
Verdict: APPROVE
Reasoning: the producer-only v2 protocol slice now satisfies its conversion, validation, compatibility, and regression contracts; adapter retry classification belongs to the stacked adapter rollout where v2 becomes reachable.
— Magi
7d1c9f6 to
1ac8a30
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Head: 1ac8a30da0e4ff94c202efd4d60e5968ceebc538
Delta base: 7d1c9f64fb464001a5bcdf4557f1c74a1193a68d (my R2 approve)
R3 delta re-review after James restacked. Local diff between the two SHAs is 8 files / +200 / -74, all inside packages/producer/src/services/distributed/**. Every one of the 5 delta bullets stated in the PR body lands, the sister-adapter classifiers on #2789 and #2790 already consume the new typed surface as terminal, and no new as casts or ! non-null assertions were introduced in production code.
R3 delta verification
A. PlanV2IntegrityError / PLAN_V2_INTEGRITY_UNRECOVERABLE — class + code declared at packages/producer/src/services/distributed/planV2.ts:47-64, re-exported from packages/producer/src/index.ts:147,155 and packages/producer/src/distributed.ts:65,67, asserted by publicExports.test.ts:55-57,123-127. Every prior generic throw new Error("[planV2] …") in the delta was converted (26 conversion sites verified by inspection of the diff between 7d1c9f64 and 1ac8a30da). Adapter wiring: AWS state machine NON_RETRYABLE_PLAN/CHUNK/ASSEMBLE at HyperframesRenderStack.ts:206-207, 221-222, 232-233 (#2789 head 5e12d68) — both the string code AND the class name land in all three per-stage lists. GCP NON_RETRYABLE_ERROR_NAMES at server.ts:906,914 (#2790 head b6664db) — same belt-and-suspenders. Terminal on both sides for tamper/corruption without consuming the retry budget.
B. Secure mkdtemp CAS staging (CodeQL #793) — writeBlob at planV2.ts:434-445 replaces the previous ${destinationPath}.tmp-${process.pid} pattern with mkdtempSync(join(dirname(destinationPath), ".plan-v2-blob-")) + join(temporaryDir, "blob"). POSIX mkdtemp(3) guarantees mode 0o700, so no permission-bit follow-up is needed. Copy → rename is same-filesystem (sibling of the destination) so renameSync remains atomic. finally { rmSync(temporaryDir, {recursive: true, force: true}) } cleans up on both success (empty dir after rename) and failure. Symlink-race is closed — the attacker no longer has a predictable path to pre-plant a link into.
C. Symlink / special-file fail-closed — listFiles at planV2.ts:167-189 walks via readdirSync(dir, { withFileTypes: true }). Dirent.isFile() is lstat-classified (does not follow links), so entry.isFile() === false for symlinks, FIFOs, sockets, block/char devices — all fall to the else and throw PlanV2IntegrityError("unsupported filesystem entry …"). This is the single ingress point for all v1→v2 artifact enumeration, so every artifact read that later flows through sha256File is guaranteed to be a regular file. Test coverage at planV2.test.ts:181-192 (Linux-only, symlinkSync under compiled/).
D. Sparse-index validation — listVideoFramePaths at planV2.ts:245-267 now enforces (a) regex-derivable numeric suffix, (b) Number.isSafeInteger(oneBasedFrameNumber) && oneBasedFrameNumber >= 1, (c) !framePaths.has(frameIndex) de-dup. Gaps and out-of-order indices remain allowed by construction — sparse selection is the point of v2 materialization, and downstream FrameLookupTable evaluation catches "index beyond frame count" at lookup time. Zero-based filename regression at planV2.test.ts:194-203.
E. Shared audio path — PLAN_AUDIO_RELATIVE_PATH = "audio.aac" declared once at shared.ts:31-38, consumed by (i) the v1 writer at plan.ts:1011, (ii) v2 artifact classification artifactTargets at planV2.ts:219 returning { chunks: [], assembler: true } — chunks pay zero audio-bytes, and (iii) v2 materialization audioPath computation at planV2.ts:805-808. Audio is written once by the plan stage, content-addressed via the same writeBlob path (hash-verified on materialization by verifyBlob at planV2.ts:747-761), and only exposed to the assembler role. No per-chunk duplication.
Adversarial lenses
Silent-catch / error-invariant. No catch in the delta swallows a PlanV2IntegrityError. readJsonFile at planV2.ts:145-153 explicitly rethrows as PlanV2IntegrityError — a SyntaxError on manifest / marker / videos / chunks / v1-plan JSON is now typed and terminal. Confirmed all in-file JSON reads route through this helper (only readFileSync occurrence in the file is line 146, inside the helper).
Concurrency. Two producers hashing the same blob is safe: existsSync(destinationPath) fast-path returns; if they race past the check, each has its own mkdtemp'd staging dir, and rename(2) is atomic — the second writer overwrites with identical bytes (same sha256). Cleanup in finally handles both branches.
Parity between v1 and v2 execution. renderChunk.ts gates on v2Manifest === null: v1 recomputes the fingerprint from disk; v2 trusts plan.planHash because materialization already hash-verified plan.json via validatePlanV2MaterializedTarget at line 353. Ordering: JSON.parse → readPlanProtocolV1 → validate. If someone tampers plan.json between materialization and the v2 hash check, recomputedPlanHash !== plan.planHash at line 439 (unchanged) still fires — belt and suspenders.
Extracted-frame indexMode split. rebuildExtractedFramesFromPlanDir at renderChunk.ts:185-224 gains indexMode: "dense-v1" | "sparse-v2" = "dense-v1". Only the v2 execution wrapper opts into "sparse-v2" (v2Manifest !== null); all pre-existing v1 callers keep the historical sorted-position behavior. Regression at rebuildExtractedFrames.test.ts:200-217 — the extracted! non-null assertions were replaced by a runtime if (!extracted) throw … guard, which is a nice small hygiene improvement inherited by this delta.
Standards + Spec + Precision + Round-trip
Standards. Zero as casts and zero ! non-null assertions in the two new production files (planV2.ts, planV2Execution.ts). Two as Record<string, unknown> casts in planV2.test.ts:41,373 — both are test-only, on JSON tampering fixtures. Delta actually removed four extracted!.framePaths.* non-null assertions in rebuildExtractedFrames.test.ts — improvement.
Spec forward — 5/5 bullets landed. (i) typed error + code exported and consumed by both sister adapters as terminal ✅ (ii) mkdtemp CAS with POSIX-0700 mode ✅ (iii) fail-closed symlink/special-file rejection ✅ (iv) sparse-index safe-integer + positivity + de-dup ✅ (v) PLAN_AUDIO_RELATIVE_PATH shared constant across writer + v2 manifest + materializer ✅.
Spec reverse — no hidden surface. Everything in the diff maps to one of the five bullets or the "regression coverage" bullet. readJsonFile is a genuinely useful side-effect of bullet (i). Chunk-index duplicate detection at parseChunkSlices:363-368 was already in R2 (not a delta addition — pre-existing).
Precision. No new numeric constants. PLAN_V2_HASH_PREFIX unchanged. PLAN_AUDIO_RELATIVE_PATH = "audio.aac" is a string literal, no numeric consistency concern.
Middle-man. No new callback signatures. readJsonFile(path, label) is a simple two-parameter helper; the label is used only for the error message. writeBlob retains its (sourcePath, destinationPath) shape.
Round-trip. PlanV2IntegrityError → .name === "PlanV2IntegrityError" (set in constructor) → matches AWS + GCP terminal lists. .code === "PLAN_V2_INTEGRITY_UNRECOVERABLE" — also in both lists. Both name AND code catch a raw-code re-throw. The readonly code field carries fallow-ignore-next-line unused-class-member — correct hint because OSS producer does not read .code, only adapter consumers do; verified by grep against the two sister PR heads.
Minor observations (non-blocking, not gating this approval)
-
renderChunk.ts:351still uses bareJSON.parse(readFileSync(planJsonPath, "utf-8")) as PlanJson— pre-existing v1 code, unchanged in this delta. Under v2 execution a corruptedplan.jsonwould surface asSyntaxErrorrather thanPlanV2IntegrityError. Materialization hash-verifiesplan.jsonbefore it lands, so the drift window requires post-materialization tampering. Not worth a follow-up unless a future PR touches this line for other reasons. -
Sparse-index validation catches negatives, non-integers, and duplicates but does not enforce an upper bound (
frameIndex < frameCount). DownstreamFrameLookupTableevaluation catches this at lookup time. Acceptable.
No P1 / P2 / P3 blockers on the delta.
— Via
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Fresh exact-head review at 1ac8a30da0e4ff94c202efd4d60e5968ceebc538.
The R3 delta closes the remaining producer-side integrity gaps cleanly:
packages/producer/src/services/distributed/planV2.ts:49-65gives every deterministic v2 validation/materialization failure a stablePlanV2IntegrityErrorclass andPLAN_V2_INTEGRITY_UNRECOVERABLEcode.packages/producer/src/services/distributed/planV2.ts:177-188,248-265now fails closed on symlinks/special files, zero-based/unsafe frame indexes, and duplicate extracted-frame indexes.packages/producer/src/services/distributed/planV2.ts:438-450replaces the CodeQL-flagged predictable temporary filename with a privatemkdtempstaging directory and guaranteed cleanup.packages/producer/src/services/distributed/shared.ts:34-40centralizes the plan audio path used by both the v1 writer and v2 transport.
I re-read the full R3 delta from the previously approved 7d1c9f64 head and checked the public exports and regression coverage. No new code blocker found.
Verification: producer typecheck passed; focused plan-v2, frame-rebuild, and plan-size tests passed 30/30; git diff --check passed. The current main build/unit/integration/lint/typecheck checks are green. Exact-head regression shards, Windows, and the fresh JavaScript CodeQL analysis are still running; the visible CodeQL alert instance is attached to an earlier merge commit, so the new mkdtemp fix still needs that fresh analysis to close it.
Verdict: APPROVE
Reasoning: The exact-head delta establishes the typed terminal integrity contract, removes the insecure temp-file construction, and fail-closes the remaining filesystem/frame-index drift with regression proof.
— Magi
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 1ac8a30da0e4ff94c202efd4d60e5968ceebc538 — delta from 7d1c9f64 (my R2).
LGTM at R3. Every carry-forward from my R2 body is addressed cleanly:
PlanV2IntegrityErrortyped class +PLAN_V2_INTEGRITY_UNRECOVERABLEcode atplanV2.ts:49-64, exported throughdistributed.ts:65-67andindex.ts:147-155so both adapters can classify by either the class name (.name) or the code string.- All 54 deterministic v2 throws are now typed — zero
throw new Error(...)remaining inplanV2.ts, verified by grep. - CodeQL #793 (predictable temp-path) closed at
planV2.ts:438-449—writeBlobnow usesmkdtempSync(".plan-v2-blob-")instead of the${destinationPath}.tmp-${process.pid}suffix. - Symlink + special-file rejection at
planV2.ts:180-183with typed error; regression test atplanV2.test.ts:182-192covers a realsymlinkSyncfixture. My R2 nit onHASH_EXCLUDED_PLAN_FILES+marker-file rollback isn't addressed but the rollback semantics remain fail-closed-and-safe, so still non-blocking. - Zero-based / duplicate frame-index rejection at
planV2.ts:255-262(Number.isSafeIntegerguard + duplicate-in-outputDir check); test atplanV2.test.ts:200-210locks in the "must use a 1-based safe integer" invariant. Nice defensive layer on the sparse-index pivot. readJsonFilehelper atplanV2.ts:145-154wraps everyJSON.parseboundary so malformed JSON throws typedPlanV2IntegrityErrorwith a${label} is not readable JSONmessage. Every prior bareJSON.parse(...)inplanV2.tsnow flows through it (v1 plan.json,videos.json,chunks.json, manifest, marker).- Shared
PLAN_AUDIO_RELATIVE_PATHconstant inshared.ts:38— eliminates the"audio.aac"literal duplication betweenartifactTargetsandmaterializePlanV2Target. - Tests assert both the instance and the code:
planV2.test.ts:170-176and:356-368nowexpect(caught).toBeInstanceOf(PlanV2IntegrityError)+toHaveProperty("code", PLAN_V2_INTEGRITY_UNRECOVERABLE)— locks in the adapter-boundary contract at the producer side. rebuildExtractedFrames.test.ts:200-215: the R2extracted!.framePathsnon-null asserts were replaced withif (!extracted) throw— CONTRIBUTING.md:55 compliance cleanup that I hadn't asked for but was happy to see.
All required CI checks green; regression shards still finishing.
Carry into the stack review: the PlanV2IntegrityError + code should show up in AWS NON_RETRYABLE_{PLAN,CHUNK,ASSEMBLE} and GCP NON_RETRYABLE_ERROR_NAMES on #2789 and #2790 respectively. Will lens for that on each slice.
1ac8a30 to
09fd4ff
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-stamping at 09fd4ff2f.
Delta since my prior LGTM at 1ac8a30da is a single 2-line lgtm[js/insecure-temporary-file] suppression comment above openSync(path, "r") in sha256File (planV2.ts:198-199). Rationale checks out — mode "r" is O_RDONLY without O_CREAT, so the syscall physically cannot create a file, temporary or otherwise; the caller has already passed the regular-file / symlink validation upstream. Narrow, documented at the sink.
Everything from R3 still stands.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head re-stamp at 09fd4ff2f229185f914c678653608224caa35000.
Audited: the complete delta from previously approved 1ac8a30da0e4ff94c202efd4d60e5968ceebc538 (two comment lines at packages/producer/src/services/distributed/planV2.ts:199-200), the reported CodeQL rule/instance, all sha256File() callers, and current review/check state.
The suppression is appropriately narrow. planV2.ts:201 opens an existing caller path with "r" and closes the descriptor in finally at :202-210; it does not create a temporary file. Creation uses the private mkdtempSync staging root at planV2.ts:477-478, while hash callers either originate from listFiles()' regular-file-only traversal (:167-188,486-498) or validate an existing artifact before read (:749-760,838-846). The js/insecure-temporary-file alert's creation recommendation therefore does not apply to this read-only sink, and the inline lgtm[...] rationale is scoped to that exact call.
Independent exact-head verification:
- focused Plan v2 / frame-rebuild / size-cap tests:
28/28 - producer typecheck
git diff --check
Required CI is currently pending rather than failing. Approval is on code merits; merge should remain gated on exact-head required CI and the fresh JavaScript/TypeScript CodeQL analysis.
Verdict: APPROVE
Reasoning: The only delta from the previously approved implementation is a justified sink-local false-positive suppression on a read-only descriptor; the underlying secure staging and artifact-validation behavior is unchanged.
— Magi
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Head: 09fd4ff2f229185f914c678653608224caa35000
Delta base: 1ac8a30da0e4ff94c202efd4d60e5968ceebc538 (my R3 approve)
Re-verification at exact head. Byte-diff between R3 and current head is a single 2-line insertion inside sha256File (planV2.ts:199-200) — every other file in the PR is identical (SHA-verified across all 14 changed files). This is the narrowest possible restack, and Rames independently re-stamped at the same head with matching reasoning (O_RDONLY without O_CREAT cannot create a file).
Verification of prior R1 findings
| # | R1 finding | Status at 09fd4ff2f |
Evidence |
|---|---|---|---|
| Blocker | Stale/tampered v1 canonized into v2 via unrecomputed sourcePlanV1Hash |
RESOLVED (holds since R2) | packages/producer/src/services/distributed/planV2.ts:469-475 — recomputePlanHashFromPlanDir(planV1Dir) runs before canonization; rejects on mismatch. Regression at planV2.test.ts:147-157. |
| Important #1 | Pressure claim ("v1 caps, v2 completes") uncommitted | RESOLVED (holds since R2) | packages/producer/src/services/distributed/planSizeCap.test.ts:150-173 — capped plan() throws PLAN_TOO_LARGE; planV2() completes the same render on the same input. |
| Important #2 | Bare as casts at JSON boundaries in new file |
RESOLVED (holds since R2) | parsePlanVideosJson (planV2.ts:327), parseChunkSlices (planV2.ts:359), readPlanProtocolV1(v1Plan) (planV2.ts:464) narrow every JSON read. grep '\bas [A-Z]' planV2.ts → 0. |
| Important #3 | Sparse frame-index pivot undisclosed and behavior-affecting | RESOLVED (holds since R2) | rebuildExtractedFramesFromPlanDir has explicit indexMode: "dense-v1" | "sparse-v2" = "dense-v1" (renderChunk.ts:185-217); both modes covered by regression. PR body line 8 discloses. |
| Nit #1 | PlanV2Result drops limitations.videoDependencyMode |
RESOLVED (holds since R2) | PlanV2Result.limitations.videoDependencyMode at planV2.ts:100,103,518. |
| Nit #2 | full-source-pack fallback untested |
RESOLVED (holds since R2) | planV2.test.ts:188-204 covers the metadata-absent branch. |
| Nit #3 | fpsNum→fps collapse silently drops fpsDen |
RESOLVED (holds since R2) | readV1PlanFps (planV2.ts:608-613) rejects any fpsDen !== 1 before canonization. |
CodeQL suppression review
Narrowness. Two-line comment at planV2.ts:199-200:
// lgtm[js/insecure-temporary-file] — read-only open of a caller-owned regular file;
// this call never creates a file, temporary or otherwise.
Applies to openSync(path, "r") at line 201 only. Not wrapped around a wider block. Cites both invariants: caller-owned regular file already validated + O_RDONLY without O_CREAT physically cannot create.
Sibling audit. Grepped every changed file at head for openSync, createReadStream, open( — only match is the sink at planV2.ts:201. Not overapplied (no writable-mode sites), not underapplied (no other read-mode sinks needing the same treatment).
Regular-file validation upstream at every call site.
createPlanV2FromV1(planV2.ts:496):file.absolutePathcomes fromlistFiles(planV1Dir)which walks viareaddirSync(dir, { withFileTypes: true })and only pushes entries whereentry.isFile()(lstat-classified, does NOT follow symlinks). Every symlink/FIFO/socket/device throwsPlanV2IntegrityError("unsupported filesystem entry …")atplanV2.ts:181.verifyBlob(planV2.ts:758): explicitstats.isFile()atplanV2.ts:755.validatePlanV2MaterializedTarget(planV2.ts:845): size-only pre-check viastatSync; symlink of matching size could reachsha256File. Content-hash gate catches substitution downstream (see TOCTOU).
Descriptor never written to. openSync(path, "r") → readSync loop → closeSync. grep -nE 'writeSync|write\(|ftruncate' planV2.ts → 0. path not reassigned between validation and open.
Residual TOCTOU (P3, non-blocking). Small window between classification/stat and sha256File where local-FS-write attacker could swap regular file for symlink. Not exploitable: staging dir is caller-owned; content-hash gate downstream catches content substitution. In caller 3 the size-only pre-check would let a matching-size symlink through, then hash comparison catches. Hygiene only.
Fresh 4-lens pass
Standards
Clean in the delta. grep '\bas [A-Z]' planV2.ts → 0; grep '\w!\.' planV2.ts → 0. Pre-existing bare casts at renderChunk.ts:351,366,367,375, assemble.ts:124,130, plan.ts:615 are on v1 paths, byte-identical to R3 and to main-ancestor — out of scope.
Spec forward-check
Clean. All 8 PR-body bullets retain delivery evidence unchanged since R3.
Spec reverse-check
P3 note only. CodeQL suppression addition is not mentioned in the PR body; the commit message discloses it clearly and it's pure-comment. Not worth blocking.
Sibling-precision divergence
Clean. No new numeric constants in the delta.
Middle-man wrap/unwrap
Clean. sha256File(path) is a (path: string) => string primitive — no object-literal round-trip in the delta.
Behavior + editor-UI lens complement
Silent-catch + error-invariant
Clean. No catch swallows PlanV2IntegrityError. AWS state-machine terminal classification (per #2789) and GCP NON_RETRYABLE_ERROR_NAMES (per #2790) both consume the error name + PLAN_V2_INTEGRITY_UNRECOVERABLE code as terminal — no retryable-vs-terminal drift between adapters.
Concurrency/lifecycle
Clean. mkdtempSync(...) staging preserves POSIX 0o700 + same-filesystem atomic renameSync. Cleanup on failure via finally { rmSync(..., { recursive: true, force: true }) }. Two producers hashing same blob race safely.
PR-body-vs-diff parity
Clean modulo the reverse-check P3 above.
Perf audit
Clean. sha256File O(n) in bytes with fixed 1 MiB Buffer.allocUnsafe — bounded memory. CodeQL suppression preserves this property.
Verdict
APPROVE. All prior R1 findings resolved and holding, CodeQL suppression is narrow and correctly rationalized at the sink with both invariants cited, sibling audit finds no other openSync/createReadStream sites needing the same treatment, and the 4-lens + behavior-lens sweep at exact head surfaces only P3 hygiene (residual TOCTOU + PR-body-not-updated-for-comment) — neither gates. Peer-match with Rames's re-stamp at the same head.
— Via
5 participants
Summary
plan.json,meta/videos.json, andmeta/chunks.jsonat their JSON boundariesPlanV2Resultand fail closed on unsupported fractional-fps inputsrenderChunkV2/assembleV2entry points and fail closed when v1 readers receive a v2 rootWhy
Plan v1 packages the whole render working set into one archive and enforces a transport-size ceiling. Plan v2 removes that monolithic transport boundary so workers can materialize only their target artifact set.
This is protocol plumbing, not a blanket production rollout. The current planner still stages a complete v1 plan locally before converting it, and
compiled/**is still shared by every chunk. Those limits are called out for the rollout follow-up.Validation
PLAN_TOO_LARGE;planV2()completes the same renderStack
Base:
mainThe AWS Lambda adapter, live AWS parity test, and smoke-test safety work are in the next stacked PR.