feat(registry): community-mirror lifecycle + upsert client (JS #2183/#2187 parity)#929
Conversation
|
Heads-up for reviewers: the community-mirror lifecycle types here are dict[str, Any] as a deliberate stopgap. The registry OpenAPI spec does not define these endpoints, so there is no generated adcp.types.registry model to use yet (the adjacent create_adagents is dict for the same reason). Filed upstream adcontextprotocol/adcp#5384 to add the endpoints + schemas to static/openapi/registry.yaml. Once that lands, run make regenerate-registry and swap the dicts for the generated types. Tracking: #925. |
|
Noted — Generated by Claude Code |
|
Stopgap resolved. adcontextprotocol/adcp#5385 (tracking the #5384 ask) is merged, adding the community-mirror lifecycle endpoints + schemas to
Spec provenance: vendored from adcp
Full local gate green: Generated by Claude Code |
|
Acknowledged — Generated by Claude Code |
There was a problem hiding this comment.
Clean additive vendoring of the registry community-mirror lifecycle. Right shape: the public surface only grows — new RegistryClient methods, a new build_community_mirror_adagents builder, and new generated DTOs — so feat(registry): is the correct semver signal and there's nothing breaking on the wire.
Things I checked
- Additive enums, no removed arms.
DiscoveryMethodgainsadagents_authoritative+community_catalog,Status1gainscommunity,Source5gainscommunity— all on deserialization-side response models (PublisherLookupResult,Property1,AdagentsJson). Adding members only widens what an adopter can parse; nothing removed, no discriminator-key change.ad-tech-protocol-expert: sound. get()catalog-only invariant moved to the type layer. The dropped manualauthorized_agents-empty check is correctly replaced byCommunityMirrorAdagentsJson.authorized_agentscarryingmax_length=0; a non-empty list raisesValidationError→_parsewraps to...invalid response..., andtest_rejects_non_catalog_mirror/test_rejects_malformed_success_responseswitched their match accordingly.max_length=0is a load-bearing protocol invariant of the mirror contract (authorized_agents: []forced server-side), not incidental strictness — right call, and correctly scoped to the mirror DTO only (AdagentsJson/CommunityMirrorCatalogDocumentkeep an unbounded list).delete()409 path._request_okwith defaultexpected_status=200raisesRegistryError(status_code=409)before_parseruns;test_maps_409_not_superseded_to_registry_errorasserts exactly that.force→{"force": "true"}threads through_request'sclient.request(...)branch.- Platform inference + consistency assert.
_community_mirror_platform_from_config(config.platform → single consistentproperties[].platform→ ambiguity error) and the pre-flight property-vs-platform assert both fire before the HTTP call;test_rejects_property_platform_mismatch/test_rejects_ambiguous_property_platformsassertrequest.assert_not_called(). - No public export removed.
CompanySearchResult(types/registry.py:1211) andFindCompanyResult(:1479) both survive — the schema$refswap is path-inline only.supported_versionsarray→string is an unnamed path-response property, not a generated public model field. No deserialization break. - Rename-map pins. The five
CommunityMirrorPublishRequest{1..5}anyOf variants andAdagentsJson1get stable rename entries — the correct defense against the codegen-renumbering churn CLAUDE.md warns about. - Test plan honest. Wire-shape assertions against mocks are the right level for an HTTP client (per CLAUDE.md); no unchecked manual-verification box claiming an unvalidated path.
tests/test_registry.py:test_exposes_wrapper_superseded_bycorrectly pins the dropped-hydration behavior (result.superseded_byset,result.adagents_json.superseded_by is None).
Follow-ups (non-blocking — file as issues)
apply_renamescorrupts generated prose.scripts/generate_registry_types.pyrewrites class names with a globalre.sub(r"\b{old}\b", new, content)across the whole file — includingField(description=...)/examples=[...]literals. This PR adds generic English words (Brand,Format,Collection,Data,Success,Severity) to that map, and the fallout is already in the diff:Policyexample now reads"CreateAdagentsData subjects must provide... consent"andFormatSummaryreads"ResolvedPropertyEntry IDs this format applies to". Strings only — no broken identifiers, no validation/wire impact, tests + typecheck pass — but the current mitigation is one-offstr.replacepatches infix_spec_reality_gaps(it already carries"Founding AgentMember" → "Founding Member"). Adding more generic-word renames to a substitution that rewrites prose is going to keep generating Mad Libs in the docstrings; worth teachingapply_renamesto skip string literals instead of patching them one at a time.CommunityMirrorCatalogDocumentis an unreferenced public export. Renamed from the codegen'sAdagentsJson1and added to__all__, but nothing references it —CommunityMirrorGetResponse.adagents_jsonusesCommunityMirrorAdagentsJson. Either confirm it's an intentional surface or drop it from the rename map so it stays an internal numbered class. (code-reviewer.)
Minor nits (non-blocking)
- Inconsistent normalization in platform inference.
_community_mirror_platform_from_configreturns the rawconfig["platform"](e.g."Meta") in one branch but the normalized value in the single-property branch. Harmless —publish_community_mirror_adagentsre-normalizes downstream — but normalizing both branches (or neither) reads cleaner.
LGTM. Follow-ups noted below.
…#2187 parity)
Add persisted community-mirror adagents lifecycle methods to RegistryClient,
matching the JS SDK route/body shapes:
- publish_community_mirror_adagents -> PUT /api/registry/mirrors/{platform}
- get_community_mirror_adagents -> GET /api/registry/mirrors/{platform}
(returns None on HTTP 404), hydrating superseded_by from the wrapper
- list_community_mirror_adagents -> GET /api/registry/mirrors (limit/offset)
- upsert_community_mirror_adagents -> helper with platform inference from the
config or a single consistent properties[].platform value
A build_community_mirror_adagents helper emits authorized_agents: [], strips
the platform routing key, and rejects authorization claims and generator-only
flags. Platform keys are trimmed/lowercased and validated against
^[a-z0-9_-]{1,64}$.
Closes #925
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…models adcp#5385 added the community-mirror lifecycle and schemas to the registry OpenAPI spec. Vendor the updated spec from adcp main, regenerate the registry types, and retype the community-mirror client methods from dict[str, Any] to the generated Pydantic models. - publish/upsert -> CommunityMirrorPublishResponse - get -> CommunityMirrorGetResponse | None (full wrapper; superseded_by is on both the wrapper and adagents_json, so the prior hydration is dropped and the catalog-only invariant is enforced by the model) - list -> CommunityMirrorListResponse - add delete_community_mirror_adagents -> CommunityMirrorDeleteResponse, mapping the 409 not-superseded case to RegistryError - build helper stops emitting authorized_agents (publish body is catalog-only; the service forces authorized_agents: []) Add stable rename-map entries for the new generated collisions (AdagentsJson1, the five CommunityMirrorPublishRequest anyOf variants) and for generic inline names surfaced by the spec sync (Brand/Format/Collection/Data/ Success/Severity/DiscoveryMethod1/AuthorizationType/SelectionType/ PublisherProperty). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3baca82 to
9416ea3
Compare
There was a problem hiding this comment.
Approving. Additive feat(registry): minor that mirrors the JS lifecycle (adcp-client #2183/#2187) cleanly — the catalog-only invariant is enforced at the type layer, and the new methods reuse the existing _request/_request_ok/_parse plumbing rather than reinventing error handling.
code-reviewer: sound-with-caveats. ad-tech-protocol-expert: sound-with-caveats. No MUST FIX: no runtime error on the happy path, no credential path, no breaking public-API change, catalog fail-closed preserved.
Things I checked
- Catalog-only invariant is load-bearing and fail-closed.
CommunityMirrorAdagentsJson.authorized_agentsis required withmax_length=0(src/adcp/types/registry.py), so a registry that ever returns a non-emptyauthorized_agentsfailsmodel_validateandget_community_mirror_adagentsraises via_parse. A community mirror must never assert sales authorization; this is the right shape.tests/test_registry.py::test_rejects_non_catalog_mirrorcovers it. - 404/409 handling.
get_community_mirror_adagentsuses_request(..., allow_404=True)→None(registry.py:1413);delete_community_mirror_adagentslets 409 surface asRegistryError(status_code=409)through the defaultexpected_status=200, withforce=Trueas the documented escape hatch. Both tested. - Platform mismatch guard.
get_*rejects a registry response whoseplatformdisagrees with the normalized request key (registry.py:1414-1416). Fail-closed beats trusting the echo. - Enum additions are additive, not discriminator changes.
DiscoveryMethod(+adagents_authoritative,community_catalog),Status1(+community),Source5(+community) are plain string enums on ordinary fields — no discriminated-union arm removed, no forward-compat hazard. - Test coverage is real, not nominal. 28 new cases across build/publish/get/list/upsert/delete, including ambiguity inference, single-property inference, the 409 mapping, and the catalog-only rejection. No unchecked manual-verification boxes in the test plan; this is a wire-shape unit suite and doesn't need a live round-trip.
Follow-ups (non-blocking — file as issues)
- The codegen rename is a whole-file regex and it's corrupting generated prose.
scripts/generate_registry_types.py:114-118runsre.sub(rf"\b{old}\b", new, content)across the entire file, so renames hit Field/example strings, not just type references. The new"Data": "CreateAdagentsData"entry rewrites the GDPR boilerplate inPolicyto "CreateAdagentsData subjects must provide freely given... consent" — a data-subject right reassigned to a response model — and the pre-existing"Property": "ResolvedPropertyEntry"mangles the newFormatSummaryfield descriptions ("Property IDs" → "ResolvedPropertyEntry IDs"). Cosmetic today (description metadata only, CI is green, no wire/runtime impact) — but"Data"is a dangerously generic token, and the day an enum value or field name is the bare wordDatathis silently corrupts the contract. Scope the reference substitution to type contexts (after:[->class(|,=) or skip string-literal lines. The class-definition sub at L107-112 is already anchored correctly; only the second sub is the offender. build_community_mirror_adagentshard-requiresformats, but the wire contract accepts five variants.CommunityMirrorPublishRequestis ananyOfover formats / properties / placements / collections / signals (any one non-empty). The builder rejects everything butformats(registry.py), and sincepublish_*/upsert_*both route through it, a spec-valid properties-only or signals-only mirror cannot be published through the SDK.ad-tech-protocol-expertconfirms the JS builder is identically narrow, so this is deliberate cross-SDK parity rather than a divergence — but_community_mirror_platform_from_configinfers the key fromproperties[].platform, advertising a path that then can't complete withoutformats. Either relax the builder to "at least one non-empty facet" or document the limitation on the builder docstring.- Two unrelated upstream spec changes rode along in the regen.
supported_versionsflips array→string andCompanySearchResult→FindCompanyResultinschemas/registry-openapi.yaml.ad-tech-protocol-expertconfirms both net to zero diff on the public Python surface (inline path response / rename-map no-op), so not a breaking SDK change — but call them out in the PR body so the next regen reviewer doesn't read them as noise, and confirm the deployed registry actually servessupported_versionsas a scalar.
Minor nits (non-blocking)
- Stale PR description. The body says
get_community_mirror_adagents"hydratessuperseded_byfrom the wrapper" and that the builder "emitsauthorized_agents: []" — both describe the first commit's behavior. The final code does neither (the docstrings correctly say no hydration is needed and the body is catalog-only). Code and tests are consistent; just the prose is behind. - Asymmetric platform normalization.
_community_mirror_platform_from_configreturns the rawconfig["platform"]un-normalized while theproperties[]branch normalizes before dedup (registry.py). A config withplatform: "Meta "andproperties[].platform: "meta"wouldn't be detected as consistent. Downstream_normalize_*cleans the published key, so harmless — normalize the config branch too for symmetry.
LGTM. Follow-ups noted below.
Closes #925
Adds the persisted community-mirror adagents lifecycle to
RegistryClient, matching the JS SDK shapes fromadcp-client#2183/#2187.Methods added
publish_community_mirror_adagents(platform, config, *, auth_token)→PUT /api/registry/mirrors/{platform}get_community_mirror_adagents(platform)→GET /api/registry/mirrors/{platform}; returnsNoneon HTTP 404; hydratessuperseded_byfrom the wrapper when the inner catalog omits it; rejects mismatched-platform / non-catalog bodieslist_community_mirror_adagents(*, limit=None, offset=None)→GET /api/registry/mirrorsupsert_community_mirror_adagents(config, *, platform=None, auth_token)→ resolves the platform key from theplatformarg, thenconfig["platform"], then a single consistentproperties[].platformvalue (ambiguous → error), then publishesPlus a module-level
build_community_mirror_adagents(config)builder that emitsauthorized_agents: [], strips theplatformrouting key, and rejects authorization claims / generator-only flags. Platform keys are trimmed/lowercased and validated against^[a-z0-9_-]{1,64}$.Deviation from JS
The JS PRs add hand-written TS interfaces (
PublishCommunityMirrorAdagentsResponse,CommunityMirrorAdagentsCatalog, etc.) — these are registry-API DTOs, not AdCP protocol schemas, so there is no generated Pydantic model inadcp.typesto validate against. Following the existingcreate_adagentsconvention, these methods take/returndict[str, Any]. Tests therefore assert the wire shape directly (HTTP method, URL path, request body) rather than.model_validate().Tests
tests/test_registry.py— wire-shape assertions: PUT/GET method +/api/registry/mirrors/{platform}path, request body (authorized_agents: [], noplatformkey,superseded_bypassthrough),Noneon 404,superseded_byhydration, platform validation/inference/ambiguity, and auth header.Verification:
pytest tests/test_registry.py(138 passed),make lint,make typecheckall pass.🤖 Generated with Claude Code