feat: add promoted template edit contracts - #3407
Conversation
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head c25c2088f4fa8e3f0aadbbfd7d3c449267f9829e.
The ownership split is coherent across all nine promoted templates: editable content/brand slots are represented by root data-composition-variables, while each TEMPLATE.md explicitly protects host chrome, typography, palette, geometry, motion, and timing. ChatGPT, Claude, Slack/iOS, notes, messages, and share-sheet contracts correctly treat the referenced website as subject content rather than owner of the surrounding product UI. The four templates that needed declarations now read those defaults through the runtime variable API without moving structural or motion code.
Registry coverage pins one packaged TEMPLATE.md, one composition, and a non-empty root declaration for every promoted identity; the full lint/runtime/catalog/Windows/regression suite is green.
Rollout note: this approval covers source head c25. The internal promotion PR must update its source pin to the final merged HyperFrames SHA before publishing artifacts.
Verdict: APPROVE
Reasoning: The source contracts draw the requested ownership boundary clearly, variable wiring stays inside declared content/brand slots, protected host identity remains byte-owned by the templates, and exact-head CI is fully green.
— Magi
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Strengths
TEMPLATE.mdcontracts are added for all 9 promoted templates with a consistentSurface ownership / Editable slots / Protectedstructure, andregistryBlocks.test.ts:32-55enforces presence of exactly one contract file plus at least one non-emptydata-composition-variablesdeclaration per template.- The
data-composition-variablesJSON on<html>is the actual machine-readable enforcement surface (checked downstream by hyperframes-internal#624 and experiment-framework#47354), not just prose — good separation between human doc and machine contract.
Important
packages/cli/src/registry/registryBlocks.test.ts:13-23— the set of "promoted" templates is a hand-maintained array (promotedTemplateIds) with no link to the registry metadata (nopromoted: trueflag or similar inregistry-item.json, confirmed by grepping tags onai-chat-reveal/registry-item.json, which only has free-formtags). This list has to stay in lockstep with the independent list hyperframes-internal'spromote-oss-templates.tspromotes. If a template is added to the internal promotion list but someone forgets to add its id here, this suite silently stops enforcing the contract on it — no CI signal on either side. Recommend deriving "promoted" from a field onregistry-item.json(single source of truth) instead of two independently maintained lists in two repos.chatgpt-exchange.html/claude-exchange.htmldeclaresearch/thinking/promptetc. as editablestringcontent variables, but the values flow intoel(cls, html, tag)→n.innerHTML = htmlsinks (e.g.mkToolbuilding`<div>${label}</div>`fromCONFIG.search/CONFIG.thinking). That means an "editable content" value can structurally alter the DOM (tag injection) unless something else blocks it. I verified experiment-framework#47354's gate (template_editor_gate.py:534-535) hard-rejects anystringdefault containing</>, which closes this specific vector — but that protection lives entirely in EF, one repo away from where the sink is. Worth a code comment in the two.htmlfiles noting these are innerHTML sinks and depend on the delivery gate's</>ban, so a future template author doesn't add a similar sink without that same protection in mind.
Notes
- Contract markdown isn't cross-validated against the actual variable list (test only checks 3 heading strings are present +
declarations.length > 0), so aTEMPLATE.mdcould claim a slot is protected/editable inconsistent with whatdata-composition-variablesactually declares. Low risk since the real enforcement point is the HTML declaration, not the prose, but the prose is what the editor-agent's system prompt is told to read first. - CI is green on all required checks;
mergeStateStatus: BLOCKEDis purelyREVIEW_REQUIRED, not a failing check.
Verdict: APPROVE
Reasoning: Core mechanism (declared-variable contract) is sound and tested; findings above are process/robustness improvements, not correctness bugs in this diff.
— Vai
|
Addressed the review hardening on head e22d6e3. Contract coverage is now derived from the existing ad-template registry tag instead of a handwritten ID list, so newly promoted templates automatically enter the gate. The ChatGPT and Claude innerHTML helpers now document their dependency on the downstream angle-bracket rejection. Focused registry tests, formatting, lint, typecheck, and pre-commit checks pass. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e22d6e35013a4906e540bd2982c51e3d80fbd1cc.
The prior process gap is closed: contract coverage now derives the promoted roster from the shared ad-template registry tag instead of a second hand-maintained ID list. That is the same metadata the internal promotion layer now validates. The innerHTML sinks in ChatGPT/Claude are explicitly documented beside the code and remain protected by the downstream angle-bracket gate.
The ownership contracts and variable wiring are otherwise unchanged from the approved head. All 8 required contexts are green at this exact SHA.
Verdict: APPROVE
Reasoning: The source ownership boundary remains sound, and promoted-template coverage is now single-sourced from registry metadata rather than a duplicated roster.
— Magi
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
APPROVE at e22d6e35 — independent adversarial pass.
Scope of this PR (narrow reading): adds a TEMPLATE.md ownership contract to 9 promoted templates, wires data-composition-variables + window.__hyperframes.getVariables() into 4 templates (chatgpt-exchange, claude-exchange, heygen-avatar-promo-card, slack-notification-ad), and adds a registry test that requires (a) an ad-template-tagged block, (b) a TEMPLATE.md file entry, and (c) data-composition-variables.length > 0 on <html>. The catalog-parity / main-reachable pin gate / export terminal-status fix claims from the stack summary live in siblings #624 (HFI) and #47354 (EF), not here.
No blockers. Non-blocking notes for follow-ups:
- P2 (defense-in-depth): XSS surface expansion.
chatgpt-exchange.html,claude-exchange.html, andslack-notification-ad.htmlfeed variable content inton.innerHTMLvia the newcontent()helper. Before this PR, CONFIG was author-baked at bundle time; after this PR, values arrive fromvariables[id], expanding the injection surface from author-only to any producer that can set the variables map. Safety now rests on an off-PR delivery gate ("rejects angle brackets") that is not exercised in this PR's tests.heygen-avatar-promo-cardcorrectly usestextContent— worth normalizing the other three the same way, or adding an in-template escape. - nit: presence-only registry-test assertion.
expect(declarations.length).toBeGreaterThan(0)passes for[{}]— no shape assertion on id/type/role/label/default. Mutation-escapable; a schema check on each declaration would harden the contract runtime code relies on. - nit: tag-only contract filter.
manifest.tags?.includes('ad-template')is the sole gate. A partial drift (a promoted template loses the tag) exits the check silently; the outerpromotedManifests.length > 0only guards against ALL templates losing it. Consider a "TEMPLATE.md present ⇔ ad-template tag present" cross-check. - nit: helper-name drift. chatgpt/claude/slack use
content(); heygen-avatar-promo-card usestext()+ a trivialcolor()alias. Not a bug, but future authors copy-pasting between templates will fork on the name. - nit: empty-string coerces to fallback.
variables[id].trim() ? variables[id] : fallbackprevents a producer from legitimately blanking a slot. Probably intentional for length-lock reasons — worth documenting in TEMPLATE.md.
Review by Via
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
APPROVE at e22d6e35 — independent adversarial pass.
Scope of this PR (narrow reading): adds a TEMPLATE.md ownership contract to 9 promoted templates, wires data-composition-variables + window.__hyperframes.getVariables() into 4 templates (chatgpt-exchange, claude-exchange, heygen-avatar-promo-card, slack-notification-ad), and adds a registry test that requires (a) an ad-template-tagged block, (b) a TEMPLATE.md file entry, and (c) data-composition-variables.length > 0 on <html>. The catalog-parity / main-reachable pin gate / export terminal-status fix claims from the stack summary live in siblings #624 (HFI) and #47354 (EF), not here.
No blockers. Non-blocking notes for follow-ups:
- P2 (defense-in-depth): XSS surface expansion.
chatgpt-exchange.html,claude-exchange.html, andslack-notification-ad.htmlfeed variable content inton.innerHTMLvia the newcontent()helper. Before this PR, CONFIG was author-baked at bundle time; after this PR, values arrive fromvariables[id], expanding the injection surface from author-only to any producer that can set the variables map. Safety now rests on an off-PR delivery gate ("rejects angle brackets") that is not exercised in this PR's tests.heygen-avatar-promo-cardcorrectly usestextContent— worth normalizing the other three the same way, or adding an in-template escape. - nit: presence-only registry-test assertion.
expect(declarations.length).toBeGreaterThan(0)passes for[{}]— no shape assertion on id/type/role/label/default. Mutation-escapable; a schema check on each declaration would harden the contract runtime code relies on. - nit: tag-only contract filter.
manifest.tags?.includes('ad-template')is the sole gate. A partial drift (a promoted template loses the tag) exits the check silently; the outerpromotedManifests.length > 0only guards against ALL templates losing it. Consider a "TEMPLATE.md present ⇔ ad-template tag present" cross-check. - nit: helper-name drift. chatgpt/claude/slack use
content(); heygen-avatar-promo-card usestext()+ a trivialcolor()alias. Not a bug, but future authors copy-pasting between templates will fork on the name. - nit: empty-string coerces to fallback.
variables[id].trim() ? variables[id] : fallbackprevents a producer from legitimately blanking a slot. Probably intentional for length-lock reasons — worth documenting in TEMPLATE.md.
Review by Via
3 participants
Summary
Validation
Notes