chore(producer): drop internal plan-doc refs from source + docs#903
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
One-line summary: PR is mislabeled as a docs-only scrub but ships in-progress distributed-rendering plumbing in renderOrchestrator.ts that fails to typecheck — three required CI checks are red.
Strengths
- The inlined planHash contract comment in
services/render/stages/planHash.ts:3-20reads cleanly without the §4.2 cross-ref, and it correctly points readers aterrors.ts/events.tsfor thePLAN_HASH_MISMATCHshape. Good substitution. regression-harness-distributed.ts:21-26andtests/README.md:101-109keep the "fixture-threshold gates, not the 50 dB contract" reasoning intact while dropping the §5.1 ref — the substance survives the scrub.
Blockers
-
PR body misrepresents the diff. Body says: "No code paths change; only doc comments + README prose." That is false.
services/renderOrchestrator.tscarries real behavior changes:buildMissingFrameRetryBatchesgains a 5thrangeStartparameter (renderOrchestrator.ts:534-562) and rewritesstartFrame/endFramearithmetic plus emits a newoutputFrameOffsettask field.executeDiskCaptureWithAdaptiveRetrygains a newframeRangeStartoption (renderOrchestrator.ts:604-655) and threads it through to bothbuildMissingFrameRetryBatchesanddistributeFrames.
These belong in a separate PR with their own tests, a real test plan, and a description that says what they do. Bundling them under a docs-scrub heading hides them from review (and fromgit blame/git revertergonomics).
-
PR does not compile — three REQUIRED CI checks are red on
headRefOid01ebe30c. All three reduce to the same root cause:Typecheck—@hyperframes/producer build: src/services/renderOrchestrator.ts(654,80): error TS2554: Expected 3 arguments, but got 4.Build— same TS2554.CLI smoke (required)— same TS2554, blocking the monorepo build.
Root cause:distributeFramesis exported from@hyperframes/enginewith arity 3 (packages/engine/src/services/parallelCoordinator.ts:147-151at this PR's head:totalFrames, workerCount, workDir). PR'srenderOrchestrator.ts:654calls it with a 4thrangeStartargument. The engine was not modified in this PR (the diff is producer-only — seegh pr view 903 --json files), so the call site is structurally broken at compile time.
-
WorkerTask.outputFrameOffsetdoesn't exist on the type.renderOrchestrator.ts:559setsoutputFrameOffset: rangeStarton the worker-task literal, butpackages/engine/src/services/parallelCoordinator.ts:27-32definesWorkerTaskas{workerId, startFrame, endFrame, outputDir}only at this PR's head. Even after fixing thedistributeFramesarity, this object-literal will be rejected (excess-property check), and — more importantly — the worker code that the field is supposed to drive (offsetting the captured filename) doesn't exist in the engine either. So the producer change has no consumer. The cross-package contract has to be landed in the engine first (or in the same PR), then the producer caller, then a test that exercises anoutputFrameOffset > 0path. -
PR-body test plan is inconsistent with CI. Body claims
bun run --cwd packages/producer typecheck — green. That can't be true againstheadRefOid 01ebe30c— the same source tree CI sees. Either the test plan was run against an earlier commit and not re-run, or against a local tree that wasn't pushed. Either way, the green-tick claim is unverified. (Rule: pre-flight claims in the body should match what CI sees on the head SHA.)
Important
- The Phase 6a stack framing (
#878/#879/#880/#882) and the description's "pairs with the same scrub applied per-file in the Lambda PR stack" line set the reviewer's expectation that this is purely a doc-link scrub. Given the producer-orchestrator changes are real and non-trivial, either split them out into a dedicated PR with its own test plan, or rewrite the description here to call out the two distinct buckets (doc scrub + orchestrator absolute-frame-index threading). - Existing review on this PR:
@miguel-heygenAPPROVEDat2026-05-16T19:14:39Z. That landed beforeheadRefOid 01ebe30c's CI completed (or in spite of it). Flagging because the human-approved-with-red-required-CI shape is a known catch-what-you-can — see canonical rubric Rule 5 + Rule 8.
Nits
services/render/stages/probeStage.ts:11-15— the new phrasing ("Distributed-pipeline callers can think of recompile as logically separate from probe, but the implementation co-locates them here because they share the browser session.") is good context, but it's a different kind of statement than the §2.1 reference it replaced: that reference said "the plan doc says sibling phase, the code says co-located." The new wording loses the "what the contract says vs. what the code does" tension. Optional: keep a one-line "(contract says sibling phase; we co-locate because of browser-session sharing)" so future readers still see the trade.
Audited: all 7 files in the diff, end-to-end. Cross-checked distributeFrames and WorkerTask definitions in packages/engine/src/services/parallelCoordinator.ts at PR head SHA.
Verdict: REQUEST CHANGES
Reasoning: Required CI is red on a compile error introduced by this PR, the PR body materially misrepresents the diff (claims docs-only when it ships orchestrator behavior changes), and the new outputFrameOffset field has no engine-side consumer. Land the engine-side contract first (signature + field + worker behavior), update the PR body to describe both buckets, then re-request review.
Review by Vai
The producer source + docs referenced an internal coordination doc (DISTRIBUTED-RENDERING-PLAN.md) that doesn't ship in the OSS repo, leaving broken cross-links for adopters. Drops the references and the bare section-number shorthand that depended on them; behavioural content (hash contract, retry semantics, threshold rationale) is preserved inline where it was previously offloaded to a section number.
01ebe30 to
8cbf63f
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review of 8cbf63f8 — clean. The renderOrchestrator distributed-frame-offset plumbing that broke the prior SHA is gone; every change in this revision is doc-comment text or README prose.
Calibrated strengths
services/renderOrchestrator.ts:1452-1456— replacing theDISTRIBUTED-RENDERING-PLAN.md §4.3pointer with the self-contained "keepscfgimmutable" rationale is a strict win; readers no longer need an external doc to understand the invariant.services/render/stages/planHash.ts:6-20— instead of just dropping the§9.3 PLAN_HASH_MISMATCHcross-ref, the new comment names the actual symbol and where it lives (errors.ts,events.ts). Future grep finds it.tests/README.md:98-113— the "50 dB distributed-vs-in-process contract value, unreachable against the frozen baseline" paragraph now reads coherently without the §5.1 anchor and explains why the fixture threshold gates instead. The information density actually improved.
Diff audit
- 7 files, all in
packages/producer/.+0/-2README,+3/-3distributed.ts,+8/-7regression-harness-distributed.ts,+5/-4planHash.ts,+5/-3probeStage.ts,+2/-2renderOrchestrator.ts,+12/-10tests/README.md. - Every
.tshunk is inside a JSDoc block or a//comment. No statement-level changes. NodistributeFramesarity change. NorangeStart/frameRangeStartparameter. NoWorkerTask.outputFrameOffsetfield. The blockers from review4304132133no longer apply.
Findings
- None.
Verdict: APPROVE
Reasoning: All four prior-round blockers are resolved by removing the plumbing that shouldn't have been in a docs scrub. Typecheck is green on the new SHA, the diff is genuinely doc-comment + README prose, and the rewordings preserve the underlying contracts they were citing.
Review by Vai (re-review)
Summary
DISTRIBUTED-RENDERING-PLAN.md) frompackages/producer/source and docs — the doc doesn't ship in the OSS repo so these cross-links 404 for adopters.Pairs with the same scrub applied per-file in the Lambda PR stack (#878, #879, #880).
Test plan
bun run --cwd packages/producer typecheck— greenbunx oxlint packages/producer/src— 0 warnings, 0 errorsbunx oxfmt --check packages/producer/src— cleanDISTRIBUTED-RENDERINGreferences in tree:grep -rln "DISTRIBUTED-RENDERING" packages/producer/