Repository navigation
feat(producer): fail a render whose variables the page does not declare - #7548
Merged
Merged
Conversation
Contributor
… for the variables check
Contributor
There was a problem hiding this comment.
Approving at 7194a5fd. I found no blockers.
What I checked:
- Off by default.
strictVariablesreachesinitializeSessiononly when the caller asks for it. The server body and the distributed config reject a value that is not a boolean, andparseRenderOverridespasses on onlytrue. 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
getDeclaredVariableSourcesis reported asdeclarations-unreadable, never asundeclared. The probe stage rethrows the error, becauseVariablesInvalidErroris 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.
getVariablesand the check both usetopLevelDeclarers(), and the check reusesreadCompositionVariablesandvalidateVariables. - No values echoed.
toIssuedropsallowedandactualfromenum-out-of-range. Intype-mismatch,actualis only the result ofjsTypeOfor 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,
strictVariablesAttestationreportsfalse. The probe browser is forced on when there are variables to check. TheonPlannedcallback keeps the result out of the manifest's content address. - Fails without the fix.
strictVariablesSession.test.tsruns in real Chromium and is now inintegration-tests.json. Without the call ininitializeSession, 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.
Merged
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A render can now ask for its variables to be checked against the page's own
data-composition-variablesdeclarations. 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 withVARIABLES_INVALID, naming the variable. It is off by default: renders that do not ask for it behave exactly as before.How it works
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.getVariables()and the newgetDeclaredVariableSources()share onetopLevelDeclarers():<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 withreadCompositionVariables(the same predicateparseCompositionVariablesnow wraps), and validates with the existingvalidateVariables. No second copy of any rule.VariablesInvalidError: codeVARIABLES_INVALID, user-owned, not retryable. Its public metadata holds at most 10 issues (undeclared,type-mismatchwith 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.strictVariablesCheckedon 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.planV2WithPublisherreports it through an optionalonPlannedcallback, not the manifest, so plan content addresses do not depend on it.strictVariableson the capture options, the render request, the producer server request body (validated as a boolean) and the distributed config.Limits, stated
Checked
executeRenderJobon a small project (not committed): a declared variable completes and reportsstrictVariablesChecked: true; an undeclared or mistyped one fails in about 350 ms in the browser probe withVARIABLES_INVALID; empty variables complete and report true; a render that does not ask is unchanged.src/runtime: 129 files, 3,014 tests. enginesrc: 3,132 pass; the other 18 arebrowserManager.test.ts(needsbunon PATH) andffprobe.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 perbun:testfile 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 --noEmitis clean in parsers, core, engine and producer, andoxfmt --checkandoxlintare clean on every changed file.src/services/strictVariablesSession.test.ts(real Chromium, integration lane). With the call ininitializeSessionremoved, 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.