Skip to content

fix(lint): check passes valid sub-compositions and scene-relative asset paths - #5337

Merged
miguel-heygen merged 8 commits into
mainfrom
fix/lint-subcomposition-roots
Oct 9, 2026
Merged

miguel-heygen merged 8 commits into
mainfrom
fix/lint-subcomposition-roots

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

What changes for a user

hyperframes check stops failing valid sub-compositions in two ways that sent authors in circles:

  • A scene without its own size. A sub-composition whose root has no data-width/data-height got root_missing_dimensions (an error), and the error stopped check before its browser audits. A mounted sub-composition without a size fills its host at 100% (compositionLoader sizes the flattened root to 100% when it declares none), and the core skill tells sub-composition roots to use 100%. The rule now applies to the entry composition only, as its own fix hint already says. Sub-compositions are found the same way root_dimensions_mismatch already exempts them (isSubComposition).

  • A scene whose <html> carries its variables. With <html data-composition-variables=…> around a <template>, the linter took <html> itself as the composition root and reported root_missing_composition_id and root_missing_dimensions against it. A <html> or <head> with no composition markers is now skipped when looking for the root, the same exemption a marker-less leading <svg> already gets, so the template's real root is found.

  • ../assets/… in a scene. The bundled guides write sub-composition asset paths relative to the scene file (../assets/logo.png from compositions/scene.html), and invalid_parent_traversal_in_asset_path rejected every ../ path as an error. The bundler, the producer and the preview runtime all resolve such a path from the scene's own folder (rewriteSubCompPaths), and project lint's missing-asset check already does too. Studio preview on the current release loads ../assets/a.png from a scene (checked: the image is rewritten to assets/a.png and decodes, no failed request). The rule now flags only a ../ path that still leaves the project once resolved from the file's folder, e.g. ../../x.png from compositions/scene.html, or any ../ from the root index.html.

  • ..\assets\x.png in a scene. The bundler and Studio preview rewrite a scene's ../ paths when inlining it into the root document, but only recognised / as a separator. A path written with backslashes was left as written, so preview resolved it against its own URL and 404'd, while render clamped it and loaded the file. rewriteAssetPath now reads a written \ as / before decoding, so both load it. As in a browser, an encoded %5C stays part of the file name and a path starting with \ stays absolute.

How

  • HyperframeLinterOptions gains compSrcPath, the file's project-relative path, the same name project lint already uses for it. lintProject passes it for the entry and every compositions/ file; Studio's lint route passes the file it lints.
  • The traversal rule walks each ../ path's segments from that folder and flags it only if it climbs above the project root. It is a plain segment walk, not node:path, because the rule also ships in @hyperframes/lint/browser. Without compSrcPath it treats the file as the project root, which keeps the old answer for callers that lint a single loose file.
  • A linked stylesheet's url()s resolve from the stylesheet's own folder, as the browser and the bundler resolve them. lintProject now hands each linked stylesheet's project-relative path to the rule. Before, url('../fonts/B.woff2') in styles/main.css linked from index.html was a false error, and a real escape in a stylesheet linked from a nested scene could pass.
  • Every relative path is walked, not only ones that start with ../. Backslashes, %2f and %2e are normalised and entities decoded first (render decodes %2f into a separator), and the query and hash are dropped, so %2e%2e/x.png, ..\x.png, assets/../../x.png and &#46;&#46;/x.png all count as climbing out. URLs with a scheme, absolute paths and fragments are skipped.
  • The file's folder depth comes from a ..-aware walk; an absolute or unnormalised path (sub/../index.html) counts as the project root, as before this change.
  • The message now says what happens: render and preview clamp such a path to the project root or resolve it outside it, so it loads a different file than written, or none.
  • The producer's lint passes its entry file as compSrcPath, so a nested entry gets the same answer there as in hyperframes lint.
  • browser.test.ts now walks the browser entry's imports, including @hyperframes/parsers subpaths, and fails on any Node built-in. The platform: "browser" build cannot catch those, because the parsers package stays external.
  • findRootTag shares one hasCompositionMarker check for <body>, <svg> and now <html>/<head>.

