fix: coerce malformed tags blob in list_creatives (untyped-blob crash) (#1508)

[mkomorski]

Summary

Closes #1508. Follow-up from PR #1493 review (@ChrisHuie); pre-existing, was out of scope for #1407.

_list_creatives_impl built the response Creative by piping the untyped JSON data blob straight into the typed Creative.tags field:

tags=db_creative.data.get("tags") if db_creative.data else None,

Creative.tags is typed list[str] | None, so a malformed blob value — a bare string, or a list with numeric / object elements ([1, 2], [{...}]) — raised a pydantic ValidationError (tags / tags.0) during response construction and 500'd the entire listing (VALIDATION_ERROR), filtered or unfiltered, on all transports, on one bad row. PR #1493 fixed the sibling concept_id/concept_name hazard via _coerce_concept_value but left tags (the list-field sibling) unprotected — a FIXME(#1508) marked the exact spot.

Fix

Generalize the coercer into a shared helper (per the issue's DRY guidance and CLAUDE.md's DRY invariant):

• _coerce_blob_scalar(value, field_label) — the scalar variant (formerly _coerce_concept_value), now field-labeled for accurate logs.
• _coerce_concept_value(value) — a thin wrapper over _coerce_blob_scalar(value, "concept"), preserving its exact "Dropping non-scalar concept value…" log message.
• _coerce_blob_str_list(value) — the list variant: a non-list value drops to None with a warning; within a list each element is coerced via _coerce_blob_scalar (scalars stringified, non-scalars dropped + logged), so [1, 2] -> ["1", "2"] and [{...}] drops the bad element. An emptied/empty list collapses to None so exclude_none omits the key — mirroring the concept fields' drop-to-absent.

No Quiet Failures: every drop is surfaced in logs.

Spec grounding

Authoritative version: AdCP 3.1.0-beta.3 (pinned adcp==5.7.0, ref v3.1-04f59d2d5). The pinned creative/list-creatives-response.json types creatives[].tags as {"type": "array", "items": {"type": "string"}} — i.e. list[str]. This change keeps the response conformant to that declared type: for well-formed data the wire is unchanged; only out-of-spec corrupt internal blob data (which no schema permits on the wire) is dropped rather than crashing the listing. Robustness/defensive fix, not a request/response contract change. Ungraded by any conformance storyboard (none grades corrupt-internal-data robustness); it is the direct list-field analogue of the concept coercion @ChrisHuie requested in #1493.

Testing

• New regression tests in tests/integration/test_list_creatives_concept_filter.py::TestMalformedTagsBlobCoerced (reuse the file's existing seed helper — no duplicate scaffolding), mirroring TestNonScalarConceptValueDropped:
  • a non-list tags blob (bare string) lists without crashing; the key is dropped; the drop is logged.
  • a tags list of [1, "keep", {"k": "v"}] lists without crashing; wire tags == ["1", "keep"]; the dropped dict is logged.
• Falsifiability: reverting the _coerce_blob_str_list call back to the raw data.get("tags") reddens both new tests with the exact pydantic string_type error (verified).
• make quality — 5239 passed, 0 failed (incl. the DRY duplication guard and all structural guards); the concept coercion file's 10 tests pass (refactor preserved behavior + the exact concept log message).

🤖 Generated with Claude Code

[KonstantinMirin]

Review — PR #1616

Overview — This is the right fix, done the right way: the shared scalar coercion is extracted into _coerce_blob_scalar, all three blob read sites (tags, concept_id, concept_name) route through it, the FIXME(#1508) marker is retired with the fix, and the new tests drive the real transport harness and assert on the wire, including the warning log. Two things need fixing before merge: the docstring-claimed empty-list-to-absent collapse is new wire behavior with zero coverage, and the two new tests copy-paste the seed-and-list scaffold instead of extracting it.

Should fix

1. New behavior documented but unpinned — the or None collapse has no test.
_coerce_blob_str_list's docstring claims "An emptied (or already-empty) list collapses to None so exclude_none omits the key." That is a real wire change introduced by this PR — before, a stored tags: [] serialized as "tags": []; now it is omitted. Neither new test reaches that line: one seeds a bare string (non-list early return), the other seeds a list that stays non-empty. Mutating return coerced or None → return coerced leaves both tests green. Add one test seeding data={"assets": {}, "tags": []} (or tags=[{"k": "v"}], which exercises the emptied branch through the same line) asserting "tags" not in creative on the wire. For the record: the pinned schema (dist/schemas/3.1.0-beta.3/creative/list-creatives-response.json, AdCP 3.1.0-beta.3 per docs/adcp-spec-version.md) permits both [] and omission — the choice is production's to make, but the choice made must be pinned.

2. Test scaffold copy-pasted — copies 3 and 4 of the seed-one-creative-and-list pattern.
Both new tests repeat, line for line, the scaffold that already exists twice in this file (TestNumericConceptCoercion, TestNonScalarConceptValueDropped): enter CreativeListEnv, seed principal, build one CreativeFactory(format="display_300x250", status="approved", data=...), call env.call_via(Transport.REST) under caplog, assert not result.is_error, read wire_response["creatives"][0]. Only the data literal and the final assertions differ — the CLAUDE.md DRY case exactly. Extract a module-level helper (e.g. _list_single_creative_with_data(data, caplog) -> result) and route both new tests through it; migrating the two pre-existing copies is cheap and in the same file, but the new tests must not add copies 3 and 4. This also removes the duplicated in-body import logging / from tests.factories import CreativeFactory blocks.

Nice to have

• _coerce_blob_str_list(value, field_label: str = "tags") — a generically-named, generically-documented helper with a field-specific default, while its sibling _coerce_blob_scalar requires the label. A future second list[str] blob field that omits the argument would mislabel its warnings as tags. Make the label required and pass "tags" at the call site, matching the scalar helper's contract.
• The new tests dispatch REST only, but _ALL_WIRE is defined and used three classes up in the same file, and "tags" not in creative is a per-transport serialization claim (exclude_none surviving each wire projection). Parametrizing over _ALL_WIRE is near-zero cost. (The REST-only shape matches the #1407 sibling guards, so this stays optional.)
• str(True) == "True" is code-commented as deliberate and now reachable through the list path, but unasserted. Adding True to the mixed-element fixture (expect ["1", "keep", "True"]) pins it for free.
• Corrupt-blob handling is spec-silent and the sibling precedent is integration-harness grading, so no BDD scenario is required — but a locally-added scenario next to BR-UC-018-list-creatives.feature would align this with the story-grounding playbook if you want it wired across all four transports.

Notes / prior-review follow-ups

No prior review rounds on this PR. Credit where due: the spec-grounding section in the PR description (pinned version, exact schema file, "ungraded" noted) is the model shape; no allowlist or duplication-baseline growth; inline FIXME debt goes down by one, retired in the same PR that fixes it.

[KonstantinMirin]

return coerced or None is the docstring-claimed empty/emptied-list-to-absent collapse — new wire behavior (a stored tags: [] used to serialize as "tags": [], now omitted) with no test reaching this line. Mutating it to return coerced leaves both new tests green. Add one test seeding data={"assets": {}, "tags": []} (or tags=[{"k": "v"}]) asserting "tags" not in creative on the wire.

[KonstantinMirin]

Generically-named and generically-documented helper, but field_label defaults to the specific field "tags" — while the sibling _coerce_blob_scalar requires the label. A future second list[str] blob field that omits the argument would mislabel its warnings as tags. Make the label required and pass "tags" explicitly at the call site.

[KonstantinMirin]

This is copy 3 of the seed-one-creative-and-list scaffold already present twice in this file (TestNumericConceptCoercion, TestNonScalarConceptValueDropped) — only the data literal and the final assertions differ. Extract a module-level helper (e.g. _list_single_creative_with_data(data, caplog) -> result) owning the env/seed/caplog/call/not result.is_error block, and keep only the fixture and the distinct wire+log assertions here.

[KonstantinMirin]

Copy 4 of the same scaffold — route through the shared helper proposed above. Optional bonus while here: add True to the mixed-element fixture (expect ["1", "keep", "True"]) to pin the code-commented str(True) choice.