Skip to content

feat: Update creative formats to match AdCP spec PR #10#13

Merged
bokelley merged 1 commit into
mainfrom
feature/creative-format-spec-updates
Aug 1, 2025
Merged

feat: Update creative formats to match AdCP spec PR #10#13
bokelley merged 1 commit into
mainfrom
feature/creative-format-spec-updates

Conversation

@bokelley
Copy link
Copy Markdown
Collaborator

@bokelley bokelley commented Aug 1, 2025

Summary

This PR updates the creative format structure to align with the AdCP specification changes from PR #10.

Key Changes:

  • Flattened Asset Structure: Removed nested requirements object
  • Asset Identification: Added asset_id field for unique asset identification
  • Required Field: Added required boolean to indicate mandatory vs optional assets
  • Direct Properties: Properties like duration_seconds, max_file_size_mb are now directly on assets
  • No Backward Compatibility: Removed legacy specs field entirely since this hasn't shipped yet

What Changed:

  1. Schema Updates (schemas.py):

    • Added new Asset model with flattened structure
    • Updated Format model to use assets array
    • Added asset_mapping to Creative for mapping uploaded content
  2. Foundational Formats (foundational_creative_formats.json):

    • All 5 formats now use the new asset structure
    • Each asset has asset_id, asset_type, required fields
    • Clear separation of different asset types (video, image, text, url, html)
  3. Format Manager (foundational_formats.py):

    • Updated to work with assets instead of specs
    • Modified extension mechanism for asset-based formats
  4. AI Service (ai_creative_format_service.py):

    • Updated to generate formats with asset structure
    • Added helper to create assets from AI analysis

Benefits:

  • ✅ Cleaner, more structured format definitions
  • ✅ Easier for orchestrators to understand requirements
  • ✅ Clear mapping between uploaded content and format requirements
  • ✅ Type-safe asset definitions
  • ✅ Aligns with AdCP spec exactly

Test plan

  • Run python3 test_format_json.py to verify JSON structure
  • Run python3 validate_format_models.py to validate model changes
  • Test with Docker deployment to ensure formats load correctly
  • Test AI format discovery generates correct asset structure
  • Verify existing media buy creation still works with new format structure

🤖 Generated with Claude Code

- Implement flattened asset structure removing nested requirements
- Add asset_id field for unique asset identification within formats
- Add required boolean field to each asset
- Update all 5 foundational formats to use new asset-based structure
- Remove backward compatibility with legacy specs field
- Update AI creative format service to generate asset-based formats
- Add validation tests for new format structure