Tests

  • rules/core.test.ts: a template sub-composition without a size reports no root_missing_dimensions; <html> with variables around a <template> finds the real root.
  • rules/composition.test.ts: the traversal tests now state the contract: ../ from the root file errors; ../assets/ from compositions/scene.html passes and ../../assets/ errors; from compositions/frames/scene.html, ../../assets/ passes and ../../../ errors. A path that dips out midway (../assets/../../x.png) and a ./-prefixed file path also error.
  • browser.test.ts: the browser entry reaches no Node built-in, including through dynamic import() and bare names like path. Re-adding a node:path helper to the rule fails it.
  • rules/composition.test.ts also covers climbs written encoded, backslashed, mid-path or entity-encoded (from the root file and from a scene), absolute and ..-containing file paths (a leading .. never counts below the root), an encoded %2f climb, skipped URLs, a query or hash that would climb if read as a path, and a file linted without compSrcPath (treated as the project root).
  • studio-server routes/lint.test.ts: the lint route passes the file it lints as compSrcPath.
  • project.test.ts: a stylesheet linked from index.html with url('../fonts/…') passes; a stylesheet linked from a scene with url('../../outside.png') is reported.
  • project.test.ts: through lintProject, a scene's ../assets/logo.png passes while ../../outside.png is reported, and a scene without a size does not get root_missing_dimensions.
  • Lint's six touched test files together, three runs in a row: 471 passed each time. tsc --noEmit clean for lint, cli, studio-server and producer; comment ratchet clean.
  • Mutations: dropping the sub-composition exemption fails one core test and one project test; keeping <html> as a root candidate fails the <html>-variables test; ignoring the file's folder fails two traversal tests and one project test; resolving stylesheets from the HTML file's folder, skipping the %2e/backslash normalisation, reading the query as path, dropping the normalisation, the entity decoding, the absolute-path rule, the ..-aware depth, or the skip for URLs each fail at least one test; dropping compSrcPath from project lint fails the project test.
  • parsers rewriteSubCompPaths.test.ts: ..\assets\x.png from compositions/scene.html rewrites to assets/x.png (fails without the change); a%5Cb.png keeps its encoded backslash and \x.png is left alone (each fails if the swap moves after decoding or the leading-slash skip goes). 15 passed, three runs; core inlineSubCompositions and htmlBundler tests 202 passed.

Before

hyperframes lint on 0.8.143, three small projects (a scene without a size, a scene with variables on <html> around its <template>, a scene using ../assets/a.png):

compositions/s2.html  error root_missing_dimensions
compositions/s6.html  error root_missing_composition_id
compositions/s6.html  error root_missing_dimensions
compositions/s5.html  error invalid_parent_traversal_in_asset_path

After

The same projects on this branch: none of these findings. A path that really leaves the project (../../outside.png from compositions/) is still an error.

Not changed here

Studio's lint route treats every HTML file outside the project graph (such as scenes/intro.html) as a sub-composition, so such a file no longer gets root_missing_dimensions there. That labelling predates this change and its test pins it, so it stays.

Two older copies of the sub-composition path rewrite still read only /: the runtime loader's fallback for scenes loaded at runtime (core/src/runtime/compositionLoader.ts) and Studio's copy-to-clipboard (studio/src/utils/authoredSource.ts). Neither is the path preview or render takes for bundled scenes; they are left for a follow-up.

@miguel-heygen
miguel-heygen force-pushed the fix/lint-subcomposition-roots branch from 7fcca79 to 69303c1 Compare October 9, 2026 08:50
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 2059 (base branch 2059), smooth 1593 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 (0)

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 9, 2026 18:28

@somanshreddy somanshreddy 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.

Review at ea2a8b40: the code is clean, no findings that block. I'm leaving a comment rather than approving only because a required check is red at this head: Tests on windows-latest, shard engine-cli. It looks unrelated:

  • The failure is packageHls.test.ts:420 ("writes a master and no audio playlist when there is no audio"), a real-ffmpeg test that hit the 5 s timeout. Its afterEach then failed with ENOTEMPTY on the temp dir.
  • This PR touches no file under packages/engine.

A rerun of that job should clear it. I'll approve on a re-ping once it's green.

Checked:

  • Size rule: root_missing_dimensions now applies only when !options.isSubComposition, the same flag root_dimensions_mismatch already uses. lintProject passes compSrcPath, the entry as rootFile and each scene's own path; the Studio route and the producer pass the file they lint.
  • Root lookup: findRootTag now skips an <html>/<head> with no composition markers, using the shared hasCompositionMarker. An <html> that carries the markers is still eligible as the root.
  • Traversal rule: it's now a segment walk from the file's folder depth, after normalising \ and %2e, decoding entities, and dropping ?/#. Scheme, absolute and # values are skipped. A linked stylesheet's url()s resolve from the stylesheet's own rootRelativePath.
  • Backslash paths: rewriteAssetPath reads \ as / before decoding and leaves a path starting with / or \ unchanged. Resolution still goes through resolve("/", join(compDir, …)), so a ..\..\..\x path is clamped to the project root like the / form. It can't reach outside the project.

Tests:

  • lint: 902 passed. 3 slideshow.test.ts failures (malformed hotspot) also fail on main on this box, so they aren't from this PR.
  • parsers rewriteSubCompPaths: 15/15. studio-server routes/lint: 3/3.
  • Mutants: 5 of 6 caught:
    • sub-composition exemption dropped;
    • file depth ignored;
    • stylesheet depth taken from the HTML file;
    • <html>/<head> skip dropped;
    • backslash swap dropped in rewriteAssetPath.

