Skip to content

feat(producer): fail a render whose variables the page does not declare - #7548

Merged
miguel-heygen merged 2 commits into
mainfrom
feat/render-variables-check
Nov 11, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
feat/render-variables-check

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

A render can now ask for its variables to be checked against the page's own data-composition-variables declarations. When a variable is not declared, has the wrong type, or the declarations cannot be read, the render stops before any frame is captured and fails with VARIABLES_INVALID, naming the variable. It is off by default: renders that do not ask for it behave exactly as before.

How it works

  • Where the check runs. In initializeSession, right after the page's ready poll (both capture modes). It reads the declarations from the live page, so it sees the same DOM the composition does.
  • One scope rule. getVariables() and the new getDeclaredVariableSources() share one topLevelDeclarers(): <html> first, then every other declarer that is not inside a sub-composition's scope; a later declarer wins a duplicate id. The check reads the raw attribute text through the page's runtime, parses it with readCompositionVariables (the same predicate parseCompositionVariables now wraps), and validates with the existing validateVariables. No second copy of any rule.
  • Fails closed. Invalid JSON, a payload that is not an array, a malformed entry, or a runtime that cannot list its declarations is reported as unreadable, never as "not declared". Empty variables need no check.
  • The error. VariablesInvalidError: code VARIABLES_INVALID, user-owned, not retryable. Its public metadata holds at most 10 issues (undeclared, type-mismatch with the two type names, enum-out-of-range, declarations-unreadable) and the total. Names are cut to 64 characters with control characters replaced; no value the caller supplied is carried. The code is on the producer's safe-error allow-list, so the existing envelope carries code, owner, retryability and metadata.
  • Reporting that it ran. Because an older producer would silently ignore the new option, the result says whether this producer honoured it: strictVariablesChecked on the render responses and on the distributed plan result (true only if the loaded page was checked, or there were no variables to check; absent when strict variables were not asked). The probe stage now launches its browser when strict variables need checking. planV2WithPublisher reports it through an optional onPlanned callback, not the manifest, so plan content addresses do not depend on it.
  • Options. strictVariables on the capture options, the render request, the producer server request body (validated as a boolean) and the distributed config.

Limits, stated

  • The check reads the declarations once the page is ready. Page scripts have already run by then (variables are injected before them, as today), so a script that rewrites the declaration attribute later is not covered.
  • Chunk workers do not repeat the check; the plan step does it once.
  • A sub-composition's declarations are its own scope and are not checked against the render's variables.
  • Entries the existing parser drops as malformed make the declarations unreadable for this check; the lint pass remains the place that explains why.

Checked

  • Real Chromium, end to end through executeRenderJob on a small project (not committed): a declared variable completes and reports strictVariablesChecked: true; an undeclared or mistyped one fails in about 350 ms in the browser probe with VARIABLES_INVALID; empty variables complete and report true; a render that does not ask is unchanged.
  • parsers: 58 files, 1,485 tests. core src/runtime: 129 files, 3,014 tests. engine src: 3,132 pass; the other 18 are browserManager.test.ts (needs bun on PATH) and ffprobe.test.ts (needs an LFS PNG fixture); with both present, those two files pass 248 of 248. producer: every touched file in both lanes passes, one process per bun:test file as the lane runner does, and the test classification check is clean. The parsers, lint, studio-server, core and engine builds (including declaration output) pass. tsc --noEmit is clean in parsers, core, engine and producer, and oxfmt --check and oxlint are clean on every changed file.
  • fails without the fix: src/services/strictVariablesSession.test.ts (real Chromium, integration lane). With the call in initializeSession removed, its three refusal tests fail ("refuses a variable the page does not declare, naming it, before any frame", "refuses a value of the wrong type without echoing it", "refuses when the page's declarations cannot be read, not as undeclared"). Unit tests: variablesCheck.test.ts (rules, bounds, no values echoed), getVariables.test.ts (declaration scope), compositionVariables.test.ts (readCompositionVariables), strictVariablesProbe.test.ts (browser need, attestation), renderRequest.test.ts, server.errorCode.test.ts.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 267 (base branch 267), smooth 108 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 7194a5fd. I found no blockers.

What I checked:

  • Off by default. strictVariables reaches initializeSession only when the caller asks for it. The server body and the distributed config reject a value that is not a boolean, and parseRenderOverrides passes on only true. A render that does not ask runs exactly as before.
  • Fails closed. Invalid JSON, a payload that is not an array, an entry the parser drops, or a runtime without getDeclaredVariableSources is reported as declarations-unreadable, never as undeclared. The probe stage rethrows the error, because VariablesInvalidError is not a transient browser error, so the render stops before the probe retries or captures any frame. The screenshot-mode relaunch runs the check again on the new session.
  • Same scope rule. getVariables and the check both use topLevelDeclarers(), and the check reuses readCompositionVariables and validateVariables.
  • No values echoed. toIssue drops allowed and actual from enum-out-of-range. In type-mismatch, actual is only the result of jsTypeOf or a fixed string. IDs are cut to 64 characters and control characters are replaced.
  • Reporting. If the producer must check in a browser but the probe session never checked, strictVariablesAttestation reports false. The probe browser is forced on when there are variables to check. The onPlanned callback keeps the result out of the manifest's content address.
  • Fails without the fix. strictVariablesSession.test.ts runs in real Chromium and is now in integration-tests.json. Without the call in initializeSession, its three refusal tests would resolve instead of rejecting.
  • CI at this head. CI (including Fallow), regression, Windows render, Comments, CodeQL and the captures all pass.

Nit, not blocking: in probeStage.ts, checksVariablesInBrowser and strictVariablesAttestation were inserted between the long probeRequiresBrowser JSDoc and probeRequiresBrowser itself. That doc now sits on top of checksVariablesInBrowser, as a second JSDoc block above its own one-line doc. Moving the two helpers above that doc block would fix it in a follow-up.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 11, 2026
Merged via the queue into main with commit f312fca Nov 11, 2026
99 of 100 checks passed
@miguel-heygen
miguel-heygen deleted the feat/render-variables-check branch November 11, 2026 09:04
@miguel-heygen miguel-heygen mentioned this pull request Nov 11, 2026
3 of 4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant