fix(core): make timing masks safe for source loaders

[jrusso1020]

What

Make timing compilation mask non-HTML source regions before rewriting timing attributes.

Why

Source-loader scripts and embedded text can contain attribute-like strings that must never be mutated as markup.

How

Restrict timing rewrites to real element tags and add adversarial source-loader fixtures.

Test plan

• core timingCompiler unit tests; strict core typecheck
• Stack-wide lint, format, build, typecheck, and relevant integration gates

[jrusso1020]

• refactor(repo): resolve changed-code audit #2174
• test(engine): isolate peer abort regression #2173
• refactor(cli): decompose render phases #2172
• refactor(cli): centralize process lifecycle #2171
• fix(cloudrun): publish adapter contract #2170
• test(repo): execute packed subpaths #2169
• refactor(repo): centralize package subpaths #2168
• refactor(core): own edit protocol contract #2167
• test(compiler): enforce preview render parity #2166
• refactor(studio): gate SDK operation families #2165
• refactor(studio): gate Acorn writer migration #2164
• refactor(producer): make capture plans immutable #2163
• refactor(producer): unify render requests #2162
• refactor(engine): type capture failures #2161
• refactor(engine): manage child process lifecycles #2160
• refactor(engine): add fingerprinted browser leases #2159
• refactor(render): centralize execution ownership #2158
• refactor(core): unify composition contract #2157
• fix(studio): enforce optimistic file concurrency #2156
• fix(studio): make sdk cutover transactional #2155
• fix(render): publish artifacts atomically #2154
• fix(render): surface structured outcomes #2153
• fix(player): own connection and media resources #2152
• fix(core): preserve runtime transport contract #2151
• fix(player): version runtime protocol #2150
• test(repo): verify packed consumers #2149
• test(repo): enforce workspace contracts #2148
• fix(core): enforce strict runtime safety #2147
• test(producer): gate source tests by execution lane #2146
• fix(core): make timing masks safe for source loaders #2145 👈 (View in Graphite)
• chore(repo): forbid tracked generated artifacts #2144
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[jrusso1020]

🤖 Automated code-review pass — issues to address

• packages/core/src/compiler/timingCompiler.ts:100 — restore now does restored.replaceAll(token, region) with region as a string replacement, so String.prototype.replaceAll interprets $-patterns inside the masked content. The old code used a function replacement ((_, i) => stash[Number(i)] ?? ""), which returned the region verbatim and was immune. This is a regression that hits exactly the case this PR targets: a masked <script>/<style> containing a regex replacement string. Concrete failures (verified): a source-loader script with "$&" restores such that the $& expands to the mask token itself — it both corrupts the script and leaks the raw �HFMASK0� token (NUL bytes included) into the compiled HTML; "$$" collapses to "$"; $`/$' get replaced with the surrounding text. The new idempotency test only passes because its fixtures contain no $. Fix: use a function replacement, e.g. restored.replaceAll(token, () => region). [CONFIRMED]

Checked: the incremental diff (NUL-delimiter change + reduce/replaceAll rewrite of restore), both maskInertRegions call sites (compileTimingAttrs restores; extractResolvedMedia only uses .masked, so it's unaffected), and re-compile/idempotency behavior. The NUL delimiter and the inert-region masking approach itself are sound; only the string-vs-function replacement is the problem.

[jrusso1020]

Valid

[miga-heygen]

#2145 — fix(core): make timing masks safe for source loaders

Verdict: LGTM 🟢

Test-only PR, 16 lines. The new test proves idempotency of compileTimingAttrs when source HTML contains JavaScript replacement-pattern specials ($&, $$, $\`, $') inside <style> and <script> blocks. Double-compiling preserves inert content verbatim and doesn't leak HFMASK placeholders.

Covers a real edge case — String.prototype.replace treats those tokens as back-references; the mask/unmask round-trip must be transparent to them.

Review by Miga

[miguel-heygen]

Final direct pass at current head 38d62f7. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.

[james-russo-rames-d-jusso]

Reviewed at 38d62f77.

Test-only pin for the existing maskInertRegions mechanism in timingCompiler.ts:91. The new re-compilation idempotency + dollar-sequence fixture at timingCompiler.test.ts:171 is a real guard, not enshrined defense — I traced the internals:

• Every internal .replace(...) on masked HTML uses a function as the 2nd arg (lines 93, 100, 176, 183, 191, 226), so String.prototype.replace's $&/$$/$`/$' substitution rules don't fire on the masked content. The only string-arg replacements (line 78 injectAttr, lines 300/305 injectDurations) receive programmatic numeric/token values, not user content, so dollar substitution can't land untrusted characters there either.
• The mask itself uses �HFMASK<n>� NUL-delimited tokens — the comment at timingCompiler.ts:87 says the raw 0x00 bytes previously "shipped once and broke every render (issue #2139)", so the � escape form is load-bearing to survive Bun's transpiler. The new test asserts no HFMASK residue after two passes, which catches any future accidental reintroduction.
• Assertion set is precise: (1) <style> content survives verbatim, (2) <script> content survives verbatim, (3) real <video class="hero"> still gets data-end="2" injected, (4) no HFMASK residue. All four exercise actual observable behavior — this isn't a .toHaveBeenCalled style over-mock.

One question on scope: the PR body says "Restrict timing rewrites to real element tags and add adversarial source-loader fixtures." The diff only does the second half — the "restrict to real element tags" mechanism (i.e., maskInertRegions) already lives at timingCompiler.ts:91-102 on the base. If the restriction landed in an earlier PR of the 31-stack, the body copy is fine as a stack-level narrative; if not, the "restrict" verb is claiming work that isn't in this diff. Either is easy to clarify.

LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Verdict: 🟡 LGTM-with-title-nit at 38d62f7

Endorses Miga + Rames-D — the double-compile round-trip + String.prototype.replace dollar-token fixtures at timingCompiler.test.ts:172-186 are a real lock-in of an existing invariant (maskInertRegions already uses callback-form replace at timingCompiler.ts:93,100,176,183,191,226, which sidesteps $-substitution). Adding one calibration note the prior reviewers didn't surface.

Findings

F1. Conventional-commit type drift — nit, non-blocking. Diff is one file, timingCompiler.test.ts +16/-0. Zero production LOC. The production fix (maskInertRegions + HFMASK\d+ NUL delimiters) already exists on the base branch 585aa9f and was actually shipped in #2140. fix(core): conventional-commit type implies a user-facing bug fix visible in changelogs; accurate type here is test(core):. Semantic-PR-title check passes on format alone, so this isn't a merge blocker — but a title fix via REST PATCH /repos/{owner}/{repo}/pulls/{number} keeps release-note tooling honest. Miguel's PR-envelope lens (feedback_hf_review_scope_pr_envelope.md in memory) treats this drift as request-changes on principle; I'm calibrating to nit given the fix is one API call away.

F2. PR body overstates scope — nit. Body's "How" says: "Restrict timing rewrites to real element tags and add adversarial source-loader fixtures." The first clause implies production changes; only the second matches the diff. Reads cleaner as: "Add adversarial round-trip test for existing inert-region masking (#1940, #2140) — no production change."

F3. Comment coverage gap in the fixture set — nit. INERT_REGION_RE at timingCompiler.ts:87 masks <!--...--> HTML comments in addition to <script> and <style>. The new test only exercises dollar-token fixtures inside script+style. Existing it("ignores media tags mentioned inside comments (issue #1938)") covers comment-with-media at base line 143 but not comment-with-dollar-tokens. Not blocking — callback-form replace is content-agnostic, so the invariant transitively holds — but a <!-- $&$$ --> fixture would lock the comment path in symmetrically.

Adversarial pass

Initial pull was to raise F1 as request-changes under strict fix(core): reading. Dropped to nit because (a) format check will pass, (b) fix: for a regression test locking a real prior fix is defensible-adjacent, (c) the fix is one REST PATCH away with no rebase risk. Considered dropping F2 as noise; kept because scope-drift signals in the body compound with F1 in the changelog. F3 initially felt load-bearing; adversarial pass demoted it because callback-form replace is content-agnostic and the transitive proof is trivial.

R1 by Via

The base branch was changed.

[miguel-heygen]

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

Reviewed the final head against its actual parent 45dfcd58. This boundary is exactly the 16-line regression test; it does not smuggle production changes from the wider stack.

packages/core/src/compiler/timingCompiler.test.ts:172-185 now pins the dangerous replacement-string sequences ($&, $$, $\``, $'`) inside both script and style regions, recompiles the output, verifies real media timing still compiles, and asserts no mask residue. I also verified the parent already restores masks with callback-form replacement, so the test is guarding the intended existing fix rather than passing over string-replacement corruption.

Verification: all 25 timing-compiler tests passed locally; all 48 final-head GitHub check runs completed without failure. The previously raised test-only title/body nits remain metadata observations, not code defects.

Verdict: COMMENT (post-merge: Ready)
Reasoning: The own-diff boundary is test-only and the adversarial fixture directly proves the source-loader corruption class stays closed.

— Deepwork