Low:

  1. ..-clamp in folderDepth untested. Removing the Math.max(0, …) passes every test. It only matters for an unnormalised compSrcPath such as ../x/scene.html, which no caller passes today.
  2. Encoded %2f isn't treated as a climb (from reading the code; not run). The rule normalises %2e but not %2f, while rewriteAssetPath decodes every well-formed escape (decodeWellFormedEscapes). So ..%2f..%2fx.png from a scene isn't flagged, yet render decodes it to ../../x.png and clamps it to x.png, while the browser preview requests a literal file name. That's niche, but it's the "loads a different file than written" case the new message describes. Normalising %2f like %2e would close it.

— Somu

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks. Both lows are in at aef230d7:

  1. folderDepth's .. clamp is pinned: ../x/scene.html linting ../y.png passes, and dropping the Math.max fails that test.
  2. %2f is now read as a separator alongside \ before the walk, so ..%2f..%2fx.png from a scene is reported (fails without the change).

The red Windows engine-cli shard is the real-ffmpeg packageHls block running under vitest's 5 s default; #5368 gives that block the 30 s budget the package's other real-ffmpeg tests use. CI is running on the new head here.

@somanshreddy somanshreddy 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 aef230d7. My comment at ea2a8b40 (5473957847) found nothing blocking, and the only commit since then takes both of my lows:

  • %2f: the traversal rule now reads an encoded %2f as a separator (.replace(/\\|%2f/gi, "/")), matching how render decodes it. ..%2f..%2fx.png from a scene is now flagged.
  • folderDepth clamp: now pinned by a test (../x/scene.html with ../y.png passes).

composition.test.ts passes 201/201. Reverting either change fails its new test.

Merges cleanly with main. Tests on windows-latest is green at this head; the packageHls timeout I flagged didn't recur, and #5368 fixes it separately. Every other required check is green; Render on windows-latest was still running when I approved.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approve at aef230d7. This is in addition to Somu's approval. Both of Somu's lows are fixed, each new test fails without its fix, and lint now agrees with render on every traversal shape I tried.

Somu's lows

  • folderDepth clamp (composition.ts:140): the test at composition.test.ts:1411 fails if Math.max(0, …) is removed.
  • %2f as a separator (composition.ts:671): the test at :1405 fails if %2f is dropped from the regex.
    • There's no false positive. Turning %2f into / only adds segments, so assets/a%2fb.png from a scene still passes, while ..%2F..%2Fx.png is flagged.
    • It matches render. From compositions/, rewriteAssetPath turns ..%2f..%2fx.png into x.png clamped to the project root, so it loads a different file than the one written.

Traversal: lint vs rewriteAssetPath

  • They agree on mixed %2E%2e and .%2e/, on %252e (decoded once, so a literal name), on ....//, fullwidth dots and %5c (not climbs), and on //host, file: and data: (skipped).
  • Quoted or space-padded url(), an inline style with &quot;, and <link href> are all flagged.

Entry exemption (core.ts:506): the entry can't pick up the sub-composition exemption. Only lintProject's compositions/ loop and Studio's lint route pass isSubComposition. A nested entryFile gets no exemption, and its depth comes from its own folder. Studio's route takes compSrcPath from its own folder walk, not from the request, and only counts depth with it.

Nits

  • The producer's new compSrcPath (producer/src/services/hyperframeLint.ts:157-160) has no test. No producer test calls runHyperframeLint, so deleting the line leaves everything green.
  • Gaps that predate this PR, where lint says OK but the path escapes; render's clamp is the real guard in each case:
    • srcset and poster (neither lint nor render handles them, parsers/src/assetPaths.ts:20)
    • CSS hex escapes such as url(\2e\2e/\2e\2e/x.png), which htmlBundler.ts:152 decodes for linked stylesheets
    • a tab or newline inside the URL, which the browser strips
  • Studio still labels any uncovered .html (for example an alternate root main.html) as a sub-composition. That's older than this PR, and the body says so.

Tests:

  • lint 907/907, parsers rewriteSubCompPaths 15/15, studio-server routes/lint 3/3.
  • 9 of 10 mutants are caught: the %2f normalisation, the clamp, checking the climb only at the end, stylesheet depth taken from the HTML file, the raw vs entity-decoded attribute, the exemption, the <html>/<head> skip, the backslash order, and Studio's compSrcPath. The survivor is the producer line above.

— Rames

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit 5ec2dd2 Oct 9, 2026
173 of 174 checks passed
@miguel-heygen
miguel-heygen deleted the fix/lint-subcomposition-roots branch October 9, 2026 20:13
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.

3 participants