test(creative): add canonical supported formats storyboard#5147
Merged
Conversation
Contributor
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape: advertise what you actually implement, fail-closed on unknown build targets, partition 3.0 from 3.1 at the resolver. The "training agent only ships one canonical (image) and rejects the rest" posture matches how a real 3.1 creative agent should bound its catalog.
Things I checked
ad-tech-protocol-expert: sound-with-caveats.capability_idreuse astarget_format_id.idis conformant —format-id.jsonaccepts the slug, agent owns its namespace, andcreative-manifest.json:67-86correctly partitions 3.1 (format_kindarm) from legacy (format_idarm). The PR's emitted{ format_kind: 'image', assets }is the 3.1 arm exactly.FORMAT_NOT_SUPPORTEDerror envelope is conformant — opencodevocabulary,recovery: correctablein the taxonomy,details.accepted_valuesis the SHOULD-level canonical key, and per-format attribution viafield: target_format_ids[${index}](task-handlers.ts:193, :245) matchesbuild-creative-response.json's mandate.- Fail-closed in 3.0 compat is correct: 3.0 buyers can't validate the
format_kindmanifest shape, so acceptingtraining_image_generationas a forward-compat extension would silently bind them to a manifest they can't parse. Test at training-agent.test.ts:4376 confirms. - Changeset
minoris correct for this surface. Old behavior —validFormatIds.get(unknown)→ undefined → silent default 300x250 manifest — was lazy passthrough, not a documented contract. Narrowing it isn't a wire break. - Largest-file rule: new compliance YAML is 212 lines. Read end-to-end; scenario flow (discover → build → reject) is coherent and the
expect_error: true/negative_path: payload_well_formedframing on the rejection phase is right.
Follow-ups (non-blocking — file as issues)
- Mode 1 library-retrieval path now resolves
creative.formatIdthroughresolveTarget. task-handlers.ts:5969 (was unconditionalvalidFormatIds.get). If a library creative was synced under aformatIdthat isn't ingetFormats()and isn't aliased,build_creative({creative_id})flips from "silent 300x250 manifest" toFORMAT_NOT_SUPPORTED. Probably fine for storyboard-seeded creatives, but worth either an inline assertion or a test pinning the library-creative path against a known-goodformatId. BUILD_CREATIVE_FORMAT_ALIASES['display_300x250_generative'] -> 'display_300x250'shadows an existing registered format atserver/src/shared/formats.ts:521. Same dimensions, so today the shadow is invisible; but the registered generative format has a different asset shape (brief/logo/click_url). If anything readsformat.assetsoff the resolved entry, the alias silently returns the wrong declaration. Either drop that one alias entry (registered format resolves on its own) or comment why dimension-passthrough must win.- Missing test for multi-target mid-batch rejection. Implementation correctly attributes per-format error via
target_format_ids[${index}](task-handlers.ts:193, :245), but no test in the newcanonical creative build capabilitiesblock exercises a mixed-good/bad batch. Worth one regression case assertingerrors[0].field === 'target_format_ids[1]'for the bad slot. - Compliance scenario
field_absent: creative.supported_formats[1].capability_id(canonical_supported_formats.yaml:463-465) locksteps the storyboard to "exactly one advertised capability." When a second canonical lands inSUPPORTED_CANONICAL_BUILD_CAPABILITIES, this storyboard breaks for unrelated reasons. The two[0]assertions above it (capability_id, format_kind, format_option_id absent) already cover the wire-shape check the storyboard's narrative claims. The[1]check is a training-agent fixture assertion living in a compliance scenario. Move it to the unit test (where the equivalentbuild_html5absence check already lives at training-agent.test.ts:4341) or drop it. canonical_catalog_version: '3.1'paired with one-entrysupported_formats(task-handlers.ts:5342-5343). The schema description framescanonical_catalog_versionas "runtime catalog the agent was built against" — typegen alignment. But a buyer reading{ canonical_catalog_version: 3.1, supported_formats: [image] }will reasonably read "this agent implements 3.1 and only supports image builds." Pre-PR the field was paired with all canonical kinds; the asymmetry now is new. Worth confirming the intended semantic inbuild_creative.mdx.- Document
target_format_id.idacceptingcapability_idslugs.ad-tech-protocol-expertflagged this is folklore — conformant but never written down. One sentence indocs/creative/task-reference/build_creative.mdxmakes it normative instead of inferred.
Minor nits (non-blocking)
buildManifestelse-if is dead. task-handlers.ts:154-159 routes anytarget.formatKind !== 'image'to HTML assets underformat_kind: target.formatKind. The onlyformatKindever set is'image'(SUPPORTED_CANONICAL_BUILD_CAPABILITIEShas one entry). Either delete the branch and let the type guard force the next contributor to think about asset shape per canonical, or comment that it's a stub.buildCanonicalImageAssetshardcodes the acme-outdoor fixture — task-handlers.ts:84-102.headline: 'Trail-ready gear for every summit',landing_page_url: https://acmeoutdoor.example/trail-pro,image url: .../acme-outdoor/banner_300x250.jpg. Other build modes threadreq.brandandreq.creative_idinto the HTML payload; the canonical image path is brand-locked. Either thread the brand through or rename tobuildAcmeOutdoorImageFixtureso the next reader knows it's a stub.- Conventional-commit prefix. PR title is
test(creative): …, but the change also narrows the advertised catalog, introduces alias-map resolution, and converts threebuild_creativemodes from silent-passthrough to fail-closed. Notable choice —feat(creative)would have telegraphed the behavior change. Changeset itself is correctlyminor, so the wire-versioning story is intact.
LGTM. Approving on the strength of the protocol-conformance verdict plus the fail-closed posture across the three build_creative modes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 3.1 creative-agent storyboard coverage for canonical
creative.supported_formatsbuild capabilities.capability_idvaluesbuild_creativethrough the advertised canonical image capability and emits a canonicalformat_kindmanifestFORMAT_NOT_SUPPORTEDCloses #4591.
Validation
npm run typechecknpx vitest run server/tests/unit/training-agent.test.ts --config server/vitest.config.tsnpm run test:storyboard-context-output-pathsnpm run test:storyboard-sample-request-schemanpm run test:storyboard-validations-pathsnpm run test:storyboard-response-schemanpm run build:compliancenpm run test:storyboardsnpm run test:storyboards:3.0-compat