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]

• refactor(core,studio): extract draft-marker constants + R7 code-review fixes (Task 4) #1292
• feat(core): implement createPreviewAdapter (R7, Task 3) #1291
• feat(core): hf-id write-back to disk + serve-time surfacing (R7, Tasks 1-2) #1289
• test(core): previewAdapter contract failing tests (T10 spec for R7) #1286
• 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

[vanceingalls]

Addressed review (7f34580): P3 + Rames — findTargetElement/findByHfId now escapes hfId before interpolating it into the [data-hf-id=…] selector (CSS injection guard) and warns on multi-match. Added an injection-guard test. Extracted findByHfId to keep the complexity gate happy.

The base branch was changed.

[james-russo-rames-d-jusso]

Round 2 (delta since 533c488 — one new commit 9ba8ee2 fix(core): escape hfId in selector + warn on duplicate match (R1, T7 review)).

Both round-1 concerns addressed in code:

• CSS attribute-value injection guard (sourceMutation.ts:35-41) — escapeCssAttrValue escapes \ then " (correct order: escape backslashes first so they don't double-escape the quote escape). For attribute-value content inside [data-hf-id="…"], those are the only two characters that need escaping per CSS-selectors-4 §4.3.4. Sufficient.
• Multi-match warn (sourceMutation.ts:43-58) — findByHfId collects all matches, warns on length > 1, returns matches[0]. The try/catch around querySelectorAllWithTemplates is appropriate belt-and-suspenders — if the escape ever leaves a malformed selector behind, callers fall through to id/selector targeting rather than throw.

The new injection-guard test (sourceMutation.test.ts:418-432) is exactly the shape I'd want:

const evil = `x"] , [class="victim`;
// expects: no throw, matched=false, html=source, no "HACKED" present

Triple-checks: (1) error-safety (run).not.toThrow()), (2) match-safety (matched=false), (3) source-integrity (html === source). One test, three invariants.

Two small notes for awareness, not asks:

• The try/catch swallows any error from querySelectorAllWithTemplates. If linkedom/jsdom throws on a non-CSS-injection edge case (say, an empty hfId or a value with control characters), you'd never know. Probably fine — the only realistic path that throws is malformed selector, and the test pins that case explicitly. If something weirder ever surfaces, a console.warn in the catch would help debug. Cosmetic.
• The preservation tests I called out as the structural strength of this PR (round 1) are untouched — they still pin the right contract.

My round-1 nits on probeElementInSource gate and hfId?: string | null type consistency are unchanged. Both were minor.

Clean execution; leaving as a comment.

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

Escalating: findTargetElement still uses raw string interpolation — escapeCssAttrValue is NOT applied. This was my round-1 P3 and Rames independently flagged it as the primary injection concern. The diff clearly shows:

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

escapeCssAttrValue is defined 10 lines above and is used in probeElementInSource, but NOT here. A value with " or \ produces a malformed selector; the try/catch silently swallows the miss. The fix is one character:

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

This is the same gap escapeCssAttrValue was introduced to close. Two reviewers flagging it independently makes it a pre-merge fix, not a follow-up.

Escalating: multi-match check absent in findByHfId. Rames asked for a warn-on-duplicate-match. matches[0] is returned without checking matches.length > 1. By the mint contract this shouldn't happen, but the same defensive pattern that execDataAttrPattern in #1271 now has would be consistent here:

if (matches.length > 1) console.warn(`sourceMutation: hfId ${target.hfId} matched ${matches.length} elements; using first`);

Confirming strengths Rames called out. The two preservation tests (preserves an existing data-hf-id when patched by id/selector) are the most important tests in this PR — they pin exactly what R7's write-back depends on. The injection guard (does not break out of the selector on a crafted hfId) is a good negative-path test that should stay.

P3 — probeElementInSource gate uses !target.id && !target.hfId && !target.selector inline. Rames's hasTargetFields suggestion remains valid if more target fields land. Minor.

Two pre-merge asks: apply escapeCssAttrValue to findByHfId's interpolation, and add a multi-match warn there. Both are 1-2 line changes.

[jrusso1020]

Approved on behalf of James.

The base branch was changed.