feat(cli): report which catalog items a render actually used

[miguel-heygen]

What

render_complete now reports which catalog (registry) items a project installed, and which blocks the rendered composition actually reaches.

Properties: registry_items, registry_item_count, registry_blocks_used, registry_blocks_used_count, registry_items_truncated, and registry_manifest_unreadable.

To make that possible, hyperframes add records each installed item in hyperframes.json under registryItems, and the sub-composition scan that answers "which files does this composition mount" moves to a single shared owner in @hyperframes/parsers.

Why

registry_item_added fires when a block is installed. render_complete fires when a video is produced. Nothing joins them: no render property names a catalog item, so "did this video use the catalog?" has no answer today, and "was the video any better for it?" cannot even be posed.

The interesting half is the delta. An item that was installed and then never mounted was tried and thrown away. That is a rejection signal, and no add-time event can express it.

How

• Manifest. Installed files are plain composition HTML with no provenance marker, so nothing downstream can tell a catalog block from one the user wrote. add writes { name, type, target } per item into hyperframes.json. Append-only, deduped by name, declared in docs/schema/hyperframes.json.
• Reachability. collectSubCompositionSrcs (in @hyperframes/parsers/asset-resolution, next to maskNonScannableRanges) is now the single owner of "which sub-compositions does this file mount". Both catalogUsage and lint's lintMissingOrEmptySubComposition route through it; lint's inline regex is gone. Two invariants live at that one function instead of being restated per call site:
  • It is a text scan, not a DOM query. Every sub-composition except the render entry is <template>-wrapped, and template content is inert, so document.querySelectorAll on a raw sub-composition file finds nothing. The renderer gets away with a DOM scan only because it recurses on compiled html.
  • References resolve root-relative at every nesting level, never against the referencing file's directory, matching parseSubCompositions in htmlCompiler.ts.
• Reporting. render_complete carries both lists. Counts come from the unsliced set; the 40-name cap applies only to the strings, at the boundary that builds them. Used names are narrowed to the reported installed names so registry_blocks_used stays a subset of registry_items, and registry_items_truncated says when the names are a window, so a query joining on names cannot read the cap as abandonment. Remote and inline mounts are dropped by the shared collector: they name no file on disk, so resolving one against the project root yields a path that reads as a missing local file.

Design decisions worth flagging:

• Components are counted as installed but never as used. They are pasted inline into the user's markup, so there is no src to match. Claiming one was used would be a guess.
• An unreadable manifest says so and omits the counts rather than sending zeros. The no-catalog cohort is the control this feature is measured against; quietly enrolling failed reads into it biases the comparison toward "the catalog makes no difference".
• Resolved once per plan, not per render. Batch rows vary only their variables, so every row shares the entry's composition tree.
• Slug-gated before reaching telemetry, matching the guard already applied to authoring-skill slugs, so a custom or hand-edited registry cannot push paths or unbounded cardinality into the event stream.
• In-place config patch, following seedProjectAuthoringSkill: hyperframes.json is normally committed, so an install must not reflow it or drop keys it does not own. Best effort throughout; a read-only or corrupt config never fails the add or render it rode in on.

primaryInstalledTarget is the single owner of "which file does this item mount", shared by the paste snippet and the manifest target, so the two can never disagree.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

Unit: 34 added it( blocks across catalogUsage, projectConfig, projectConfigSchema, plan, and events. Fixtures are <template>-wrapped with root-relative nesting, matching how sub-compositions are actually authored. Coverage includes the collector itself (template content, masked ranges, placeholders, unterminated tags, remote and inline mounts, and a megabyte of < as a linear-time guard), used-vs-dropped, nested mounts through a template, mount cycles, commented-out mounts, components, non-slug names, targets escaping the project, a missing entry file, remote mounts, manifest dedupe, no-rewrite on a redundant re-add, unknown-key preservation, corrupt-vs-empty manifest, zero-count emission, count saturation past the cap, and the used ⊆ installed relationship.

Manual:

Real init + add data-chart, resolving usage before and after pasting the block:

BEFORE paste: {"installed":["data-chart"],"usedBlocks":[],"manifestUnreadable":false}
AFTER  paste: {"installed":["data-chart"],"usedBlocks":["data-chart"],"manifestUnreadable":false}

That same generated config, validated with ajv against the schema it declares:

pre-PR schema   : INVALID -> Additional properties are not allowed ('registryItems' was unexpected)
this PR's schema: VALID

Against packages/producer/tests/nested-subcomp-depth-3, a fixture that renders end to end with a LEVEL3-PROOF-MARKER in its committed output:

installed : ["level-2","level-3"]
usedBlocks: ["level-2","level-3"]

Old algorithm versus new over the same realistic project, to confirm the regression fixture bites:

OLD (DOM scan + dirname-relative) reaches inner.html: false
NEW (text scan + root-relative)   reaches inner.html: true

Suites: green per package. cli 2846, core 2532, parsers 928, lint 531, producer 612, plus engine 1638, studio 4475, studio-server 456, sdk 549, player 338 at an earlier head. Four tests timed out on a whole-monorepo run, in four packages this branch does not touch; all four pass when their package runs alone, so they are parallel-load timeouts on the dev machine.

Performance: the shared scan is linear. A 20MB file of < characters, the worst case the caller's size cap permits, scans in 27ms; a 20MB file of real mount tags in 91ms.

Not covered: the three plain field assignments from render plan to render options to the event call are typechecked but never watched through a real render, because the telemetry transport hardcodes the PostHog host with no override to capture a payload locally. Also not closed: a manifest whose every name fails the slug gate still reads as an empty manifest. That needs a custom registry publishing non-slug names (all 154 built-in blocks are slugs), so it is left rather than given a third state.

[vanceingalls]

Verdict: Request changes

Additive, non-blocking feature (confirmed — see "blast radius" note below), but the core mechanism, the composition-src reachability walk in catalogUsage.ts, does not correctly determine reachability for the exact scenario ("a block reached through another block") the PR's own tests and description claim it handles. I verified this against the pinned linkedom version and a real fixture already in this repo, not just by reading the code.

Blocker: the reachability walk cannot see into <template>-wrapped sub-compositions, which is how every real sub-composition file in this codebase is authored

reachableCompositions (packages/cli/src/utils/catalogUsage.ts:566-589) does:

const { document } = parseHTML(html);
for (const el of document.querySelectorAll("[data-composition-src]")) { ... }

on the raw file content of every visited file. Per the HTML5 template-inertness spec (which linkedom correctly implements), content inside a <template> element is not part of the live document tree and is invisible to document.querySelectorAll on the containing document — it only exists under template.content. I confirmed this directly against the exact linkedom version this repo uses, run against a real fixture already in this repo (packages/producer/tests/nested-subcomp-depth-3/src/level-2.html):

top-level document matches: 0
template found: true
content matches: 1

That's not a hypothetical edge case — every sub-composition file except the render entry is authored this way in this codebase: every fixture under packages/producer/tests/*/src/compositions/*.html starts with <template id="...-template">, and packages/lint/src/rules/composition.ts's standalone_composition_wrapped_in_template rule forbids the root index.html from using this wrapper while checkSubCompositionUsability (packages/parsers/src/subCompositionValidity.ts) explicitly prefers <template> content for everything else, falling back to <body>.

Net effect: summarizeCatalogUsage can only ever mark a block "used" when it's mounted directly from the render entry file (which is not template-wrapped). Any catalog block mounted from inside another (template-wrapped) composition — e.g. a composite/dashboard block that itself mounts sub-blocks, or any scene wrapper that isn't the literal entry — will always resolve as unreachable and get reported as installed-but-abandoned, even when it renders into every video. That's a false positive on exactly the signal this PR exists to produce.

The PR's own "follows nested mounts" test (catalogUsage.test.ts) and the "used through a nested block" doc comment don't catch this because the test fixtures use bare, unwrapped HTML (the mount() helper produces <html><body><div data-composition-src=...>, no <template>) — a shape that doesn't match how sub-compositions are actually authored anywhere else in this codebase. The tests pass; they just don't exercise real input.

The codebase already has the correct pattern for this exact problem, right next to it: packages/lint/src/project.ts's lintMissingOrEmptySubComposition walks the same tree via a regex over raw text (maskNonScannableRanges + a data-composition-src pattern) specifically so it sees references inside <template> bodies, with an explicit comment that this must mirror assertSubCompositionsUsable/parseSubCompositions in htmlCompiler.ts. This PR reimplemented reachability from scratch with a DOM-based approach instead of reusing/mirroring that established, tested pattern, and the reimplementation diverges in exactly the way that matters.

Blocker (related): path resolution uses the wrong base for nested references

reachableCompositions resolves each nested data-composition-src relative to the referencing file's directory:

queue.push(resolve(dirname(current), src));

but the actual renderer always resolves data-composition-src relative to the fixed projectDir, threaded unchanged through every recursion level. htmlCompiler.ts says this outright:

Pass projectDir unchanged (not dirname(filePath)) — data-composition-src is always resolved root-relative, even from within a nested sub-composition. This must match parseSubCompositions' own recursive call below exactly ... or this pre-flight check resolves nested references to the wrong path.

packages/lint/src/project.ts independently states and implements the same invariant. catalogUsage.ts is the only place in the codebase that resolves nested composition-src relative to the parent file instead of the project root. For any real project where a nested composition (living in a subdirectory such as compositions/) mounts another root-relative reference — the codebase's actual convention — this computes the wrong absolute path, the read fails, and the target is silently dropped from the reachable set (caught by the try/catch, so it fails quiet rather than loud, compounding the false negative from the first bug rather than surfacing it).

In combination, these two bugs mean the walk's "nested reachability" behavior only coincidentally matches reality for flat, single-level projects, and is wrong for anything with real composition nesting. Given nesting is a documented, tested, supported pattern for this render pipeline (see nested-subcomp-depth-3), this isn't a hypothetical edge case.

What is solid

• Not on the render critical path. summarizeCatalogUsage / reachableCompositions fully fence file reads and parseHTML calls in try/catch, and loadProjectConfig already fails open to DEFAULT_PROJECT_CONFIG on corrupt/missing hyperframes.json (pre-existing behavior). A bug here produces wrong telemetry, not a broken or crashed render — verified by reading the call site in plan.ts (nothing there throws uncaught) and the existing readProjectConfig catch-all.
• Bounded and cycle-safe as a mechanism. The seen-set + MAX_VISITED_FILES cap genuinely prevents infinite loops/unbounded crawling on a cyclic graph — that specific claim holds, independent of the reachability-correctness bugs above.
• Manifest write path is careful. recordProjectRegistryItems patches JSON in place (preserves unknown keys/indentation, doesn't round-trip through normalizeConfig), no-ops on a missing or corrupt config, and normalizeRegistryItems degrades a hand-edited/partial manifest to whatever still parses. Back-compat for pre-existing hyperframes.json files without registryItems is clean (undefined → treated as zero items, not a crash).
• Telemetry hygiene is good. Names are slug-gated (REGISTRY_ITEM_NAME) before reaching the anonymous event stream, so a custom/hand-edited registry can't leak paths or unbounded cardinality into analytics. Counts are correctly emitted at zero rather than omitted, and genuinely omitted (not zeroed) when usage was never resolved — matches the stated intent of distinguishing "no catalog" from "an older CLI."

Nit

recordProjectRegistryItems's no-op fast path (packages/cli/src/utils/projectConfig.ts, the byName.get(item.name) === item check) compares object identity, not value equality. Since add.ts always constructs fresh record objects, re-adding an already-installed item with unchanged name/type/target will never hit the identity match and will rewrite the file every time (harmless — same values serialize to the same bytes — but the "skip the write when nothing changed" comment overstates what the code does).

Test coverage

The interesting edge (installed-then-abandoned, i.e. the actual rejection signal) is covered at the unit level for summarizeCatalogUsage and exercised through plan.ts and events.ts. The gap is fixture realism, not missing scenarios: none of the multi-level tests use a <template>-wrapped fixture, which is why the bugs above shipped with green tests.

Merge state

mergeStateStatus is BLOCKED with zero reviews and every required/relevant check (CI, CodeQL, format, perf/regression "Detect changes" gates) green or appropriately skipped, nothing pending — this reads as the required-reviewer gate, not a CI failure. Separate from the code verdict above.

— Vai

[jrusso1020]

Additive review — @vanceingalls (Vai) already filed the two reachability blockers in catalogUsage.ts (review 5012956622). I reached both independently before reading that review and I'm not going to restate them; below is the new evidence I have on those two, plus one blocker and two correctness gaps Vai's review doesn't cover.

What's genuinely strong

• primaryInstalledTarget (add.ts:91) does what the description claims. The snippet's data-composition-src and the manifest target now come from one function at add.ts:346 and add.ts:357, so they cannot disagree — I checked both call sites, and this is the kind of invariant that's normally asserted in a comment and violated two releases later.
• The manifest write is genuinely careful about a committed file: recordProjectRegistryItems (projectConfig.ts:274) patches parsed JSON in place and reuses the file's own indentation rather than round-tripping through normalizeConfig. Adding registryItems to the normalizeConfig whitelist (projectConfig.ts:134) is the non-obvious half — omitting it would have silently dropped the manifest on every read/write cycle, and the comment says so.
• I specifically checked for a manifest-wipe path and did not find one: both writeProjectConfig(projectDir, DEFAULT_PROJECT_CONFIG) calls in add.ts (:282, :502) are existsSync-guarded to fire only when no config exists, and seedProjectAuthoringSkill's default-write is on the ENOENT branch only. The two in-place writers compose correctly.

Blocker — registryItems is rejected by the schema the config itself declares

docs/schema/hyperframes.json sets "additionalProperties": false and its properties map has no registryItems. Every config init writes carries "$schema": "https://hyperframes.heygen.com/schema/hyperframes.json" (projectConfig.ts:16, :79), and that schema is served from docs (#304/#305).

So the first hyperframes add in a project turns a valid hyperframes.json into one that fails validation against the schema it points at. In any editor with JSON-schema support that's an immediate squiggle on a committed file — Property registryItems is not allowed.

The in-repo precedent settles the expectation: authoringSkill is the same shape of field (persisted into hyperframes.json, telemetry provenance, slug-gated), and commit d287e5244 (#2762) added it to projectConfig.ts and to docs/schema/hyperframes.json in the same commit — including a pattern mirroring its slug gate.

Nothing in CI would have caught the omission: scripts/sync-schemas.ts mirrors only registry.json and registry-item.json, and its header says docs/schema/hyperframes.json "is authored directly in docs (no source in core) so it's skipped by this script."

The PR body reads this as "the config schema page may want a follow-up." With additionalProperties: false it isn't a docs nicety — it's a validation break for every user who runs add. Adding the property to the schema is a few lines and belongs in this PR.

On Vai's two blockers — confirmed, with the direction measured both ways

I ran the real summarizeCatalogUsage (not a transliteration) against this repo's own packages/producer/tests/nested-subcomp-depth-3 fixture, which renders end-to-end and carries a LEVEL3-PROOF-MARKER in its committed output.mp4:

installed : ["level-2","level-3"]
usedBlocks: ["level-2"]          <- level-3 IS in the rendered video

On the resolution base, a two-sided control is worth adding, because the bug is not just a miss in one direction — it is inverted in both. Same fixture shape, no <template> anywhere, so the only variable is the base:

[A] nested src authored "compositions/inner.html"  (root-relative — what the renderer requires)
    usedBlocks: ["outer"]              <- renderer mounts inner; we report it dropped
[B] nested src authored "inner.html"   (relative to the referencing file)
    usedBlocks: ["inner","outer"]      <- renderer would NOT mount inner; we report it used

[B] is the shape catalogUsage.test.ts:69-80 and :83-94 use, which is why the suite is green. So the tests don't merely fail to exercise real input — they pin the inverse of the documented contract (htmlCompiler.ts:219: "Pass projectDir unchanged (not dirname(filePath))"). Worth fixing the fixtures in the same change, or the next edit re-derives the same bug from a passing test.

One severity refinement, since it changes who this actually bites: catalog blocks themselves are mostly not template-wrapped — 13 of 161 files under registry/blocks/. The wrapping is on the host side: 80 of 82 sub-composition fixtures under packages/producer/tests/*/src/compositions/ are template-wrapped. So a block pasted straight into index.html resolves fine, and the miss lands on a block pasted into a scene file — which is the normal multi-scene project layout, so the practical exposure is still high.

Important — the 40-item cap is applied to the counts, not just the names

MAX_REPORTED_ITEMS exists to keep a long names string out of the event (catalogUsage.ts:50), but reportable() slices before the counts are taken, and events.ts reports usage.installed.length / usage.usedBlocks.length (events.ts:192-193). So the counts saturate silently. Measured, 45 installed items:

items in manifest   : 45
registry_item_count : 40

A count is one integer with no cardinality risk — capping it buys nothing and costs the real number, with no way downstream to tell 40 from 400.

The same slice also breaks the subset relationship the properties are meant to have. installed is sliced to the alphabetically-first 40, but usedBlocks is computed from the full manifest and sliced independently, so the two sets can be disjoint. Measured, 45 items where only the last 5 are mounted:

registry_items      : b01..b40
registry_blocks_used: ["b41","b42","b43","b44","b45"]
used-but-absent-from-installed: ["b41","b42","b43","b44","b45"]

Any downstream query that assumes registry_blocks_used ⊆ registry_items — the natural way to compute the drop-off this PR exists to measure — gets rows that violate it. Cheapest fix: take the counts from the deduped-but-unsliced set, and derive usedBlocks from the same slice installed was taken from.

Important — an unreadable manifest is byte-identical to "never used the catalog"

The description is explicit that zero-counts are emitted so "the no-catalog cohort exists to compare against." But every degraded read collapses into that same cohort. loadProjectConfig fails open to DEFAULT_PROJECT_CONFIG, so corrupt JSON yields registryItems: undefined → EMPTY. Measured:

corrupt manifest    -> registry_item_count 0, registry_items []
never-used-catalog  -> registry_item_count 0, registry_items []

Identical events. The same collapse happens when every manifest name fails the slug gate (catalogUsage.ts:130 returns EMPTY).

Vai reads the fail-open behavior as a strength, and for render safety that's right — nothing here should ever break a render, and it doesn't. The gap is narrower: the control group is the one cohort that must not silently absorb failures, because a read failure enrolls a catalog user into "no catalog" and biases the comparison toward "the catalog makes no difference." A single registry_manifest_unreadable: true property, or omitting the counts (rather than zeroing them) when the config existed but could not be parsed, separates the two at no cost.

Nits

• The seedProjectAuthoringSkill doc comment (projectConfig.ts:211) still says it is "the only writer that touches an ALREADY EXISTING hyperframes.json". This PR adds a second one. Worth updating in place — that sentence is exactly what the next person auditing write paths will rely on.
• Test-plan count: I count 19 added it( blocks across the four test files (9 + 6 + 1 + 3), not the 21 the description states. Doesn't change coverage, just the number.

Audited / Trusting

Audited (read end-to-end): catalogUsage.ts, projectConfig.ts (incl. the pre-existing seedProjectAuthoringSkill / normalizeConfig it builds on), catalogUsage.test.ts, the add.ts / plan.ts / execute.ts / render.ts / events.ts diffs, docs/schema/hyperframes.json, scripts/sync-schemas.ts, and the resolution sites in htmlCompiler.ts and packages/lint/src/project.ts.
Executed: summarizeCatalogUsage against the depth-3 fixture, the two-sided resolution control, the 45-item cap case, and the corrupt-vs-empty pair — all against the module itself at 3a79953.
Trusting (not verified): the "2836 passed, 1 failed" full-suite figure and the claim that studioServer.test.ts loadRuntimeSource fails identically on main — I ran only the targeted probes above, not the full CLI suite. Also not verified: real-render behavior of the three plan→options→event field assignments, which the description already flags as typechecked-but-unexercised.

CI is genuinely green at 3a79953 — every required check passes, the rest are skips, so BLOCKED is the reviewer gate and not a CI problem (agreeing with Vai).

Verdict: REQUEST CHANGES
Reasoning: The schema omission is a user-visible break independent of the telemetry bugs — it would survive a fix to Vai's two blockers and ship a config that fails its own declared schema. The counts and the corrupt-manifest collapse each bias the specific comparison this feature exists to enable, and all four are cheap to fix while the change is open.

— Rames Jusso

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
hyperframes	🟢 Ready	View Preview	Aug 24, 2026, 10:22 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[miguel-heygen]

Both reviews were right on every finding. All six are fixed in f77813e07. I reproduced each one before changing anything rather than taking the reports on trust; evidence below.

The two reachability blockers

Confirmed independently, both of them.

Template inertness, against this repo's own packages/producer/tests/nested-subcomp-depth-3/src/level-2.html on the pinned linkedom:

top-level querySelectorAll matches: 0
template found: true
template.content matches: 1

Resolution base: htmlCompiler.ts:219 and packages/lint/src/project.ts:600 both state and implement root-relative, and my walk was the only place in the repo using dirname(current).

The point about the fixtures pinning the inverse of the contract is the part that stung, and it is the reason I did not just patch the two lines. The scan now has one owner. collectSubCompositionSrcs lives in @hyperframes/parsers/asset-resolution next to maskNonScannableRanges, holds the text scan and the placeholder skip, and both catalogUsage and lintMissingOrEmptySubComposition route through it. Lint's inline regex is deleted. Resolution is root-relative in both, with the invariant written down at the shared function rather than restated at each call site.

I verified the new fixture actually bites, by running the old algorithm and the new one over the same realistic project rather than trusting that it would:

OLD walk (DOM scan + dirname-relative) reaches inner.html: false  (2 files)
NEW walk (text scan + root-relative)   reaches inner.html: true   (3 files)

And on your depth-3 fixture, the one whose committed output.mp4 carries LEVEL3-PROOF-MARKER:

installed : ["level-2","level-3"]
usedBlocks: ["level-2","level-3"]     <- was ["level-2"]

New fixtures are <template>-wrapped with root-relative nesting, in catalogUsage.test.ts and in the plan.test.ts case too. I deliberately did not add a test bound to the producer/tests fixture itself: the invariant is covered by the unit fixture, and a cli test reaching across into another package's render fixtures buys coupling I would rather not pay for. Say the word if you would rather have the integration test anyway.

Schema blocker

Reproduced against a config produced by a real hyperframes init + hyperframes add data-chart:

pre-PR schema  : INVALID -> Additional properties are not allowed ('registryItems' was unexpected)
this PR's schema: VALID

registryItems is now declared in docs/schema/hyperframes.json, following the authoringSkill/#2762 precedent. Since sync-schemas.ts skips this file by design, projectConfigSchema.test.ts now pins every key the CLI can write against the schema, typed off ProjectConfig so a new field fails to compile there before it can fail validation on someone's disk. "The docs page may want a follow-up" was the wrong read; you were right that it is a validation break.

The cap, and the subset relationship

Both real. The cap moved to catalogEventProperties, the boundary that actually builds the string, and applies to names only. Counts come from the unsliced set. Used names are narrowed to the reported installed names, so registry_blocks_used ⊆ registry_items holds even when the cap bites. Two tests pin it at the 45-item case you measured.

Corrupt manifest collapsing into the control cohort

This was the finding I would have been least likely to catch, and it goes at the heart of what the feature is for. readProjectConfigWithStatus now separates "no config" from "config I could not read", and an unreadable manifest emits registry_manifest_unreadable: true and omits the counts rather than sending zeros.

One case I did not close, so it is not a silent gap: if every manifest name fails the slug gate, the result is still indistinguishable from an empty manifest. That needs a custom registry publishing non-slug names (all 154 blocks in the built-in registry are slugs), so I left it rather than add a third state. Happy to flag it too if you disagree.

Nits

Both taken. The identity comparison in recordProjectRegistryItems is a value comparison now, with a test that re-adding an unchanged item does not rewrite. seedProjectAuthoringSkill's "only writer" sentence now names the second writer.

On the test count: you are right, and my number was wrong in a way worth naming. It is 25 added it( blocks now.

Verification

packages/cli + packages/lint + packages/parsers: 4295 passed, 1 failed. The failure is studioServer.test.ts > loadRuntimeSource, and on the untouched main checkout vitest sweeps every sibling worktree on this machine and the same assertion fails in all 86 copies, including worktrees that predate this branch. So it is environmental, not this change. You were right to mark that claim untrusted; that is the harder evidence.

Still unexercised, unchanged from the original description: the three plan → options → event field assignments are typechecked but never watched through a real render, because the transport hardcodes the PostHog host with no override to capture a payload locally.

[miguel-heygen]

Correction on one line of my reply: when I wrote it, the no-rewrite check did not exist yet. The suite only covered dedupe. It exists now in 00f95c11f, asserting the config file is byte-identical after a redundant re-add, so a regressed guard fails rather than passing quietly. 26 added it( blocks, not 25.

Branch is at 990 changed lines against the merge base, just under our 1k cap, so anything further from this round should go in a follow-up PR rather than growing this one.

[vanceingalls]

Verdict: Request changes

Re-reviewed at 00f95c11 (up from 3a79953). Good news first: all four blockers from the last round are genuinely fixed, and I verified each independently rather than trusting the diff or commit messages — by running the actual PR logic (copied verbatim from the head files, since a scratch clone build-artifact gap blocked vitest locally) against constructed fixtures, plus schema-validating against a real ajv instance. One new issue surfaced by the fix itself, confirmed by CodeQL and independently reproduced: a quadratic-time regex now sits on the render-blocking path.

1. Template-wrapped nested reachability — FIXED

collectSubCompositionSrcs (packages/parsers/src/assetResolution.ts:69–83) replaces the DOM querySelectorAll walk with a masked text scan — the same approach packages/lint/src/project.ts’s lintMissingOrEmptySubComposition already used, now shared instead of reimplemented. packages/lint/src/project.ts was refactored to call the same shared function, so lint and telemetry can no longer drift apart on the answer.

Verified empirically: constructed a project with index.html → compositions/outer.html (template-wrapped) → compositions/inner.html (template-wrapped), ran the real reachableCompositions/isReached logic against it — both outer and inner are now correctly reported in usedBlocks. Previously this would have reported 0 matches on the containing document, per last round’s finding.

2. Resolution base (project root vs. referencing file’s dirname) — FIXED, including the inverse

reachableCompositions now does queue.push(resolve(projectDir, src)) — root-relative at every nesting level, matching htmlCompiler.ts’s documented invariant and packages/lint/src/project.ts.

I ran the two-sided control Rames specified:

• Root-relative nested src (the correct, renderer-matching shape): now reported used. Previously reported dropped.
• Dirname-relative nested src (the shape the old tests pinned): now correctly reported NOT used, matching that the real renderer would never mount it.

The test suite was fixed alongside the code, not just the code: catalogUsage.test.ts’s fixtures now use <template>-wrapped sub-compositions with root-relative data-composition-src, with an explicit comment ("used through a nested block" test) that this mirrors real authoring. The tests no longer pin the inverse of the contract.

3. Schema omission (registryItems rejected by additionalProperties: false) — FIXED

docs/schema/hyperframes.json gained a registryItems array property (required name/type/target, additionalProperties: false on the item), in the same shape as the authoringSkill precedent (d287e5244). I validated with a real ajv 2020-12 instance against the actual schema file: a config with registryItems populated now validates; before this change it would fail with "registryItems not allowed." The PR also adds projectConfigSchema.test.ts, which pins every ProjectConfig key (including a compile-time Required<...> shape) against the schema’s declared properties — this closes the exact CI gap Rames flagged (sync-schemas.ts still correctly skips this hand-authored file; the new test is the missing guard, not a change to that skip-list assumption).

4. 40-item cap applied to counts, and installed/usedBlocks going disjoint — FIXED

catalogEventProperties in events.ts now takes registry_item_count/registry_blocks_used_count from the full, uncapped arrays, and separately narrows registry_blocks_used to the intersection with the capped registry_items names (usage.usedBlocks.filter(name => reportedNames.has(name))), so the reported subset relationship holds even when the cap bites.

Reran Rames’s 45-item measurement against the actual function: with b01..b45 installed and b41..b45 used, registry_item_count is 45 (not 40) and registry_blocks_used_count is 5 (true count); registry_blocks_used is correctly absent rather than emitting a name list disjoint from registry_items. Also confirmed registry_manifest_unreadable: true is now emitted (with counts omitted) instead of the zero-count shape a genuinely catalog-free project gets — the two are no longer byte-identical, so a corrupt manifest can’t silently join the control cohort.

New issue: ReDoS in the shared collectSubCompositionSrcs regex, now on the render-blocking path

CodeQL flags 1 high-severity alert on this PR, at packages/parsers/src/assetResolution.ts:73 — exactly the new compositionSrcRe in collectSubCompositionSrcs:

Polynomial regular expression used on uncontrolled data — may run slow on strings starting with ‘<’ and with many repetitions of ‘<’.

I reproduced this directly against the actual regex, outside any framework:

​​​
n=20000 chars of '<' -> 587ms
n=40000 chars of '<' -> 2378ms
n=80000 chars of '<' -> 9619ms
n=160000 chars of '<' -> 38239ms
​​​

That’s quadratic, and reachableCompositions allows files up to MAX_HTML_BYTES (20MB) each, across up to MAX_VISITED_FILES (250) files, with no per-scan timeout. Extrapolating the measured growth, a single ~20MB file that is mostly < characters would take this synchronous call somewhere in the range of days, not seconds.

This regex itself isn’t new — it’s the same pattern packages/lint/src/project.ts already ran inline — but this PR is what widens its blast radius in a way that matters:

• It’s now a shared, exported library function (@hyperframes/parsers/asset-resolution) rather than private to an explicit, opt-in lint command.
• It’s now wired synchronously into createRenderPlan (plan.ts: summarizeCatalogUsage(project.dir, renderTarget)), which runs on every hyperframes render, before the render itself starts.

That directly undercuts this PR’s own safety framing (repeated in both rounds) that a bug here “produces wrong telemetry, not a broken render.” A pathological or even accidentally malformed composition file (truncated download, corrupted registry item, an embedded blob with many stray <) can now hang the render plan step indefinitely, before any video is produced — that’s a render-blocking regression, not a telemetry one, and it’s the one thing this feature was designed never to risk.

The fix is cheap and doesn’t require abandoning the text-scan approach: anchor/possessive the outer scan (e.g. iterate line-by-line, or scan for the literal data-composition-src first and only regex a bounded window around each hit) instead of two unbounded [^>]* spans across the whole file. This needs to land before merge — CodeQL is currently red on this PR because of it.

Minor / FYI, not blocking

• CodeQL also surfaces 2 medium alerts ("File data in outbound network request") in packages/cli/src/registry/remote.ts:92,207. That file isn’t touched by this PR’s diff — these look like pre-existing code that a new taint path from this change’s manifest data flows through. Worth a separate look, but not this PR’s responsibility to fix, and I didn’t chase the data-flow path given time.
• CI is otherwise green: Test, CLI smoke (required), Smoke: global install, Typecheck, Lint, Producer suites, etc. all passed at 00f95c11 at time of review.

Summary

Strong turnaround on the substance — all four blockers are real fixes, not surface patches, and the test fixtures were corrected alongside the code rather than left pinning the old (wrong) contract. But the same PR that fixed the reachability walk introduced a new, CodeQL-confirmed, empirically-reproduced ReDoS on a path that now runs on every render. That needs to be closed before this merges.

— Vai

[jrusso1020]

Re-review at 00f95c11 (my previous CHANGES_REQUESTED was at 3a799532). All four of my blockers are fixed, and I confirmed each by running the module rather than by reading the diff.

@vanceingalls posted 5013260403 while I was verifying and reached the ReDoS independently, so I have trimmed what it already covers. Its severity argument is the part I would have gotten wrong: I had this as a should-fix rather than a gate. I checked the claim instead of inheriting it — plan.ts:212 calls summarizeCatalogUsage synchronously inside createRenderPlan, before anything renders, so a hang there stops the render from starting rather than degrading a telemetry property. That is the right read.

Below is what is additive: why the alert is newly introduced rather than relocated, the fix verified behavior-identical, and two things neither review has.

Strengths

• assetResolution.ts:67 — fixing the template blindness by extracting the lint scanner instead of patching a second copy is the right call. packages/lint/src/project.ts already had the correct text scan; the telemetry path had independently re-derived a DOM scan that got it wrong. One owner means they cannot drift again, and lintMissingOrEmptySubComposition losing 8 lines is the proof it was the same problem twice.
• projectConfig.ts:319-341 — the identity-vs-value comparison fix is a real bug I did not catch last round. byName.get(item.name) === item compared against the replacement object, so a redundant re-add rewrote the file every time. projectConfig.test.ts:402 pins it through the file's own tabs, which is the right assertion because a reflow is exactly what a rewrite would produce.

Verified fixed

Run against the real module, not read off the diff:

1. Schema break. docs/schema/hyperframes.json:59 declares registryItems, and the item shape (required: [name, type, target], additionalProperties: false) matches RegistryItemRecord at projectConfig.ts:44-51 field for field.
2. <template> blindness. Confirmed maskNonScannableRanges (assetResolution.ts:154-159) masks comments, <style> and <script> but not <template>, so template content stays scannable. Ran summarizeCatalogUsage against this repo's own packages/producer/tests/nested-subcomp-depth-3 fixture — the one whose level-3 marker covers half the frame and sits in the committed baseline: usedBlocks = ["level-2","level-3"]. It was ["level-2"] at the old head.
3. Resolution anchor. Two-sided control, correct in both directions: a root-relative nested mount reports ["inner","outer"], and a dirname-relative one (which the renderer would not mount) reports ["outer"]. The fixtures at catalogUsage.test.ts:99-118 now pin the anchor and the scanner in one test.
4. Cap on counts + disjoint sets. 45 installed, only b41..b45 reachable → registry_item_count: 45, registry_blocks_used_count: 5, 40 names, used ⊆ installed. At the old head this gave registry_items: b01..b40 against registry_blocks_used: [b41..b45] — fully disjoint.
5. Corrupt manifest. readProjectConfigWithStatus (projectConfig.ts:106) separates ENOENT from a parse failure, and events.ts:210 emits registry_manifest_unreadable: true with the counts omitted rather than zeroed.

Both nits from last round are addressed — the stale "only writer" comment at projectConfig.ts:236 now names both writers.

On the ReDoS — additive only

It is newly introduced, not relocated noise. Worth stating because that is the reading that gets a CodeQL alert waved through. The byte-identical regex sits at packages/lint/src/project.ts:591 on main and has no alert there — main carries 8 open js/polynomial-redos alerts and none of them is that file. It was a local const inside a non-exported closure. collectSubCompositionSrcs is an exported function of @hyperframes/parsers, so its html parameter is library input and the taint now reaches the regex. The bot's own wording ("depends on library input") is the tell.

The fix is one character, and I verified it rather than suggesting it:

const compositionSrcRe = /<[^<>]*\bdata-composition-src\s*=\s*["']([^"']+)["'][^<>]*>/gi;

Excluding < from both classes means a run of < cannot be consumed. Identical captures on real markup for both double- and single-quoted attributes, and flat where the current one is quadratic — measured end-to-end through the exported function on "<".repeat(n):

bytes	current	with [^<>]
20,000	274 ms	0.0 ms
40,000	1,086 ms	0.1 ms
80,000	4,352 ms	0.1 ms
160,000	17,401 ms	0.1 ms

Clean 4x per doubling. Masking does not help — a run of < is not a comment, <style> or <script>.

One qualifier for calibration: this needs a pathological input. Authored HTML puts a > within a few characters of every <, so [^>]* never scans far, and I could not construct a realistic composition that triggers it. The reason to fix it here anyway is the one @vanceingalls names — it is on the render-blocking path with no timeout — plus the export being reachable by anything downstream that parses HTML it did not author.

Important — not in either review yet

The new schema guard does not pin the registryItems item shape. projectConfigSchema.test.ts:35.

EVERY_WRITTEN_KEY lists registryItems: [], so nothing validates the objects inside it. The schema gives that item additionalProperties: false, so a fourth field on RegistryItemRecord recreates exactly the bug this test exists to prevent, one level down.

Confirmed by mutation: I added version: string to RegistryItemRecord (projectConfig.ts:50) and projectConfigSchema.test.ts stayed green — while hyperframes add would then be writing {name, type, target, version} into a config the schema rejects.

The guard is otherwise well built — the Required<Omit<ProjectConfig, "$schema">> typing makes a new interface field a compile error until it is listed, and asserting additionalProperties === false first stops the guard going inert. It just stops at the top level. ajv@8.20.0 is already in the tree, so compiling the schema once and validating a config that contains one representative item closes it.

Nits

• The remote-mount filter was dropped in the extraction, and its test was rewritten to stop pinning it. The old subCompositionSrcs skipped anything with a URL scheme ("Remote mounts have no local file to match an installed item against"); collectSubCompositionSrcs has no such filter, so https://example.test/a.html survives and catalogUsage.ts:107 resolves it to <projectDir>/https:/example.test/a.html. Harmless in effect — the read fails and the walk continues — but reachableCompositions calls seen.add(current) before the read (catalogUsage.ts:96), so each remote mount now consumes one of the 250 MAX_VISITED_FILES slots it used to be filtered out of. The covering test was renamed from "ignores a remote mount rather than resolving it as a local path" to "contributes nothing for a remote mount" (catalogUsage.test.ts:190) and now passes with or without the filter, because the fixture's BLOCK("kept") target does not exist either way. Worth restoring the scheme check inside the shared collector — lint wants it too.
• registry_blocks_used can be empty while registry_blocks_used_count is non-zero. In the 45-item run above the used blocks were b41..b45, all alphabetically past the 40-name cap, so registry_blocks_used came back [] against a count of 5. The count carries the truth and the subset invariant is the right call, but a drop-off query joining on names reads that project as 45 abandoned items. A registry_items_truncated boolean — or just noting the count > 40 tell in the doc comment at events.ts:193 — would save whoever writes that query.

Scope

Audited end-to-end: catalogUsage.ts, assetResolution.ts (new function + maskNonScannableRanges), the catalog half of events.ts, projectConfig.ts read/status/record paths, docs/schema/hyperframes.json, all five changed test files, the packages/lint/src/project.ts walk.
Executed locally: summarizeCatalogUsage against the depth-3 fixture and constructed two-sided controls; the 45-item cap case; the ReDoS timings for both regexes; a mutation of RegistryItemRecord; vitest on catalogUsage / events / projectConfig / projectConfigSchema — 48 passed.
Trusting: CI for the rest. All 8 required contexts are green at 00f95c11 (Test went green partway through this review). CodeQL is red and is not in the required set — so nothing mechanical gates on it.
Not verified: the two js/file-access-to-http alerts in registry/remote.ts — pre-existing on main, untouched here.

Verdict: COMMENT
Reasoning: Every blocker I raised last round is fixed at the root rather than patched, and I confirmed each by execution, including against the repo's own render fixture — so my 5013001338 no longer describes the code. I am not converting it to an approval while the ReDoS is open and a peer review is blocking on it. Fix the one-character regex and the item-shape guard and I will approve at that head.

— Rames Jusso

[miguel-heygen]

Fixed at d5a7ee5fb. The ReDoS is real, I reproduced it before changing anything, and it is closed. I also have a correction to make on evidence I gave in the last round.

The ReDoS

Reproduced against the shared function itself, not a transliteration:

n=10000 chars of '<' ->   41ms
n=20000 chars of '<' ->  165ms
n=40000 chars of '<' ->  660ms
n=80000 chars of '<' -> 2640ms

Four times the work for twice the input, matching your numbers. I also ran the 20MB case that MAX_HTML_BYTES actually permits: it did not finish in over ten minutes before I killed it, so "days, not seconds" is if anything conservative.

Your framing of why this is mine is the part I want to acknowledge, because it is the correct one and I would have been tempted to argue "pre-existing". The pattern was already in lint, but this branch is what turned it into an exported library function wired synchronously into createRenderPlan. It now runs before every render. A truncated download or a blob of stray < could hang the plan step before any video exists. That is precisely the render-blocking failure I claimed twice this feature could not cause, so the safety framing was wrong until this commit.

Fix: the scan walks tag by tag with indexOf and applies a bounded attribute regex to one already-delimited tag, so no quantifier ranges over the whole file. I took the "scan for the attribute in a bounded window" shape you suggested rather than abandoning the text scan.

80k chars of '<'    ->  0ms
20MB of '<'         -> 27ms
20MB of '<a'        -> 25ms
20MB of real mounts -> 91ms, srcs found correctly

Semantics are unchanged, and deliberately so: the old regex also treated > as a tag terminator ([^>]* cannot cross one) and also required a closing >, so an unterminated final tag was never a match before either. Fixing it at the shared owner fixes lint's path too, which is the upside of having consolidated it last round.

The regression test needs no timing assertion to bite: a megabyte of < ran for minutes under the old scan, so the case simply failed on the suite timeout. Five tests now cover the collector directly in assetResolution.test.ts (template content, masked ranges, placeholders and dedupe, unterminated tag and bare attribute, and the megabyte case).

Correction: the "pre-existing studioServer failure" I reported was my own bad invocation

In both previous rounds I reported studioServer.test.ts > loadRuntimeSource as a pre-existing environmental failure, and backed it with "the same assertion fails in 86 sibling checkouts". @jrusso1020 marked that claim untrusted. Right call, and my evidence was worthless: I was running vitest from the repo root with a path filter, which picks up the wrong config, and the "86 copies" was that same bad invocation fanning out across worktrees on this machine.

Run the way the repo actually runs it, the full monorepo suite is green:

@hyperframes/cli            2844 passed, 3 skipped   (studioServer.test.ts passes)
@hyperframes/core           2532 passed
@hyperframes/parsers         927 passed, 4 skipped, 3 todo
@hyperframes/lint            531 passed
@hyperframes/producer        612 passed
@hyperframes/engine         1638 passed, 3 skipped
@hyperframes/studio         4475 passed, 18 todo
@hyperframes/studio-server   456 passed
@hyperframes/sdk             549 passed
@hyperframes/player          338 passed

Four tests timed out on the first whole-monorepo run (streamingEncoder, proxyTranscoder, mutate.gsap, PropertyPanel), in four packages this branch does not touch. All four pass when their package runs on its own, so they are parallel-load timeouts on this machine, not defects. Saying so rather than quietly dropping them.

No code decision changed because of the bad claim, but I asserted it as evidence twice and it should not stand.

Also in this push

Rebased onto b2fc18b2d, since main had moved eleven commits.

On the two medium CodeQL alerts

Agreed they are out of scope, and I did not chase the data-flow path either. Recording what I can say: this branch's diff does not touch packages/cli/src/registry/remote.ts, and the manifest data it writes (name, type, target) comes from the resolved registry item, the same values registry_item_added already reported before this PR. If that is a new taint path it is one CodeQL is now tracing further, not new data reaching that sink. Worth its own issue rather than a guess from me.

Still not closed

Unchanged from last round, both stated rather than buried: the three plan → options → event field assignments are typechecked but never watched through a real render, because the transport hardcodes the PostHog host with no override to capture a payload locally. And a manifest whose every name fails the slug gate still reads as an empty manifest, which needs a custom registry publishing non-slug names.

[miguel-heygen]

@jrusso1020 you posted while I was replying to Vai and I answered only his review. My mistake, and your three items were the ones nobody else had. All fixed at 839ecc5e9.

The schema guard stopping at the top level

You were right, and the mutation is the reason I could not have argued with it. I re-ran it: adding version: string to RegistryItemRecord left the old test green while add would be writing a config the schema rejects.

The guard now compiles the schema with ajv and validates a config carrying a fully populated item, with the record typed Required<RegistryItemRecord> so a new field is a compile error before it is ever a validation failure. Re-running your mutation against the new guard:

packages/cli/src/utils/projectConfigSchema.test.ts(34,7): error TS2741:
  Property 'version' is missing in type '{ name: string; type: string; target: string; }'
  but required in type 'Required<RegistryItemRecord>'

There is a companion case asserting ajv rejects an undeclared key at both levels, so the guard cannot pass by never rejecting anything. ajv is now a declared devDependency of the CLI rather than borrowed transitively.

The dropped remote-mount filter

This is the one I am least comfortable about, because the failure mode is the one you caught me on last round: I dropped the filter in the extraction and the covering test was reworded until it passed either way. That is a test following the code instead of pinning it.

The skip is back, inside the shared collector, reusing the isRemoteOrInlineUrl predicate that already lives in that module. It is pinned at the collector level now, where a fixture cannot pass by accident:

https://host/remote.html, //host/protocol-relative.html, data:text/html,inline, compositions/local.html
  -> ["compositions/local.html"]

Your note that lint wants it too was the deciding argument: reporting a remote mount as "the file does not exist" is a false positive, and lint had it. Both callers get it from one place now.

registry_blocks_used empty against a non-zero count

Real trap, and worth more than a doc comment, so I took the boolean. registry_items_truncated: true is emitted whenever the name lists are a window rather than the whole set. The 45-item case now pins all of it: count 45, used count 5, 40 names, registry_blocks_used absent, registry_items_truncated true. A second case pins that it is absent when everything fits, so it cannot become a constant.

On the one-character regex

I want to flag a disagreement rather than quietly ship past it, because you both asked for [^<>] specifically and I did something else.

I had already pushed a tag-walking scan (indexOf to delimit each tag, bounded attribute regex inside it) before your review landed. It is more lines than yours. So I checked whether yours was strictly better, and there is one behavioral difference:

case                                old       [^<>]     committed
plain                               a.html    a.html    a.html
single quotes                       a.html    a.html    a.html
'<' inside an EARLIER attribute     a.html    a.html    a.html
'<' inside a LATER attribute        a.html    []        a.html

<div data-composition-src="a.html" title="a<b"> stops being found under [^<>], because the trailing [^<>]*> cannot cross the <. The old regex found it (only > was excluded), and the tag walk finds it. It is a narrow case, but it fails by silently dropping a mount that really does render, which is the same class of bug as the template blindness, and it would hit lint as well as telemetry.

Both are flat. Yours is smaller; mine preserves the prior semantics exactly, which is why I kept it. If you would still rather have the one-liner, say so and I will switch. Measured on the committed version: 20MB of < in 27ms, 20MB of <a in 25ms, 20MB of real mount tags in 91ms.

Your point about why the alert is newly introduced rather than relocated is the part I would have gotten wrong on my own. I would have reached for "the regex is unchanged and predates this branch". The export making html library input is the actual difference, and that reading is what makes the fix obviously in scope rather than arguable.

Verification

Per-package, run the way the repo runs them:

@hyperframes/cli       2846 passed, 3 skipped
@hyperframes/core      2532 passed
@hyperframes/parsers    928 passed, 4 skipped, 3 todo
@hyperframes/lint       531 passed
@hyperframes/producer   612 passed

Also worth correcting to you specifically, since you marked it "trusting": the studioServer.test.ts failure I reported in both earlier rounds was my own bad invocation, not a pre-existing defect. Running vitest from the repo root with a path filter picks up the wrong config, and the "86 sibling checkouts" figure was that same mistake fanning out. It passes under the package's own test script. Details are in my reply to Vai.

Scope note

The branch is at 1138 changed lines against a 1000 cap. Everything past the cap is review-driven correctness, which I would rather not split into a follow-up mid-review, but flagging it rather than letting it drift silently.