fix(protocol): pin AdCP 3.1 stable#953
Conversation
e519e9b to
4ac58bb
Compare
There was a problem hiding this comment.
The spec-pin itself is sound — clean rc.13→3.1.0 regeneration with the right additive surface. Holding off on approve for one reason: the release handoff this PR describes contradicts the live state of the PR it hands off to, and the workflow auto-publishes on merge.
Things I checked
- Public surface is additive-only. 0 files removed across all 855;
public_api_snapshot.jsongains exactlyProvenanceDeclaredByandSiSponsoredContextDeclaredBy, no removals.fix(protocol):is the correct semver signal — no breaking change, no!needed. DeclaredBycollision wiring is internally consistent across all five touched files (consolidate_exports.py:96collision set →aliases.pyqualified imports +__all__→__init__.pyre-export block at L640/657, inside thefrom adcp.types.aliases importwindow, not the direct-generated_pocblock → snapshot →test_collision_aliases.py:60-61,165).code-reviewer: clean.- The two
DeclaredBytypes are genuinely distinct, so two qualified aliases is the right shape, not a union.core.provenanceRole{creator, advertiser, agency, platform, tool}vssi_sponsored_contextRole{brand_agent, seller, network, platform}— onlyplatformoverlaps. Merging would illegally widen each object'sroleenum on the wire.ad-tech-protocol-expert: sound. Same mechanism as the existingSignal/Unitqualified aliases. registry_event.pystatus: Literal['removed']narrowing is faithful to the 3.1registry-event.jsoncollection.removedarm (const: "removed", parentStatus | None). The# type: ignore[assignment]added bypost_generate_fixes.pyis error-code-scoped, not blanket — within the CI floor.- Forward-compat intact.
_forward_compat.py/_ergonomic.pyuntouched;UnknownFormatAsset/UnknownGroupAssetfallback arms still present (aliases.pyL1797-1870). TheRegistryEventN/PayloadNrenumber churn is expecteddatamodel-code-generatortraversal-order noise, not a semantic delta. - Generated diff is regeneration output, not hand-edited —
registry_event.pymatches the newpost_generate_fixesfunction exactly.
The open question (what flips this to approve)
dx-expert verified against the live repo:
- This PR body asserts pending Release Please PR #949 "should update to the stable 6.3.0 release."
- #949 currently reads
6.0.0-beta.1(manifest +pyproject6.0.0b1).mainis at6.3.0-beta.10; latest published tag isv6.3.0-beta.9. release-please.ymlrunsrelease_created→ build →twine uploadin the same job — merge of the RP PR auto-publishes with no gate.
So if #949 is merged in its current shape after prerelease mode is dropped, it most likely publishes a stable 6.0.0 — below the already-published 6.3.0-beta.9, non-monotonic and unyankable. Manifest history shows this class of footgun already fired once (55b5bfcf chore: keep release on 6.3 beta line, rescuing a 7.0.0-beta.7 miscompute).
This is not a defect in this diff — this PR doesn't bump the manifest, tag, or publish. But this PR is the trigger that changes RP's recompute, and the documented handoff is currently false against #949's state. What flips me to approve:
- Confirm that after this lands, RP re-runs and updates #949's title/manifest to
6.3.0(monotonic abovev6.3.0-beta.9) — and that #949 is not merged while it still reads6.0.0-beta.1. - (Optional, but it's what the prior rescue did) Anchor the base explicitly rather than trusting the suffix-strip to graduate cleanly.
Follow-ups (non-blocking — file as issues)
- A CI guard that refuses to tag/publish a version
<=the latest published tag would convert this whole class of release regression from a silent PyPI publish into a failed workflow. Two near-misses now. fix(protocol):yields a patch bump — confirm post-merge the tag lands where intended, since this is the prerelease→stable cutover.
Diff is sound; the hold is on the release-coordination question, not the code. Answer (1) and I'll approve.
4ac58bb to
b97ce8d
Compare
There was a problem hiding this comment.
Clean version pin. The bundle-key indirection is the right shape: collapsing 3.1.0 → 3.1 is exactly what makes the on-disk lookup find the regenerated bundle once the rc suffix drops.
Things I checked
resolve_bundle_keywiring is load-bearing and correct.resolve_bundle_key("3.1.0")→"3.1"via the no-prerelease branch (validation/version.py:69-73), which matches the shippedschemas/cache/3.1/layout. This isn't cosmetic — pre-pin the rc kept its full id (3.1.0-rc.13) as the cache key so the old raw-adcp_versionlookup worked; pinning to stable collapses the key to3.1, so the resolver is the fix, not decoration. Both lookup paths incanonical_formats/registry.py:54,74usebundle_key, and theFileNotFoundErrormessage carries it.- No import cycle.
adcp.validation.versionimports onlyre— the newfrom adcp.validation.version import resolve_bundle_keyinregistry.pycan't cycle back throughadcp.types/adcp.canonical_formats. - Public surface is additive-only.
public_api_snapshot.jsondiff is two adds (ProvenanceDeclaredBy,SiSponsoredContextDeclaredBy), zero removals — no required↔optional flip, no enum-member drop, no response-model reshape.fix(protocol):with no!is the correct semver signal for the negotiated wire contract. DeclaredBycollision is real, not a codegen artifact.ad-tech-protocol-expert: sound.core/provenancerole enum[creator, advertiser, agency, platform, tool]vssi_sponsored_contextrole enum[brand_agent, seller, network, platform]— onlyplatformoverlaps; unifying them would corrupt both. Disambiguation reuses the existing test-backed collision machinery (consolidate_exports.py:96, aliases wired across__init__.py/aliases.py/ snapshot /test_collision_aliases.py). Right shape.- The
# type: ignore[assignment]is a narrowing marker, not a masked bug.Payload4(CollectionPayload)narrows inheritedstatus: Status | NonetoLiteral['removed'];'removed'is a realStatusmember.ad-tech-protocol-expertconfirmed this rendersregistry-event.json'sallOf+const: "removed"faithfully. Specific code, not blanket. The fixer inpost_generate_fixes.py:3439is idempotent and applied at regeneration time — generated code stays generated. - Apparent field removals in
registry_event/tasks_get_responseare renumber-window churn.ad-tech-protocol-expertverifiedPayload6/Payload7still carry their original event payloads — the anonymous-variant numbers just shifted after the additivecollection.*branches landed. Discarded as churn per CLAUDE.md.
Follow-ups (non-blocking — file as issues)
- Semver prose vs prefix.
ad-tech-protocol-expertflagged that the regeneration pulls in net-new optional wire surface (SI sponsored-context family,registry_eventcollection.*event branches,DistributionIdentifierType:+youtube_channel_*,MediaBuy:+status_as_of). All additive, none breaking — but the most precise conventional-commit type for net-new fields/enum members isfeat:, notfix:. No wire defect either way, and since Release Please targets a minor bump (6.3.0) the version math lands correctly regardless. Worth a one-line policy confirmation, nothing more. - Package build unrun. PR body notes
python3 -m buildisn't installed in the workspace, so the local package build was skipped. Non-critical — Release Please owns publish — but CI should confirm the wheel builds against the Production/Stable classifier before #949 merges.
The version envelope negotiates at release precision (3.1), so rc.13 and stable advertise the same contract — bokelley.
Summary
3.1.0release and regenerate schemas/types from the verified protocol bundle.DeclaredBycollision handling and explicit aliases for provenance and SI sponsored context types.Validation
PYTHONPATH=src python3 -m pytest tests/test_schemas_version_pin.py tests/test_validation_version.py tests/test_adcp_version_option.py tests/test_dispatcher_version_routing.py tests/test_schema_validation.py tests/test_sync_schemas.py tests/test_collision_guard.py tests/test_collision_aliases.py -qmake validate-generatedPYTHONPATH=src python3 -m pytest tests/test_public_api.py tests/test_collision_aliases.py tests/test_type_aliases.py -qruff check scripts/consolidate_exports.py src/adcp/types/__init__.py src/adcp/types/aliases.py tests/test_collision_aliases.pyruff check scripts/post_generate_fixes.py scripts/consolidate_exports.py src/adcp/types/__init__.py src/adcp/types/aliases.py tests/test_collision_aliases.pyPYTHONPATH=src python3 -m pytest tests/test_schemas_version_pin.py tests/test_collision_aliases.py tests/test_public_api.py -qPYTHONPATH=src python3 -m pytest tests/test_canonical_formats_registry.py tests/test_canonical_formats_roundtrip.py tests/test_canonical_formats_v1_to_v2.py tests/test_decisioning_specialisms.py::test_signal_owned_manifest_exercises_discovery_only -qPYTHONPATH=src python3 -m pytest tests/test_schemas_version_pin.py tests/test_validation_version.py tests/test_dispatcher_version_routing.py tests/test_public_api.py tests/test_collision_aliases.py tests/test_normalize_pyproject_prerelease.py -qruff check src/adcp/canonical_formats/registry.py tests/test_decisioning_specialisms.pyLocal package build was not run because
python3 -m buildis not installed in this workspace.Release Notes
pyproject.toml,.release-please-manifest.json, andCHANGELOG.mdintentionally still carry the current beta package version in this spec-pin PR. Release Please owns those release-file updates.After this lands, the existing Release Please PR (#949,
autorelease: pending) should update to the stable6.3.0release because this PR removes prerelease mode fromrelease-please-config.json. Merging that Release Please PR is the step that should create the GitHub release/tag and publish to PyPI.