feat(core): sourceMutation data-hf-id targeting (R1, T7)

[vanceingalls]

What

Adds hfId targeting to sourceMutation (packages/core/src/studio-api/helpers/sourceMutation.ts) and closes a CSS injection gap.

New targeting field:

export interface SourceMutationTarget {
  id?: string | null;
  hfId?: string;        // ← new
  selector?: string;
  selectorIndex?: number;
}

findTargetElement tries hfId first via findByHfId, then id, then selector.

CSS injection guard:

function escapeCssAttrValue(value: string): string {
  return value.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); }

A crafted value like hf-1234"][data-x="y would otherwise produce a valid compound selector that silently matches the wrong element.

Stable id protection: data-hf-id is blocked in both attribute and html-attribute op cases — it can never be overwritten by a patch operation.

Split clone cleanup: splitElementInHtml removes data-hf-id from the cloned element so the next parse mints a fresh stable id for the new half.

probeElementInSource updated to accept hfId as a valid probe target.

Why

T7 spec — the server-side mutation API (files.ts route) uses sourceMutation to apply patches to composition source files. It needs to accept hfId targeting for the same reason as sourcePatcher (T3): stable R1 ids must be usable as mutation targets from all surfaces. Depends on PR #1270.

The injection fix is a security correctness issue: without it, a user-controlled hfId value could produce a CSS selector that matches an unintended element.

How

• escapeCssAttrValue: escapes \ before " (order matters — escaping " first would double-escape the backslash)
• findByHfId wraps the selector call in try/catch in case the escaped value still somehow produces an invalid selector
• splitElementInHtml clone: clone.removeAttribute("data-hf-id") before inserting — stable id must be unique; the parser will mint a new one on next round-trip

Test plan

• T7 spec tests in sourceMutation.test.ts — hfId targeting, injection guard, attribute protection
• 42 tests pass, 0 fail

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat(core): sourceMutation data-hf-id targeting (R1, T7) #1272 👈 (View in Graphite)
• feat(studio): sourcePatcher data-hf-id targeting (R1, T3) #1271
• feat(core): clip-model hf- ids minted at parse, emitted as data-hf-id (R1) #1270
• feat(core): ensureHfIds node-id pass + shared hf- mint helper (R1) #1269
• main

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

[miguel-heygen]

Best PR in the stack. The CSS injection guard is the right security catch — attribute-selector injection via a crafted hfId is a real risk if this surface is ever caller-controlled, and escapeCssAttrValue is correctly ordered (backslash first, then quote). The try/catch wrapper on querySelectorAllWithTemplates adds a clean second line of defense.

The data-hf-id write-protection and splitElementInHtml clone cleanup are exactly right — id uniqueness invariant is preserved, and the next parse will mint a fresh id for the split half.

One minor thing:

P3 — findTargetElement uses raw string interpolation, not escapeCssAttrValue:

const matches = querySelectorAllWithTemplates(document, `[data-hf-id="${target.hfId}"]`);

escapeCssAttrValue is defined in this file but not used in findTargetElement. If target.hfId comes from a caller (not always a round-tripped stable id), a value containing " or \ would produce a malformed CSS selector, and the try/catch on querySelectorAllWithTemplates would silently swallow the miss. Should be:

`[data-hf-id="${escapeCssAttrValue(target.hfId)}"]`

This closes the same gap escapeCssAttrValue was introduced to address.

[james-russo-rames-d-jusso]

Core-side mutation targeting. Symmetric with #1271's patcher but uses real DOM selectors via querySelectorAllWithTemplates (which handles <template> content, important for sub-compositions) rather than regex. The two preservation tests at the end are the most important contribution in the whole stack — they pin that data-hf-id SURVIVES a patch targeted by id or selector. Without those, R2+ write-back would silently destroy the stable handle on first edit.

Concerns

[CSS selector injection via the data-hf-id string interpolation] Line 36-37:

const matches = querySelectorAllWithTemplates(document, `[data-hf-id="${target.hfId}"]`);

If target.hfId ever carries a " or a closing-bracket / opening-bracket character, the selector parse goes sideways — either matches nothing, throws on invalid selector, or matches an unintended element if the attacker can craft the value. Right now callers are all internal and pass minted ids (which match /^hf-[a-z0-9]{4}$/), so the value is safe. But this PR is the foundation layer that future callers will build against, and once R2+ wires in user-facing flows (e.g. a Studio caller passing a value from an HTML form, or a serialized snapshot deserialized through a network boundary), the contract gets exercised more broadly.

Two defensive options:

1. CSS.escape if the runtime has it (node-DOM polyfills usually do). \[data-hf-id="${CSS.escape(target.hfId)}"]``.
2. Validate at the boundary: if (!/^hf-[a-z0-9]{4}$/.test(target.hfId)) return null; — rejects anything that doesn't match the spec, fails closed.

Either closes the loop. Cheap insurance.

[findTargetElement doesn't check multi-match for hfId] Same shape as the concern I raised on #1271's execDataAttrPattern: matches[0] is returned without checking matches.length > 1. By the mint contract this shouldn't happen, but the defensive check is one line:

const matches = querySelectorAllWithTemplates(document, `[data-hf-id="${escapedId}"]`);
if (matches.length > 1) {
  console.warn(`sourceMutation: hfId ${target.hfId} matched ${matches.length} elements; using first`);
}
if (matches[0]) return matches[0];

…or fail-closed if you'd rather. Worth doing — quietly patching one of two ambiguous matches is harder to debug than failing loudly.

Strengths worth calling out

[Two preservation tests close the most important gap] The last two tests:

it("preserves an existing data-hf-id when the element is patched by id", () => { ... })
it("preserves an existing data-hf-id when the element is patched by selector", () => { ... })

…with the comment "the stable handle is destroyed by the next edit. This is the preservation guarantee the write-back design depends on." Exactly the right contract to pin. These are the tests that make this PR safe to merge even though R2+ isn't wired yet — anyone refactoring the patch path will hit a red test the moment they break preservation. Excellent.

Nits

[probeElementInSource gate update] Line 213: if (!target.id && !target.hfId && !target.selector) return false; — correct, but the OR chain will grow if more target fields are added. A hasTargetFields(target) helper would scale better. Minor.

[hfId?: string vs id?: string | null mismatch] Same observation as on #1271 — the new field omits the null arm. Probably fine.

Verdict

Strongest PR in the stack from a testing perspective — the preservation tests pin exactly what R2+ depends on. The CSS injection defense and multi-match check are both 1-2 line additions worth landing before merge, but neither is structurally blocking the contract. Clean execution; leaving as a comment.

— Review by Rames D Jusso