[codex] Add canonical format curriculum coverage#5059
Conversation
There was a problem hiding this comment.
LGTM. Curriculum addition is intentionally scoped — closes #4694/#4698 and explicitly defers recertification wiring, third-party creative-agent flow, and buyer exercises to follow-up work. Migration 496 lands cleanly inside the runner's transaction at server/src/db/migrate.ts:130-142, and S2 is already seeded in 274_curriculum_revision.sql:763 so the RAISE EXCEPTION guards won't fire on fresh deploys.
Things I checked
- Migration idempotency: every mutation guarded by
NOT EXISTSor cardinality checks; re-run is safe (server/src/db/migrations/496_curriculum_3_1_canonical_formats_criteria.sql— key-concept guard, exercise guard, criterion guard). - Function lifecycle:
_update_s2_canonical_exerciseand_append_criterionareCREATE OR REPLACE'd, called, thenDROP'd in the same transaction. Advisory-lock serialization atmigrate.ts:206rules out cross-migration collision. - Schema-vs-docs coherence: no
static/schemas/source/**change. Thecreative_approval_moderewording atdocs/protocol/get_adcp_capabilities.mdx:202is a clarification, not a contract change. - Glossary alignment: S2 glossary entries (
docs/learning/specialist/creative.mdx:136-145) match the canonical spec atdocs/creative/canonical-formats.mdx:24-58. - Exercise renumbering 6→12 in
creative.mdx— no orphan cross-refs in the diff. - Three new criterion IDs are stable, namespaced, human-readable (
s2_ex1_sc_format_kind_selection,s2_ex1_sc_format_options_cardinality,s2_ex1_sc_source_taxonomy). - Changeset:
patchis right for additive curriculum/docs.
Follow-ups (non-blocking — file as issues)
- Criterion coverage gap. Lab exercise 6 has five sub-bullets (
docs/learning/specialist/creative.mdx:182-187) but the migration only ships three success criteria. The "canonical-first / product-second validation order" bullet — arguably the highest-leverage outcome, and the one most likely to drive recert when the canonical spec moves — has nosc_validation_ordercriterion. Either add it or drop the bullet from the lab. asset_sourcerubric drift. Rubric language atcreative.mdx:203says Addie evaluates "mapsasset_source" buts2_ex1_sc_source_taxonomyonly requiresbuyer_uploadedvsagent_synthesized. The glossary atcreative.mdx:145lists all five values. A learner could pass without ever demonstratingseller_pre_rendered_from_brief,seller_human_designed, orpublisher_host_recorded— three values that matter for podcast and generative-DSP flows.- Dangling fixture pointer. "The canonical fixture pool" at
creative.mdx:182isn't named. Fixtures live atstatic/examples/products/canonical/per the migration guide — link them and pre-identify which fixture is the multi-format display and which is the generative video product, otherwise the lab isn't runnable as written. - Tenant-ordering invariant.
ORDER BY tenant_idat496_*.sql:13discards the primary-tenant-first contract from464_certification_module_tenants.sql:7-10. It works by coincidence for S2 becausecreativesorts first alphabetically — notable, and a footgun for the next module that copies the pattern with a non-alphabetical primary. salestenant rationale. Migration expands S2 coverage tosalesalongsidecreativeandcreative-builder. Canonical-format authoring is a creative-side skill; ifsalesis in here because product authors touchformat_options[], say so in the migration comment.- Recertification trigger. PR body confirms wiring is deferred to refs #4695. Until that lands, the three new criterion IDs are stable but not yet load-bearing. Worth one line in the changeset so accreditors don't assume otherwise.
Minor nits (non-blocking)
- Tenant UPDATE always rewrites.
496_*.sql:8-16rewritestenant_idseven when no new tenants are added. Result is idempotent, but inconsistent with the explicitNOT EXISTSguards elsewhere in the same migration.
Approving.
There was a problem hiding this comment.
LGTM. Migration is correctly idempotent and the curriculum extension is well-scoped; follow-ups below are real but non-blocking.
Things I checked
- Migration 496 idempotency:
NOT EXISTSguards on thekey_conceptstopic match (496_curriculum_3_1_canonical_formats_criteria.sql:30-34),ILIKE '%format_options%'guard on the sandbox action (:62-66), andc->>'id' = p_criterion_idguard on each criterion append (:123-126). Safe to re-run. - Helper-function lifecycle: both
_update_s2_canonical_exerciseand_append_criterionare created and dropped within the file, matching migration 494's contract that these are not persistent fixtures. - Test expectation honesty:
['creative', 'creative-builder', 'sales']matches PostgresORDER BY tenant_idASCII collation (-= 0x2D,c<s). The assertion atcertification-module-tenants.test.ts:59will pass. - Failure modes:
RAISE EXCEPTIONon missings2_ex1(:81-83,:138-140) follows the precedent set by migration 494 — abort-loud, don't silently no-op. - Changeset wording and
patchbump are correct: additive curriculum content with no wire-shape change. - Depth gradient: A2 gets one objective and one glossary row (shallow orientation); S2 gets the lab exercise, glossary, and three new success criteria (deep). Right shape.
Follow-ups (non-blocking — file as issues)
- Tenant-order fragility. The
ORDER BY tenant_iddedup atmigrations/496:8-16produces['creative', 'creative-builder', 'sales']only becausec < slexically. A futureaddressableaddition would silently demotecreativeto second, and the migration 464 contract was "primary order preserved." Switch to a first-seen-order dedup (array_agg ... ORDER BY MIN(ord) FROM unnest WITH ORDINALITY) so the intent survives the next tenant addition. - Assessment weight didn't move with scope.
creative.mdx:201renames the 25% row "Cross-platform" → "Cross-platform and canonical formats" but the weight stayed at 25%. Four canonical-format competencies (format_options[]reading,format_kindselection,asset_sourcemapping, product narrowing) now share a quarter of the grade with cross-platform adaptation. Either split into two graded rows or raise the combined row to ~30% and rebalance — leaving it ambiguous invites Addie to re-weight ad hoc. v1_format_ref[]is tested but not taught.s2_ex1_sc_format_options_cardinality(migrations/496:151-152) requires explaining multi-size fan-out, butv1_format_ref[]is not in the S2 glossary (creative.mdx:137-145). Either add a glossary row or scope the criterion to single-option vs multi-option.capability_idvsformat_option_idhas no criterion. The glossary atcreative.mdx:143-144explicitly warns against substituting one for the other — exactly the namespace confusion the protocol most wants to prevent — but exercise #6 has no bullet and no success criterion exercising the distinction. A learner can pass all three criteria without ever demonstrating they tell the two IDs apart.
Minor nits (non-blocking)
- Title/description overwrite is inconsistent with the
sandbox_actionsguard pattern.migrations/496:58-59unconditionallyjsonb_sets{title}and{description}ons2_ex1whilesandbox_actionscorrectly guards withalready_present(:62-76). Idempotent in value, but a human edit between runs would be silently reverted. Either guard the title/description writes the same way or note in a comment that S2 exercise 1 prose is migration-owned. - Agent-surface table is now stale.
creative.mdxaddssalestotenant_idsbecause exercise #6 readsformat_options[]from a sales-agent product, but the agent-surface table aroundcreative.mdx:128-132still ties sales to exercise 3 only. One-line consistency fix. creative_approval_moderewording is an unrelated commit.get_adcp_capabilities.mdx:202changes "Capability declaration" → "Discovery-time capability signal" — a fine clarification but orthogonal to the canonical-format theme of this PR. Worth its own line in the changeset prose if it stays.
LGTM. Follow-ups noted below.
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Idempotent data migration on a shared seed table, additive curriculum coverage, and the test expectation is the single source of truth for the tenant change.
Things I checked
- Migration 496 mirrors the established 407 pattern: tenant_ids update guarded with
IS DISTINCT FROM, key_concept append guarded withNOT EXISTS, sandbox-action append guarded withalready_present, criterion append guarded byidexistence. Temporary_update_s2_canonical_exercise()and_append_criterion(...)are both dropped at the end. Re-running the migration is a no-op. RAISE EXCEPTIONon missing S2 / s2_ex1 is consistent with407_curriculum_3_0_criterion_ids.sql:26-28,51-53. The seed comes from274_curriculum_revision.sql, so ordinal-order migrations guarantee the row exists when 496 runs.certification_modulesis a shared seed table — single-row update byid='S2'has no cross-tenant surface. Addingsalesto tenant_ids is the right shape: the lab asks creative learners to readget_productsformat_options[]on a sales agent.server/tests/integration/certification-module-tenants.test.ts:59is the only assertion of S2tenant_idsin the suite (grep confirmed acrossserver/tests/andserver/src/).- All four internal doc link targets resolve. Both fixtures cited in lab #6 (
static/examples/products/canonical/nytimes_homepage_mrec.json,veo_generative_video_15s.json) exist and carry theformat_options[]/format_kind/asset_sourcefields the exercise operates on. - Teaching glossary at
docs/learning/specialist/creative.mdx:138-145does not contradict normativedocs/creative/canonical-formats.mdx. The orientation row ata2-protocol-architecture.mdx:61sits at a different altitude and does not duplicate. - Changeset type
patchmatches the additive, non-wire-touching scope. - No
static/schemas/source/**changes, so no spec-vs-docs drift surface.
Follow-ups (non-blocking — file as issues)
- Rubric does not name two of the four new criteria. Migration adds
s2_ex1_sc_format_options_cardinality(496_curriculum_3_1_canonical_formats_criteria.sql:149) ands2_ex1_sc_validation_order(:155), but the published rubric row atdocs/learning/specialist/creative.mdx:203only namesformat_options[]/format_kind/asset_sourceand product narrowing. Cardinality (format_option_idselection, single-vs-multi-option) and canonical-first validation order are testable in the criteria but invisible to the learner — surfacing them closes the criterion-to-rubric loop and reduces appeal risk. - Glossary misses two terms the lab exercises.
v1_format_ref[]is invoked in lab #6 bullet 4 (docs/learning/specialist/creative.mdx:186) but absent from the glossary table at:138-145. "Manifest asset map" is operationalized at:184without a definition. Both belong in the teaching glossary so the lab is self-contained.
Minor nits (non-blocking)
creative_approval_modewording is a stylistic outlier.docs/protocol/get_adcp_capabilities.mdx:202reads "Discovery-time capability signal," while sibling sections on the same page (idempotencyat L89-91,reporting_delivery_methodsat L185-187) use "Declares..." framing. Accurate as written; "Declares whether auto-approval-dependent behavior applies" would parallel the rest of the page.
There was a problem hiding this comment.
LGTM. Follow-ups noted below.
Curriculum docs and the DB-backed runtime curriculum change together — the right shape. Migration 496 is idempotent, well-guarded, and the five new criterion IDs are stable for the recertification wiring that lands later.
Things I checked
- Migration 496 idempotency:
tenant_idsUPDATE guarded byIS DISTINCT FROM(L13-14),key_conceptsappend guarded byNOT EXISTS(L27-32), criterion appends keyed by stable IDs (L122-126). Helper functionsRAISE EXCEPTIONif S2 ors2_ex1is missing. Re-runnable. _append_criterion(L95-146) shares its signature with the helper in migration 407 — bothDROP FUNCTIONat file end, no cross-migration residue.s2_ex1.success_criteriais heterogeneous (string entries from migration 274,{id,text}objects from 407 onward). Thec->>'id'idempotency probe correctly returns NULL on string entries, so the appends still go through without clobbering legacy rows.- S2 assessment weights sum to 100%: 20 + 15 + 10 + 25 + 15 + 15 (
docs/learning/specialist/creative.mdx:204-210). - Exercise renumber 6→7..11→12 in
creative.mdx:194-199is consistent with the agent-surface table atcreative.mdx:130-132— the new exercise 6 is sales-anchored, so the existing "Sales agent ... Exercises 3, 6" mapping still holds. - Test expectation
server/tests/integration/certification-module-tenants.test.ts:59matches the migration target array['creative','creative-builder','sales']. - Fixtures referenced by the lab exist and expose
format_options[]:static/examples/products/canonical/nytimes_homepage_mrec.json(multi-option) andveo_generative_video_15s.json(single-option) — gives the lab a real negative case for the "explain why a single-option product may not expose a selector" beat atcreative.mdx:187. creative_approval_modewording tightening atdocs/protocol/get_adcp_capabilities.mdx:202is prose-only; the schema description atstatic/schemas/source/protocol/get-adcp-capabilities-response.jsonis unchanged. No drift.- No
static/schemas/source/**changes — patch-level changeset is the right type.
Follow-ups (non-blocking — file as issues)
- Exercise-title drift between docs and runtime. The mdx lab calls exercise 6 "Canonical-formats authoring" (
creative.mdx:184), whilemigration 496:58rewritess2_ex1.titleto "Creative production and canonical-format authoring" and merges the lab into ex1's sandbox actions. Docs present a standalone exercise 6; the runtime sees a merged ex1. Pick one — rename the mdx heading or split the runtime exercise. - Cross-platform weight at 15% now spans six sub-skills (format discovery, adaptation, compliance, preview modes, tracker audit, broadcast identifiers) at
creative.mdx:207. Worth re-splitting into sub-competencies or restoring weight before the next bar-raise. - Criterion rubrics are explanation-graded. The five IDs at
migration 496:148-161use "explains / distinguishes / chooses" verbs without a named submission artifact. Attach a graded artifact (filled asset-map JSON, or written rationale of stated length) before this becomes the canonical-formats recertification gate — otherwise contract-cheat resistance is weak. - Recertification trigger wiring is explicitly deferred to the follow-up issue. Fine for shipping the criterion IDs, but the next 3.1.x canonical-format wire change can't invalidate prior certifications without it. Track as load-bearing for the broader 3.1 cert story, not just curriculum polish.
Minor nits (non-blocking)
- Unconditional
jsonb_setof title/description.migration 496:58-59rewritess2_ex1.titleanddescriptionon every execution. The inline comment at L56-57 calls this out as intentional ("migration-owned"). In normal forward-only operation this fires once. The only way it bites is if an operator manually re-runs an already-applied migration — at which point it would silently overwrite any production edits to those fields. Acceptable per the annotation; worth keeping in mind for future migrations that touch the same JSONB columns.
Approving on the strength of the matched docs/runtime change and idempotent migration.
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Patch changeset is right — no wire shape moves, just curriculum + a migration-owned exercise rewrite.
Things I checked
- Migration 496 idempotency:
tenant_idsupdate guarded byIS DISTINCT FROM,key_conceptsappend guarded byNOT EXISTS, sandbox action guarded byILIKE/tool=match,_append_criterionguarded by ID lookup. Title/description ons2_ex1is deliberately re-asserted on every run; the comment atserver/src/db/migrations/496_curriculum_3_1_canonical_formats_criteria.sql:38-40calls that out. Pattern matches407_curriculum_3_0_criterion_ids.sqlprecedent. RAISE EXCEPTIONpaths fire if S2 ors2_ex1is missing — fail-closed beats silently no-op'ing a 3.1 curriculum push.- Test parity:
server/tests/integration/certification-module-tenants.test.ts:59updated to the new['creative','creative-builder','sales']tuple. Searchedserver/**for callers that depend on the prior two-element shape —training-agent/config.tsconsumes the array shape-agnostically, no other callers pin the length. - Exercise renumbering 6→7..11→12 in
docs/learning/specialist/creative.mdx: greppeddocs/**andserver/**for old numeric IDs, nothing depends on them. Endpoint table at line 130 was updated in step ("Exercises 3, 6"). Weights sum to 100% (20+15+10+25+15+15). - Criterion IDs (
s2_ex1_sc_format_kind_selection,_format_options_cardinality,_option_vs_capability_id,_source_taxonomy,_validation_order) match the{module}_{exercise}_sc_{concept}convention from 407 and don't collide with the four S2 IDs added there. creative_approval_modereword indocs/protocol/get_adcp_capabilities.mdx:202— clarification, not semantic drift. New wording aligns with the schema's "Tenant-wide applicability signal" description verbatim.- Fixtures cited in Exercise 6 exist:
static/examples/products/canonical/nytimes_homepage_mrec.json,veo_generative_video_15s.json.
Follow-ups (non-blocking — file as issues)
asset_sourcetaxonomy is under-fixtured. Criterions2_ex1_sc_source_taxonomygrades on all five values (buyer_uploaded,agent_synthesized,seller_pre_rendered_from_brief,seller_human_designed,publisher_host_recorded), but the two cited fixtures only exercise the first two.static/examples/products/canonical/the_daily_30s_host_read.jsonalready exists and would close the host-recorded gap. Without it Addie is grading by recitation, not by reading.- Validation order is asserted, not done. Exercise 6 bullet 6 and the matching criterion ask the learner to explain canonical-first/product-second validation, but no step makes them perform it. A concrete two-step walk-through against the MREC fixture (canonical
imagefirst, thenparams.max_file_size_kb=200) would turn an explanation criterion into a demonstration. - Docs rubric is ahead of the DB.
creative.mdx:206-210now lists six dimensions includingPricing and accounts 15%andCanonical formats 10%; the DB row from migration 274 still has the four-dimension shape and Addie surfaces those verbatim. This is widened by the PR but not introduced by it, and the PR body marks recertification wiring as deferred. Worth a tracking issue so the doc/DB rubrics reconverge.
Minor nits (non-blocking)
format_optionsvsformat_options[]notation.docs/creative/canonical-formats.mdx:30uses the bare form; the new A2 entry (a2-protocol-architecture.mdx:61) and S2 glossary (specialist/creative.mdx:140) use the bracketed form. Normalize to the array-suffix form across all three — easier for agents grepping schema shape.- Order-sensitive
tenant_idsguard.496_*.sql:13compares against the literal array['creative','creative-builder','sales']. Safe today becausetenantUrlsForModulereads primary-first, but a future reorder elsewhere would silently re-trigger the update. Worth a comment so the next author doesn't trip the guard. - Long rubric cell.
creative.mdx:207'sCanonical formats"What Addie evaluates" cell is one sentence with five clauses. Breaking into 2–3 sentences would read better in the rendered table. format_option_idglossary. Could note the "SHOULD be set on every entry" guidance fromcanonical-formats.mdx:35so learners don't assume the field is only set on collision.
The migration-owned exercise prose pattern is a notable choice — re-running 496 silently restores s2_ex1 title/description, so any future docs-only edit to that row will need a follow-up migration to stick. Flag in the PR body or a CONTRIBUTING note if it isn't already.
Approving.
Summary
496to project the S2 update into DB-backed curriculum data, including the runtime lesson plan, exercise metadata, sales/creative tenant coverage, and three stable criterion IDs.format_options[],format_kind, and canonical-first/product-second validation.creative_approval_modeas a discovery-time capability signal, addressing the review note that it is not a notification workflow.Closes #4694.
Closes #4698.
Refs #4695: adds the three fixture-backed S2 criterion IDs, leaving third-party creative-agent flow, preview-status criteria, and recertification trigger wiring for separate work.
Refs #4697: adds buyer-track orientation, but leaves the requested buyer exercise/glossary/error-code treatment open.
Validation
git diff --checknpm run test:migrationsnpm run test:docs-navnpm run check:owned-linksnpm run lint:schema-linksnpm run test:test-dynamic-importsnpm run test:unitnpm run typechecknpm run test:snippetsstill fails repo-wide on existing snippets outside this diff: missing Pythonadcppackage, unsetADCP_AUTH_TOKENbash examples, existingget_media_buy_deliverymissing-buy examples, andsync_creativesSDK validation failures.