This aligns our implementation with the AdCP spec changes for cleaner,
more structured creative format definitions that orchestrators can
easily understand and map uploaded content to.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
"duration_seconds": 30,
"max_file_size_mb": 10,
"additional_specs": {
"codecs": ["h264", "vp9"],
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't these be part of the asset definition? doesn't video asset have these?

"asset_id": "video_file",
"asset_type": "video",
"required": True,
"name": "Video File",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a name field? seems like description would make more sense

@bokelley
Copy link
Copy Markdown
Collaborator Author

bokelley commented Aug 1, 2025

Response to Review Comments

1. "shouldn't these be part of the asset definition? doesn't video asset have these?"

The video asset properties are correctly implemented according to the AdCP spec:

  • Direct properties (flattened): duration_seconds, max_bitrate_mbps, max_file_size_mb, acceptable_formats
  • Additional specs: aspect_ratios, resolutions, codecs, audio settings

This follows the spec's pattern of having common properties directly on the asset while format-specific details go in additional_specs.

Example from our implementation:

{
  "asset_id": "video_file",
  "asset_type": "video",
  "duration_seconds": 30,
  "max_bitrate_mbps": 10,
  "additional_specs": {
    "aspect_ratios": ["16:9", "9:16", "1:1", "4:5"],
    "resolutions": { ... },
    "codecs": ["h264", "vp9"]
  }
}

2. "is there a name field? seems like description would make more sense"

Both fields exist and serve different purposes per the AdCP spec:

  • name: Short, human-readable label (e.g., "Video File", "Product Images")
  • description: Explains the asset's purpose (e.g., "Main video creative file", "Collection of product images for the carousel")

This allows UIs to show a concise label while having detailed explanations available when needed.

The implementation aligns with the AdCP spec PR #10 requirements. Happy to make any adjustments if there are specific concerns!

@bokelley bokelley merged commit a3929e1 into main Aug 1, 2025
3 checks passed
@bokelley bokelley deleted the feature/creative-format-spec-updates branch August 1, 2025 11:09
bokelley added a commit that referenced this pull request Sep 15, 2025
…-spec-updates

feat: Update creative formats to match AdCP spec PR #10
KonstantinMirin added a commit to KonstantinMirin/prebid-salesagent that referenced this pull request Mar 31, 2026
Address all 39 findings from inspect_bdd_steps.py across 6 step files:

uc002_create_media_buy.py (10 findings):
- Assert packages exist instead of silent if-guard skips (#1, prebid#4, prebid#6, prebid#7)
- Strengthen success path with media_buy_id verification (#2)
- Add DB verification after seller rejection (#3)
- Re-commit creative.data mutation to DB (prebid#5)
- Document proposal SPEC-PRODUCTION GAP with implementation notes (prebid#8, prebid#9, prebid#10)

uc003_update_media_buy.py (4 findings):
- Add max_daily_package_spend comparison when tenant config available (prebid#12)
- Assert tenant exists for creative status validation (prebid#13)
- Validate placement_ids non-empty and against product config (prebid#14)

uc019_query_media_buys.py (5 findings):
- Verify principal_id consistency in Given steps (prebid#18, prebid#20)
- Validate date parsing in given_today_is (prebid#19)
- Strengthen adapter/snapshot setup documentation (prebid#21, prebid#22)

uc026_package_media_buy.py (2 findings):
- Verify pricing_options content matches step parameter (prebid#27)
- Verify format_ids content matches step parameter (prebid#28)

then_error.py (2 findings):
- Fix guard logic: allow Exception subclasses with .recovery (prebid#34)
- Use _get_error_dict for consistent field extraction (prebid#35)

then_media_buy.py (2 findings):
- Explicit error path in status check (no silent fallthrough) (prebid#36)
- Clarify seller event type assertions (prebid#37)

Remaining 14 findings use correct SPEC-PRODUCTION GAP xfail pattern
(try assertion first, xfail only on known gap with descriptive message).
ChrisHuie added a commit that referenced this pull request Apr 19, 2026
…uards (L0-01)

Per .claude/notes/flask-to-fastapi/L0-implementation-plan-v2.md §L0-01,
landing rows #1-#16 of the §5.5 Structural Guards Inventory plus guard #34
(no_werkzeug_imports, reassigned to L0-01 per v2 plan §7.1 RATIFIED).

Guards (each with AST scanner + main test + planted-violation meta-fixture):
- test_architecture_no_flask_imports.py                (#1, Captured→shrink)
- test_architecture_handlers_use_sync_def.py           (#2, frozen carve-out)
- test_architecture_no_async_db_access.py              (#3)
- test_architecture_middleware_order.py                (#4, DORMANT)
- test_architecture_exception_handlers_complete.py     (#5, DORMANT)
- test_architecture_csrf_exempt_covers_adcp.py         (#6, DORMANT)
- test_architecture_approximated_middleware_path_gated.py (#7, DORMANT)
- test_architecture_admin_routes_excluded_from_openapi.py (#8, DORMANT)
- test_architecture_admin_routes_named.py              (#9, DORMANT)
- test_architecture_admin_route_names_unique.py        (#10, DORMANT)
- test_architecture_no_module_scope_create_app.py      (#11, Captured→shrink)
- test_architecture_scheduler_lifespan_composition.py  (#12)
- test_architecture_a2a_routes_grafted.py              (#13)
- test_architecture_form_getlist_parity.py             (#14, Captured→shrink)
- test_architecture_no_module_level_engine.py          (#15, Captured→shrink)
- test_architecture_no_direct_env_access.py            (#16, Captured→shrink)
- test_architecture_no_werkzeug_imports.py             (#34, Captured→shrink)

Shared helpers in tests/unit/architecture/_ast_helpers.py (walk_py_files,
read_allowlist, relpath) per the DRY invariant in CLAUDE.md.

Captured→shrink baselines seeded:
- allowlists/no_flask_imports.txt           (40 files importing flask)
- allowlists/no_werkzeug_imports.txt        (6 files importing werkzeug)
- allowlists/module_scope_create_app.txt    (7 tests/*.py files)
- allowlists/no_form_getlist.txt            (3 admin routers)
- allowlists/no_module_level_engine.txt     (2 legacy gam services)
- allowlists/no_direct_env_access.txt       (121 path:line call sites)

Meta-guard `test_structural_guard_allowlist_monotonic.py` (§5.5 row #28)
enforcing allowlist monotonicity will land in a later L0 work item.

DORMANT guards (scanner present; repo state trivially compliant until
L0-04..L0-18 foundation modules land): #4, #5, #6, #7, #8, #9, #10. Each has
a planted-violation fixture that DOES fire the scanner; dormancy is about
the repo state, not scanner correctness.

Guard #15 landed as a new guard (the sibling test_architecture_no_module_level_get_engine.py
referenced in the plan does not exist in the pre-L0 state, so #15 is
not redundant with it).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ChrisHuie added a commit that referenced this pull request Apr 20, 2026
…failing

Part of L0-22. Adds tests/unit/harness/test_admin_env.py asserting the
admin-client harness extension contract (Red state, pre-implementation):

1. get_admin_client() returns a functional TestClient
2. Non-existent admin routes return 404 (empty router at L0)
3. override_dependency() context manager is visible in handlers and is
   restored on exit (Agent B Risk #13 isolation)
4. SessionMiddleware cookie round-trips across sequential requests
5. session_payload= kwarg pre-populates request.session
6. authenticated=True seeds the minimal admin session contract

All tests currently fail with AttributeError on get_admin_client because
the method does not yet exist. Green commit follows.

Refs: .claude/notes/flask-to-fastapi/L0-implementation-plan-v2.md §L0-22
      .claude/notes/flask-to-fastapi/flask-to-fastapi-execution-details.md A.11-A.12
      .claude/notes/flask-to-fastapi/flask-to-fastapi-deep-audit.md Agent-B Risk #13
ChrisHuie added a commit that referenced this pull request Apr 20, 2026
…isolation (L0-22)

Extends IntegrationEnv with get_admin_client() — a FastAPI TestClient
bound to an isolated per-env FastAPI() app that carries:

  * SessionMiddleware with session_cookie="adcp_session" (byte-locked
    rename from Flask's "session" per migration CLAUDE.md Invariant 5)
  * The empty build_admin_router() (L0-15 scaffold)
  * Per-env dependency_overrides with a snapshot/restore pair on
    __enter__ / __exit__ (Agent B Risk #13 isolation)

New APIs on IntegrationEnv:

  * get_admin_client(authenticated=False, session_payload=None) ->
    TestClient — lazy-builds the admin app the first time, re-uses on
    subsequent calls, and seeds the adcp_session cookie with the given
    payload via itsdangerous.TimestampSigner (matches SessionMiddleware
    wire format byte-for-byte).
  * admin_app (@Property) — exposes the env-owned FastAPI for tests
    that need to register probe routes.
  * override_dependency(key, fn) — context manager that installs a
    dependency_overrides entry scoped to a with block; the canonical
    path tests MUST use for overrides (guard-enforced in the next
    commit).

Decision: extend IntegrationEnv directly rather than adding an AdminEnv
subclass. The extension is ~160 LOC of straight-line code parallel to
get_rest_client(); a subclass would fragment the env hierarchy for no
functional benefit, and every existing integration env that inherits
IntegrationEnv gets the admin client for free.

Per flask-to-fastapi-execution-details.md §C risk row, the admin client
targets an isolated FastAPI() (NOT src.app.app). Rationale: at L0 the
root app has no SessionMiddleware, no admin router inclusion, and no
TrustedHost/UnifiedAuth layers. L1a will swap to src.app.app once
the real middleware stack is in place.

All 7 tests added in the Red commit now pass. No changes to any public
signature of IntegrationEnv — purely additive.

Refs: .claude/notes/flask-to-fastapi/L0-implementation-plan-v2.md §L0-22
      .claude/notes/flask-to-fastapi/flask-to-fastapi-execution-details.md A.11-A.12
      .claude/notes/flask-to-fastapi/flask-to-fastapi-deep-audit.md Agent-B Risk #13
ChrisHuie added a commit that referenced this pull request Apr 20, 2026
…13

AST-scans tests/ and fails if any test writes to
env.admin_app.dependency_overrides via raw subscript assignment, .update(),
.setdefault(), .pop(), or .__setitem__(). The canonical path is the
env.override_dependency(key, fn) context manager added in L0-22 Green.

The scoped context manager guarantees deterministic teardown on __exit__
regardless of test-body exceptions. Raw assignments leak across tests
under xdist worker reuse, surfaced in flask-to-fastapi-deep-audit.md
§582-584 as Agent B Risk #13 "harness overrides leakage".

The guard ships with a meta-fixture at fixtures/
test_harness_overrides_isolated_meta_fixture.py.txt that contains the
exact banned pattern; a meta-test asserts the scanner flags it, so the
guard cannot silently regress to "always pass" if the AST-matching logic
breaks.

No allowlist — there are no existing violations (the method was just
added in the Green commit).

Refs: .claude/notes/flask-to-fastapi/L0-implementation-plan-v2.md §L0-22
      .claude/notes/flask-to-fastapi/flask-to-fastapi-deep-audit.md Agent-B Risk #13
ChrisHuie added a commit that referenced this pull request May 22, 2026
…llow-up

Addresses items #6, #12, #16, #17, #19, #20, #23, #24 from the
2026-05-20 review (small mechanical fixes; no architectural change):

- #6 + #17: ``extract_error_info`` envelope branch now coerces
  ``recovery`` through ``_coerce_recovery()``, which validates the value
  against ``typing.get_args(RecoveryHint)`` instead of a hard-coded tuple
  duplicating the Literal. Both the envelope and legacy ToolError branches
  go through the same helper, so a future RecoveryHint extension is picked
  up automatically.

- #12: ``_body_contains_builder_call`` docstring now correctly says
  "N-level transitive call analysis with cycle detection via ``seen``"
  (was "1-level transitive", but the implementation actually walks the
  full call graph with cycle detection).

- #16: dropped the duplicate ``logger.warning`` in ``on_message_send``'s
  AdCPError handler — ``_handle_explicit_skill`` already logs the same
  error (with audit log + activity feed) before raising. Two log lines
  for the same failure was noise.

- #19: ``_translate_to_tool_error`` now uses ``raise error`` (not bare
  ``raise``) on its passthrough branches so the ``NoReturn`` contract
  holds even when called outside an active ``except`` block.

- #20: ``_handle_tool_error`` defensively copies ``e.envelope`` before
  returning to ``JSONResponse`` (preserves the envelope-builder's
  immutability contract — the dict is owned by the AdCPToolError instance
  and may be referenced elsewhere).

- #23: ``_handle_tool_exception`` Context extraction switched from
  ``hasattr(arg, "tenant_id")`` to explicit ``isinstance(arg, (FastMCPContext,
  ToolContext))`` — the hasattr check matched any Pydantic model with a
  ``tenant_id`` field, treating request bodies as Contexts.

- #24: hoisted ``from fastmcp.exceptions import ToolError`` to the top
  imports in ``src/app.py``; dropped the mid-file ``# noqa: E402``.

Local quality: 4682 passed, 1 skipped, 20 xfailed.

Deferred (separate scope):
- #5 byte-identical test, #7 delete per-boundary wrappers (30+ call
  sites), #9 decorator extraction, #10/14/25 move AdCPToolError to
  neutral module, #11 fail_step caller (#1311 coordination), #13
  CWD-relative paths, #15 dead legacy fallback, #18 already softened
  in b5c4356, #21 status_code keyword-only, #22 synthetic mutation.
ChrisHuie added a commit that referenced this pull request May 22, 2026
…llow-up

Addresses items #6, #12, #16, #17, #19, #20, #23, #24 from the
2026-05-20 review (small mechanical fixes; no architectural change):

- #6 + #17: ``extract_error_info`` envelope branch now coerces
  ``recovery`` through ``_coerce_recovery()``, which validates the value
  against ``typing.get_args(RecoveryHint)`` instead of a hard-coded tuple
  duplicating the Literal. Both the envelope and legacy ToolError branches
  go through the same helper, so a future RecoveryHint extension is picked
  up automatically.

- #12: ``_body_contains_builder_call`` docstring now correctly says
  "N-level transitive call analysis with cycle detection via ``seen``"
  (was "1-level transitive", but the implementation actually walks the
  full call graph with cycle detection).

- #16: dropped the duplicate ``logger.warning`` in ``on_message_send``'s
  AdCPError handler — ``_handle_explicit_skill`` already logs the same
  error (with audit log + activity feed) before raising. Two log lines
  for the same failure was noise.

- #19: ``_translate_to_tool_error`` now uses ``raise error`` (not bare
  ``raise``) on its passthrough branches so the ``NoReturn`` contract
  holds even when called outside an active ``except`` block.

- #20: ``_handle_tool_error`` defensively copies ``e.envelope`` before
  returning to ``JSONResponse`` (preserves the envelope-builder's
  immutability contract — the dict is owned by the AdCPToolError instance
  and may be referenced elsewhere).

- #23: ``_handle_tool_exception`` Context extraction switched from
  ``hasattr(arg, "tenant_id")`` to explicit ``isinstance(arg, (FastMCPContext,
  ToolContext))`` — the hasattr check matched any Pydantic model with a
  ``tenant_id`` field, treating request bodies as Contexts.

- #24: hoisted ``from fastmcp.exceptions import ToolError`` to the top
  imports in ``src/app.py``; dropped the mid-file ``# noqa: E402``.

Local quality: 4682 passed, 1 skipped, 20 xfailed.

Deferred (separate scope):
- #5 byte-identical test, #7 delete per-boundary wrappers (30+ call
  sites), #9 decorator extraction, #10/14/25 move AdCPToolError to
  neutral module, #11 fail_step caller (#1311 coordination), #13
  CWD-relative paths, #15 dead legacy fallback, #18 already softened
  in b5c4356, #21 status_code keyword-only, #22 synthetic mutation.
ChrisHuie added a commit that referenced this pull request May 23, 2026
Per the 2026-05-20 structural follow-up review on PR #1306, three Low
items previously deferred to follow-up work:

- #13: Architecture guard tests anchor scan paths on
  Path(__file__).resolve().parents[2] instead of CWD-relative
  Path("src/..."). Pytest from a subdir would otherwise silently match
  nothing and the guard would pass without scanning anything.
    - test_architecture_error_code_compliance.py: _SCAN_DIRS anchored
    - test_architecture_error_envelope_two_layer.py: _function_calls_builder
      resolves paths via _REPO_ROOT

- #15: Legacy flat-shape fallback in tests/harness/_base.py
  _envelope_to_adcp_error marked DEPRECATED with FIXME removal condition.
  Production envelopes use the two-layer adcp_error/errors[] shape; the
  fallback is only reached via the test-only helper _adcp_to_a2a_error,
  which still emits top-level error_code. Removal tied to that helper's
  deletion (planned in #7 batch).

- #21: AdCPToolError.__init__ now keyword-only on status_code. All 9
  existing callers already pass status_code= as kwarg, so no behavior
  change. Prevents a missing positional arg from silently defaulting to
  500 and misclassifying a 4xx as 5xx.

Local quality: 4682 passed, 1 skipped, 20 xfailed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant