fix: allow owned signals discovery without activation#792
Conversation
124e8b0 to
00d0003
Compare
00d0003 to
22e9e78
Compare
There was a problem hiding this comment.
LGTM. Clean split — the protocol shape now matches the spec, where activate-signal-request requires destinations with minItems: 1, so activating an owned signal onto the seller's own inventory was always a structurally nonsense call.
Things I checked
SignalsPlatform(OwnedSignalsPlatform[TMeta], Protocol, Generic[TMeta])with@runtime_checkableproduces the rightisinstancesemantics on both arms — verified bytest_owned_signals_platform_runtime_checkable_with_get_signals_onlyattests/test_decisioning_specialisms.py:117-124._KNOWN_SPECIALISM_PROTOCOLSlists bothOwnedSignalsPlatformandSignalsPlatform, so_all_specialism_methods()still unions to{get_signals, activate_signal}even thoughvars(SignalsPlatform)now only containsactivate_signal(platform_router.py:138-150).- Boot vs runtime gates stay consistent:
REQUIRED_METHODS_PER_SPECIALISM["signal-marketplace"](dispatch.py:209-215) still requires both methods;_require_platform_method("activate_signal")athandler.py:2274plus the_OPTIONAL_PLATFORM_METHODSlisting athandler.py:347gives buyers a cleanUNSUPPORTED_FEATURErejection instead of an AttributeError if they call activation against an owned-only seller. - Conventional-commits framing as
fix:is defensible — no existingsignal-ownedadopter breaks (relaxed validation passes both shapes), andtools/listdroppingactivate_signalonsignal-ownedis the right direction per spec. Borderlinefeat!:if a buyer hard-coded the advertised set, but tolerable. specialismenum slugs unchanged (schemas/cache/3.0/enums/specialism.json); no schema touch needed.
Follow-ups (non-blocking — file as issues)
get_signalsresponse schema still requiresdeployments(schemas/cache/3.0/signals/get-signals-response.json:90-100). Owned-signal adopters need to emit a self-referentialtype:"agent"deployment entry. Worth a one-line contract note in theOwnedSignalsPlatform.get_signalsdocstring atsrc/adcp/decisioning/specialisms/signals.py:69-86so first adopters don't ship a schema-invalid response.- No router-synthesis test for a
signal-owned-only child platform intests/test_platform_router.py. The handler-level advertisement test intest_decisioning_advertised_per_specialism.pycovers the wire surface; the router fan-out path is still implicitly covered via the union but not explicitly locked. - Release notes / CHANGELOG should flag the
tools/listadvertisement change forsignal-ownedsellers as a buyer-observable delta.
Minor nits (non-blocking)
- Stale docstring assertion in
_protocol_method_names.src/adcp/decisioning/platform_router.py:162-166still claims "there's no Protocol inheritance to chase, so a singlevars()pass gets every declared method." That's no longer true now thatSignalsPlatforminherits fromOwnedSignalsPlatform. The code is fine because both Protocols are independently enumerated in_KNOWN_SPECIALISM_PROTOCOLS, but the next maintainer who adds a sub-Protocol without listing it independently will be misled. - Comment cross-reference in
_OPTIONAL_PLATFORM_METHODS.src/adcp/decisioning/handler.py:345-347reads as if this set is what keepsactivate_signalrequired for marketplace. The actual gate isREQUIRED_METHODS_PER_SPECIALISM["signal-marketplace"]; cross-reference it explicitly so the defense-in-depth role is obvious. examples/hello_seller_signals.pyis now marketplace-only in spirit but still implements both methods. Docstring update is accurate; a follow-uphello_owned_signals.pywould be the cleaner pairing.
Approving.
22e9e78 to
1d3be96
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. The dual-Protocol split with SignalsPlatform(OwnedSignalsPlatform[TMeta], ...) is the right shape — preserves static narrowing via isinstance, matches the existing CreativeBuilderPlatform / CreativeAdServerPlatform precedent, and the validation/advertised-tools/manifest tables all line up.
Things I checked
- Inheritance preserves
runtime_checkablesemantics.vars(SignalsPlatform)carries onlyactivate_signalpost-split;vars(OwnedSignalsPlatform)carriesget_signals. Both classes are in_KNOWN_SPECIALISM_PROTOCOLS(src/adcp/decisioning/platform_router.py:138-150), so_all_specialism_methods()(line 184) still unions to{get_signals, activate_signal}— synthesized delegation is unaffected.isinstance(get_signals_only_impl, SignalsPlatform)returns False as asserted attests/test_decisioning_specialisms.py:119-128. - Defense-in-depth on marketplace adopters is intact.
REQUIRED_METHODS_PER_SPECIALISM["signal-marketplace"]atsrc/adcp/decisioning/dispatch.py:212-216still requires both methods at boot; the new_require_platform_method("activate_signal")gate atsrc/adcp/decisioning/handler.py:2274is runtime defense-in-depth, not a weakened gate. - Existing adopter compatibility. A
signal-ownedadopter that previously implemented both methods (forced) still validates after this change —get_signalsrequired,activate_signalsimply ignored by the gate. Pure additive relaxation. - Capabilities projection.
SPECIALISM_TO_PROTOCOLS[\"signal-owned\"]still maps to{\"signals\"}, so a discovery-only owned-signals platform emitssupported_protocols: [\"signals\"]correctly (tests/test_decisioning_capabilities_projection.py:146-157). OwnedSignalsPlatformis added toadcp.decisioning.__all__atsrc/adcp/decisioning/__init__.py:362— clean public-surface addition, no removals.
Follow-ups (non-blocking — file as issues)
- Manifest hand-edit is fragile.
schemas/cache/3.0/manifest.jsonis a cached upstream artifact;scripts/sync_schemas.py(replace_cache_from_bundle) rmtrees and rewritesschemas/cache/3.0/wholesale on every sync. The CI schema-drift check at.github/workflows/ci.yml:347-353logs but does NOT fail on diff, so this PR passes CI either way. If upstream3.0.7(src/adcp/ADCP_VERSION) still listssignal_ownedintools.activate_signal.specialisms, the next cleanpython scripts/sync_schemas.pysilently reverts the hand-edit andtest_signal_owned_manifest_exercises_discovery_only(tests/test_decisioning_specialisms.py:212-231) starts failing. Confirm upstreamadcontextprotocol/adcphas shipped the matching manifest update on the 3.0.x train, or add a post-sync patch step so the SDK contract doesn't depend on a manifest line nobody else is updating. - Conventional-commit prefix is a judgment call.
fix:is defensible (relaxing a fictional activation requirement IS the bug fix), but the diff also adds a new public exportOwnedSignalsPlatform.feat:→ 5.7.0 would be the more honest semver signal against the 5.6.0 baseline at.release-please-manifest.json. Non-blocking — maintainer call. - JS-side parity unverified. Docstring at
src/adcp/decisioning/specialisms/signals.py:25-26claims this mirrorssrc/lib/server/decisioning/specialisms/signals.tsin the JS SDK. If the JS side still ships a singleSignalsPlatforminterface, the cross-SDK conformance storyboard will see an advertised-tools mismatch for owned-signal adopters.
Minor nits (non-blocking)
get_signalsdocstring drift.src/adcp/decisioning/handler.py:2253still reads "Catalog discovery for signal-marketplace / signal-owned" — accurate, but every other comment in this diff was updated to call out the discovery-only owned variant. Cosmetic.
Approving on the strength of the inheritance shape plus the manifest test catching the regression if it happens.
…dits (#795) * feat(schemas): patches/ post-process infrastructure Hand-edits to the regenerated schema cache used to get silently overwritten by ``make regenerate-schemas`` — that's exactly how PR #753's forward-looking ``revoked_publisher_domains[]`` + ``publisher_domains[]`` compact-form patches got wiped on PR #791's bump to 3.0.12. The patches were invisible to anyone running the regen, and the loss only surfaced on line-by-line diff review of the regen output. This introduces ``schemas/patches/`` as a tracked, reviewable layer of hand-edits applied AFTER the upstream-verbatim extraction in ``scripts/sync_schemas.py``. Each ``.patch`` file is a unified diff with a comment header (Patch / Reason / Filed / Upstream status / Drop when), applied in lex order from the repo root via ``patch -p1``. The state machine in ``apply_tracked_patches`` classifies each patch as: - **Alive** — forward-applies cleanly → apply, continue. - **Dead** — reverse-applies cleanly (upstream landed it) → exit non-zero with the patch name + directive to delete the file. - **Broken** — neither direction applies → exit non-zero with the patch name + directive to either update the hunks or remove the patch. Dead and broken both fail loudly because silently no-op'ing on dead would let stale ``.patch`` files linger forever, and silently skipping broken would let the SDK ship a cache whose patched fields don't actually exist in the working tree. Patch-application runs ONCE at the end of ``main()`` (after all primary + preview bundles have been extracted), not inside ``_sync_one`` per-bundle. Per-bundle would misclassify a 3.0 patch as "dead" during the subsequent 3.1 preview pass because the cache wouldn't reset between passes. The existing ``make check-schema-drift`` target picks up patch-apply without changes — it already re-runs ``sync_schemas.py`` and diffs the cache. With patches in the directory, the diff now validates that patches still apply AND the resulting cache matches what's checked in. The directory ships empty in this commit. The two #753-restoration patches follow in a separate commit once PR #791 (3.0.12 regen) lands on main — they need the post-regen cache as the diff base. Refs #791, #753 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(schemas): restore #753 + #792 hand-edits via tracked patches Three patches restore prior hand-edits to the 3.0 schema cache that were silently dropped on the 3.0.7 → 3.0.12 regen (#791): 01-adagents-revoked-publisher-domains.patch ``adagents.json`` gains top-level ``revoked_publisher_domains[]`` with the Reason enum. SDK dict-layer helpers (``validate_revoked_publisher_domain_entry``, ``filter_revoked_selectors``) implement the contract today; this patch restores the field on the Pydantic-model layer so adopters get parity across both code paths. Filed in PR #753 — upstream took the shape into 3.1.0-beta.x rather than 3.0.x, so the patch stays alive until the SDK moves to a 3.1 floor. 02-publisher-property-selector-publisher-domains.patch ``publisher-property-selector.json`` gains optional ``publisher_domains[]`` (compact form) on the `all` and `by_tag` selectors, XOR with the singular ``publisher_domain``. SDK helpers (``_fanout_publisher_properties``, ``validate_publisher_properties_item``, ``get_properties_by_agent``) implement the contract; same Pydantic- layer parity restoration. Filed in PR #753 — same upstream-status as patch 01 (landed in 3.1.0-beta.x). 03-manifest-signal-owned-discovery-only.patch ``manifest.json`` removes ``signal_owned`` from ``activate_signal.specialisms`` and ``activate_signal`` from ``signal_owned.exercised_tools``. Owned signal agents are discovery-only by design; upstream's 3.0.12 manifest still includes the old two-specialism shape, which would require owned signal agents to implement ``activate_signal`` (defeating the specialism purpose) and make conformance runners exercise marketplace activation. Filed in PR #792 — SDK self-correction. Drops when upstream updates the manifest shape. Each patch carries its own audit-trail header (Patch / Reason / Filed / Upstream status / Drop when) so the next reader has full context without needing git archaeology. See ``schemas/patches/README.md`` for the convention. Pydantic regen (``make regenerate-schemas``) picks up all three restored fields: src/adcp/types/_generated.py — RevokedPublisherDomain, PublisherDomain (per-arm) src/adcp/types/generated_poc/adagents.py src/adcp/types/generated_poc/core/publisher_property_selector.py SCHEMA_DELTAS.md now reports "no field-shape changes detected" because the post-patch generated types match the prior committed state. PR #791 had recorded the deletions as part of its regen; this commit reverses them via the patches infra. Tests: ``pytest tests/`` — 5026 passed, 30 skipped, 1 xfailed. ruff + mypy clean. Refs #753, #791, #792 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
signal-owneddiscovery-only by requiring and advertisingget_signalswithoutactivate_signalsignal-marketplaceon the existingget_signals+activate_signalsurfaceOwnedSignalsPlatformas the public protocol for seller-owned signal catalogsRoot Cause
signal-ownedreused the marketplace/provisioned signal method and advertised-tool set, forcing seller-owned signal adopters to expose a fictional activation step even when returned first-party signals are already usable on seller inventory.Validation
uv run --extra dev python -m pytest tests/test_decisioning_specialisms.py tests/test_decisioning_advertised_per_specialism.py tests/test_decisioning_capabilities_projection.py tests/test_platform_router.py tests/test_decisioning_handler_shims.pyblack,ruff,mypy,bandit, whitespace, YAML/JSON, large file, merge conflict, case conflict, private key checks