refactor(lint): consolidate asset-src placeholder skip into a shared predicate - #2894

R1 review @ c7d8f5213

Clean execution of the helper-extraction I suggested in #2893 R1. Load-bearing details all check out:

• isUnresolvedAssetPlaceholder(rawSrc) at assetResolution.ts:41-43 folds the __UPPER__ check (with .trim()) and hasUnresolvedTemplatingToken into one predicate, with a docstring that explicitly states the raw-value requirement. The .trim() scope is right — it applies only to the anchored __UPPER__ regex (which needs the whole string to be the placeholder), not to the templating regex (which matches anywhere in the string, so audio/${name}.mp3 still trips).
• Every asset-src site routes through it now. The four project.ts lint sites, hevcPreviewLint.ts, and the two previously-drifting non-lint sites mediaCodecMap.ts:229-233 and htmlCompiler.ts:169-172 — all use the same predicate on the raw value. The stale htmlCompiler.ts comment ("matches lint's skip" — a lie by omission before this PR, as I flagged in R1) is now honestly annotated as "shared with lint via @hyperframes/parsers".
• Test coverage for the shared helper is genuinely thorough. assetResolution.test.ts covers __UPPER__ (raw + padded), the tricky cases that motivated Miguel's R1 blocker (${asset?.url}, ${a ?? b}, ${u}?v=1), and — nice defensive test — audio/${name}.mp3 (embedded token in an otherwise path-shaped value). Real paths, remote URLs, empty string, and whitespace-only all correctly return false.
• Design call to NOT fold the remote/inline check in is correct — the audio-src site uses a narrower http/data/blob allowlist while everyone else uses isRemoteOrInlineUrl, so keeping those as per-call-site checks avoids papering over an asymmetry that's actually meaningful.

Nits

• hasUnresolvedTemplatingToken remains exported from assetResolution.ts but every previous external consumer now routes through isUnresolvedAssetPlaceholder. If nothing outside assetResolution.ts still calls it, worth either dropping the export or marking it @internal so future contributors don't reach past the composite helper. grep -r "hasUnresolvedTemplatingToken" packages/ should confirm.
• mediaCodecMap.ts and htmlCompiler.ts get no dedicated regression test in this PR — the argument for skipping them is fine (both are post-substitution paths, so no false-positive exists today), but a token-src smoke test for each would lock in the invariant symmetrically with the lint suite. Purely defensive; not a blocker.

Questions

