fix: harden media buy response normalization#846
Merged
Conversation
d6f69ef to
94ba8e9
Compare
94ba8e9 to
532459a
Compare
There was a problem hiding this comment.
Clean follow-up. Centralizes the legacy lifecycle inference set in one place and aligns the dict-based MCP boundary with the typed-validator behavior — coherence win.
Things I checked
MEDIA_BUY_LEGACY_STATUS_VALUESatsrc/adcp/types/media_buy_status_helpers.py:13-22matches the upstreamMediaBuyStatusenum minuscompleted. Droppingdraftis correct (not in the upstream enum at all). Dropping Britishcancelledis correct (upstream uses Americancanceledonly)._normalize_response_envelopeatsrc/adcp/server/mcp_tools.py:67-71collapses to the right shape now thatcompletedis no longer in the legacy set — the prior innerif raw_status != "completed":guard is semantically equivalent to the new code, but only because the frozenset excludes it. Worth a code-side comment so a future re-add doesn't quietly break it.- Generated-file regeneration audit:
scripts/post_generate_fixes.py:1515-1537media_header/update_media_headerproduce exactly what landed insrc/adcp/types/generated_poc/media_buy/create_media_buy_response.pyandupdate_media_buy_response.py. TheSequenceimport shows up only in the update variant, as expected. - Layering: new
media_buy_status_helpers.pyhas zerogenerated_poc//_generatedimports;mcp_tools.pyreaches it throughadcp.types(public surface). Layering rule holds. - Validator branch ordering at
create_media_buy_response.py:35-53/update_media_buy_response.py:37-55has no fall-through bugs; each arm copies before mutating. - New
elif raw_status == "completed":arm is the enum→string normalizer forMediaBuyStatus.completedconstructor inputs.test_typed_success_does_not_infer_completed_lifecycleattests/test_server_dx.py:189-194and:248-251exercises it. UpdateMediaBuyResponse3re-export atsrc/adcp/types/aliases.py:1880is non-breaking — already insrc/adcp/__init__.py.__all__pergit show HEAD~. Intermediate layer now matches.
Follow-ups (non-blocking — file as issues)
- Warn-mode wire-conformance gap.
ad-tech-protocol-expertflagged two: (a){"task_id": ...}with no status now falls through to a status-less response (mcp_tools.py:73-74), and (b) handler-returnedstatus="draft"ships unchanged in warn mode (tests/test_schema_validation_server.py:278-298codifies this). The previous behavior was also wrong (syntheticcompletedstamp, or rewritingdraftintomedia_buy_status), but both flavors leave the MCP boundary willing to emit a status that isn't inTaskStatus. Decide the contract: either defaultstatus="submitted"whentask_idis present, or fail in both modes on a non-TaskStatusvalue. Same question for Britishcancelled— file a small follow-up if a tolerance window is intended. - Default-status loss for non-media-buy methods.
mcp_tools.py:73changed fromresult.setdefault("status", "completed")(global default) to a guarded form. The guard targets the task-envelope case, but any future tool returning{"task_id": ..., "status": <something>}withstatusaccidentally omitted will silently miss thecompleteddefault. Worth a one-line comment scoping the guard's intent. - Changelog note for the behavior change. Adopters returning
status="draft"orstatus="cancelled"(British) previously got auto-normalized; they now hit validation. The PR description covers this — the released changeset should too so adopters know to fix their handlers.
Minor nits (non-blocking)
- Comment the "no-op" completed branch.
create_media_buy_response.py:43-45,update_media_buy_response.py:45-47, and the mirror sites inscripts/post_generate_fixes.py:1935-1937/:2008-2010—elif raw_status == "completed": data["status"] = "completed"reads as a no-op until you remember it's theMediaBuyStatus.completedenum→string coercion. One-line comment saves the next reader an audit. __all__re-export style.src/adcp/types/__init__.py:790-797usesas NAMEforMEDIA_BUY_LEGACY_STATUS_VALUESbut not forunwrap_enum_value. Pick one — the explicit-re-export idiom for type checkers is to useas NAMEon both.- Test-only.
tests/test_schema_validation_server.py:278-298confirms the draft payload survives unmodified but doesn't assert the validation warning was actually logged. Acaplogassertion or a strict-mode counterpart would lock in the "validation is the enforcement point" contract.
LGTM. Follow-ups noted.
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
Follow-up to #842 for the non-blocking Argus cleanup items that were worth doing now.
draft,cancelled, or ambiguouscompletedas inferredmedia_buy_statustask_idasstatus="completed"UpdateMediaBuyResponse3public for source compatibility while preservingUpdateMediaBuySubmittedResponseas the semantic aliasValidation
PYTHONPATH=src python3 -m ruff check src/adcp/types/media_buy_status_helpers.py src/adcp/types/aliases.py src/adcp/types/__init__.py src/adcp/__init__.py src/adcp/types/generated_poc/media_buy/create_media_buy_response.py src/adcp/types/generated_poc/media_buy/update_media_buy_response.py scripts/post_generate_fixes.py tests/test_server_dx.py tests/test_schema_validation_server.pyPYTHONPATH=src python3 -m pytest tests/test_schema_validation_server.py tests/test_server_dx.py tests/test_import_layering.py tests/test_public_api.py tests/test_create_media_buy_response_types.py tests/test_type_guards.py tests/test_code_generation.py -qPYTHONPATH=src python3 -m pytest tests/test_schema_validation.py tests/test_protocols.py -qPYTHONPATH=src python3 -m mypy src/adcp/PYTHONPATH=src python3 -m mypy --strict tests/type_checks/python3 scripts/post_generate_fixes.pyblack,ruff,mypy,bandit, whitespace/EOF/JSON/large-file/conflict/key checks