• The PR description notes it's stacked on #2893 with base retargeting to main once #2893 merges. Since #2893 R2 still has Miguel's CHANGES_REQUESTED open at the R1 SHA (his ?-split concern is fixed on the current head but the stamp hasn't been refreshed), is the plan just to wait on Miguel's re-review, or should #2894 be reviewed as if it'll land on top?

What I didn't verify

• Full lint + parsers + studio-server + producer typecheck locally (trusting the "green, 511 tests" claim in the PR body).
• That mediaCodecMap.ts and htmlCompiler.ts truly never see a raw templating token in the wild (both are post-substitution today, but if a partially-substituted path ever slips through, the new skip would kick in — that's the "can't drift again" property the refactor is buying).

LGTM from where I sit — clean follow-up.

— Review by Rames D Jusso

R1 review @ c7d8f52

Verdict: APPROVE — extracts a shared placeholder predicate, correctly routes all sibling call sites through it, tests cover the tricky raw-vs-cleaned edge case.
Grade: A Rubric: CORRECT

Approach. Adds isUnresolvedAssetPlaceholder(rawSrc) in packages/parsers/src/assetResolution.ts:41 (OR of the __UPPER__ shape after .trim() and the existing hasUnresolvedTemplatingToken). Routes 7 asset-src skip sites through it: 4 in packages/lint/src/project.ts (286, 329, 381, 531), packages/lint/src/hevcPreviewLint.ts:91, packages/studio-server/src/helpers/mediaCodecMap.ts:234, and packages/producer/src/services/htmlCompiler.ts:172. Lint sites are behavior-preserving (equivalent OR/order); the two non-lint sites gain the templating-token check they lacked.

Findings.

• P1: none.
• P2: none.
• Non-blocker: mediaCodecMap.ts:234 and htmlCompiler.ts:172 now check hasUnresolvedTemplatingToken on raw, which is new behavior at those sites. PR body claims "post-substitution, so they don't false-positive today" — plausible (both run after substitution) and strictly safer (would only cause a skip, never a spurious flag). Would be nice to have one integration-shaped test at htmlCompiler proving a ${foo} data-composition-src no longer throws; the parser-level unit tests plus the "@hyperframes/lint suite green, 511" claim in the body cover it, so not a blocker.
• Non-blocker: the four lint sites that used to check __UPPER__ after cleanAssetUrl(src) now check it on the raw src (with .trim() inside the predicate). Since __UPPER__ values contain no ?/#, the two paths are equivalent; the trim inside the predicate is a strict superset. Confirmed by re-reading each callsite (project.ts:329, 381, hevcPreviewLint.ts:91 — all pass rawSrc before cleanAssetUrl; project.ts:286 passes src = match[1]! which is also raw).

Test coverage. packages/parsers/src/assetResolution.test.ts:24-57 adds a new describe("isUnresolvedAssetPlaceholder") block: positive tests for __UPPER__ (raw + whitespace-padded), for <<>>/{{}}/${} tokens, and — importantly — for ${asset?.url} and ${u}?v=1 (the values that would be broken by cleanAssetUrl() splitting on ?, which is why the predicate must run pre-clean); negative tests for real paths (audio/clip.mp3, clip.mp4?v=1), remote (https://…, //host/…), empty and whitespace. Good coverage of the exact hazard the raw-vs-cleaned split introduces. No golden updates needed (behavior-preserving at lint sites).

Sibling / parity. All three Rames-named sites are hit: hevcPreviewLint.ts:91, mediaCodecMap.ts:234, htmlCompiler.ts:172. I grepped the tree for any remaining asset-src __[A-Z_]+__ skip patterns — the only non-migrated matches are unrelated build-time sentinels (__CLI_VERSION__, __HF_MEDIA_CODEC_MAP__, __NEXT_DATA__, __HYPERFRAMES_RUNTIME_CDN_URL__, __STUDIO_VERSION__, __HF_ROOT_PROPS__, etc.) and a copy inside a lint error-message string at packages/lint/src/project.ts:265 (not a skip check). No missed asset-src callsite. Full sibling parity achieved.

Stack context (vs #2893). Base branch of #2894 is 07-24-lint_skip_templating_token_audio_src, which is #2893's head branch — confirmed by gh pr view 2893. Stacked in Graphite. Diff of #2894 alone is scoped to the refactor described in the body (no bleed-over from #2893's changes). Once #2893 merges, base auto-retargets to main. mergeStateStatus: UNSTABLE reflects in-flight regression shards, not a real block.

Verdict rationale. Meets the "approve" trigger verbatim: shared isUnresolvedAssetPlaceholder helper and all three sibling sites Rames named. Tests present and target the exact ?-inside-${…} failure mode. Refactor is strictly-safer at the lint sites and adds defensive coverage at the two non-lint sites.

— Via

Exact-head review at c7d8f521.

The extraction is complete and contract-preserving. packages/parsers/src/assetResolution.ts:41-42 centralizes padded __UPPER__ and templating-token recognition while preserving raw-value inspection; tree-wide grep shows every prior asset-src __UPPER__ skip now routes through it. packages/parsers/src/assetResolution.test.ts:24-56 pins the tricky optional-chaining, nullish-coalescing, query-suffix, embedded-token, real-path, and remote-URL cases. Producer and studio-server already depend on the exported parser subpath, so the new imports do not introduce a package-boundary change.

nit: packages/parsers/src/assetResolution.ts:22-24 still says the older __UPPER__ shape keeps an inline check at each call site. This PR intentionally removes those inline checks, so that parenthetical is stale and can be cleaned up before merge or in a follow-up.

All 20 exact-head checks are terminal: 18 success, 2 intentionally skipped, no failures or pending jobs.

Verdict: APPROVE
Reasoning: The shared predicate closes the cross-package drift identified in #2893 without changing remote/inline semantics, and direct tests pin the preprocessing edge cases that caused the original false positive.

— Magi

Re-approved at exact head 109797e. The current six-file diff preserves the reviewed shared raw-src placeholder predicate across all seven call sites, and the only follow-up commit corrects the stale documentation aside. All exact-head checks are terminal success/skipped.