feat(client)!: align safety and asset APIs with JavaScript v13 - #1032
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| f"Webhook signature verification failed for agent {self.agent_config.id}" | ||
| ) | ||
| raise ADCPWebhookSignatureError("Invalid webhook signature") | ||
| elif not self.allow_unauthenticated_webhooks: |
There was a problem hiding this comment.
MUST FIX: This flips the MCP webhook default from fail-open to fail-closed. An adopter on the released 7.0.2 line with an MCP webhook receiver and no webhook_secret handled unsigned callbacks fine; after this change the same code raises ADCPWebhookSignatureError. That is a breaking behavior change, and it ships under a fix: commit — release-please cuts a patch (7.0.3), so the break lands on every auto-upgrade without a major.
Two problems compound here: (1) per repo policy a breaking diff under a non-breaking conventional-commit prefix is a high finding, and (2) the WG constitution treats webhook signature/auth posture as never patch-eligible. Carry fix!: / feat!: or a BREAKING CHANGE: footer so this cuts a major. The additive asset exports and RegistryError fields in this same commit are feat:-class (minor) and are also under-signalled by fix:.
There was a problem hiding this comment.
Ladon verdict: Request changes
Request changes — 1 blocking finding.
Blocking findings
- src/adcp/client.py:5090 — Breaking webhook default-flip ships under
fix:prefix (patch). Per the repo's mandatory semver-signal rule, a breaking change to public/wire behavior (flipping the MCP webhook default to fail-closed) plus additive public exports must carryfeat!:/fix!:or aBREAKING CHANGE:footer. release-please cuts a patch/minor from afix:prefix, so the break would ship without a major bump.
Otherwise the diff is clean and defensive: bounded Retry-After and error bodies, preserved timeouts via _request, the A2A path genuinely untouched, and additive registry-backed exports with structured/bounded RegistryError recovery metadata. No forward-compat, import-layering, generated-code, or credential issues found. Re-label the commit with the appropriate breaking prefix/footer (migration note is present in MIGRATION_v6_to_v7.md) to clear this.
Decision table: Row 1 fires — a high severity finding is present.
Blocking findings
- src/adcp/client.py:5090 — Breaking webhook default-flip ships under fix: prefix (patch); requires feat!:/fix!: or BREAKING CHANGE: footer per mandatory semver-signal rule
BREAKING CHANGE: ADCPClient.handle_webhook now rejects unsigned MCP callbacks unless the isolated-receiver compatibility escape is explicitly enabled.
bokelley
changed the title
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
The prior high finding — a breaking webhook fail-closed default-flip shipping under a non-breaking fix: prefix — has been resolved. The PR now carries feat(client)!, which forces a major version bump across the release, correctly signaling the break per the repo's semver-on-public-surface gate.
This is a subsequent pass. The remaining delta is hardening plus additive public surface: authenticated raw-body processing on the legacy HMAC path, activity-metadata redaction, a strict/scoped allow_unauthenticated_webhooks opt-in, a real 20-member AssetInstance discriminated union mirroring the generated AssetVariant, and bounded registry retry-hint parsing. The reviewer confirms no forward-compat regression on discriminated unions and no import-layering violation. All changes are well-tested.
No critical, high, or medium findings in this run.
Decision-table walk:
- Rows 1: no critical/high findings — skip.
- Row 2:
gated_pathsis false — skip (the CHANGES_REQUESTED review_decision does not gate here since no path gate applies). - Row 3: no
(deleted)high-risk reasons (all(modified)) — skip. - Rows 4–5: no medium findings — skip.
- Row 6: prior decision was
request-changes(notescalate) — sticky rule does not apply. - Row 7: no no-auto-approve team match — skip.
- Row 8: fewer than three medium findings — skip.
- Row 9: approve.
Net improvement over the prior run: the blocking break is now correctly labeled and no new blocking or medium issues surfaced.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass with no blocking findings.
The reviewer surfaced no critical, high, or medium findings. The delta since the prior approve tightens MCP webhook routing to derive task_type/operation_id from the authenticated payload body rather than untrusted URL arguments, moves legacy-vs-canonical surface selection onto the authenticated body, reduces WEBHOOK_RECEIVED activity metadata to task_id/status/protocol, and rewrites _bounded_retry_seconds to clamp oversized retry hints and avoid OverflowError on unrepresentable int bodies. Both logic changes are fail-closed and pinned by new tests.
High-risk flag is true, but all four matching files (src/adcp/__init__.py, src/adcp/types/__init__.py, src/adcp/types/_eager.py, src/adcp/types/aliases.py) are (modified) with no medium-or-higher findings, so row 5 does not fire and the modifications are presumed safe. No gated paths, no no-auto-approve team match, no deletions.
Decision table: rows 1–8 do not fire (no critical/high, gated_paths false, no deletions, no medium findings, prior decision was approve, no team gate, fewer than three mediums) → row 9 approve.
Release-As: 8.0.0-beta.1
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — release-automation and docs delta only, no blocking findings.
What I checked:
- No no-auto-approve team match;
gated_paths: false— no hard gates apply. high_risk: true, but all four high-risk reasons are(modified)type-system files with NO findings on them. Row 5 requires a medium finding to escalate — none exist. No(deleted)reasons (row 3 N/A).- Delta since the prior
approveis purely release-please config (versioning=prerelease,prerelease-type=beta,prerelease=trueto enter the SDK 8 beta line), README version-support prose/example updates to 8.0.0b1 against AdCP 3.1.15, and the renamedtest_release_configuration.pyasserting the three new config keys. - Chain is internally consistent: SemVer-to-PEP440 normalizer maps 8.0.0-beta1 → 8.0.0b1; manifest/pyproject stay at 7.0.2 so the worktree-version test still holds.
- No public-API, wire-shape, discriminated-union, import-layering, or code-path changes in this delta. No credential/ctx_metadata concerns. CI gates remain armed.
No critical/high/medium findings. Rows 1–8 do not fire → row 9 approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — no blocking or medium findings.
Changes are confined to the v3 reference-seller example (list_creative_formats_legacy gains a video_30s catalog entry and filters the static catalog to req.format_ids with trailing-slash-normalized (agent_url, id) keys; empty/None format_ids correctly falls through to the full catalog) and storyboard CI plumbing (the shell runner gates --test-kit behind a capability probe plus a kit-file existence check, fail-closed). A new test covers the trailing-slash filter path.
No public adcp.* surface, wire-shape, forward-compat, generated-code, or credential paths are touched. The high_risk flag fired only on (modified) entries under src/adcp/types/** and __init__.py, but the reviewer surfaced no medium-or-higher concerns on those files, so the modifications are presumed contract-preserving. No no-auto-approve team match, gated_paths is false, and prior decision was also approve.
Decision table: rows 1–8 do not fire (no critical/high/medium findings, no deleted high-risk paths, no gate, no team match, fewer than three mediums) → row 9 approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean CI fix on the reference-seller example with no blocking or medium findings.
What I checked:
- Delta since the prior approve is a single focused fix:
list_creative_formats_legacynow builds its static catalog viaLegacyFormat.model_construct(format_id=LegacyFormatId(...))instead ofmodel_validateon a dict. This preserves the byte-exact origin-onlyagent_urlthatAnyUrlcoercion would otherwise slash-normalize. The reviewer confirmsmodel_constructis correct here —LegacyFormatIdstill runs its own field validators and the remainingFormatfields are optional withNonedefaults. - Added a regression test asserting the wire output, so the byte-exact behavior is now guarded.
high_riskis true (modifiedsrc/adcp/__init__.py,types/__init__.py,_eager.py,aliases.py), but the reviewer surfaced no medium-or-higher concerns on these modified files, so the flag is heuristic only. No(deleted)reasons.gated_paths: false; no no-auto-approve team match.
Decision path: No critical/high/medium findings (row 1, 4, 5 do not fire). Row 2 does not apply (gated_paths false). Row 3 no deletions. Prior decision was approve, so row 6 is moot. No team gate (row 7). Fewer than three medium findings (row 8). Falls through to row 9 → approve.
| skip_counts = summary.get('skipped_by_reason') or {} | ||
| allowed_partial_skips = { | ||
| 'missing_test_controller', | ||
| 'missing_tool', |
There was a problem hiding this comment.
Medium: missing_tool is accepted unconditionally, but nothing in the gate ties it to the absent controller. The comment reads it as "missing controller tools," yet SDK 13 emits missing_tool for any tool the storyboard needs and can't find on the upstream — including a real AdCP task tool the translator should support. Because the accept only requires controller_detected is False, steps_failed == 0, and missing_test_controller > 0, an upstream that silently drops a genuine capability tool produces a missing_tool skip, keeps steps_passed > 0 from the rest, and grades partial → CI goes green on a real coverage regression this conformance gate exists to catch. missing_test_controller and prerequisite_failed are genuinely controller-cascade; missing_tool is not scoped to it. Consider dropping missing_tool from the allowed set, or asserting its skips are controller-tool-scoped.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent review, no findings.
This is a follow-up to a prior approve. The delta since then is a single, tightly-scoped change to the Assert storyboard passed CI gate in .github/workflows/ci.yml: it now accepts overall_status == 'partial' only under a fail-closed topology (zero failed steps/tracks, positive executed coverage, explicit controller absence, no skip class outside the allowlist). The loosening is justified by SDK 13 grading a controllerless translator as partial, and the residual coverage-collapse window remains covered by the separate anti-façade upstream-traffic gate. This is not disarming a CI gate — it stays armed and fail-closed on missing/malformed keys.
The high-risk flag is true only due to (modified) type-system files (src/adcp/__init__.py, types/__init__.py, _eager.py, aliases.py), but the reviewer surfaced no medium-or-higher concerns on any of them, so the modifications are presumed safe. No no-auto-approve team match, gated_paths is false, no deletions.
Decision-table walk: no critical/high (rows 1 skip), gated_paths false (row 2 skip), no deletions (row 3 skip), no medium findings (rows 4/5/8 skip), prior decision was approve not escalate (row 6 skip), no team gate (row 7 skip). Falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — CI gate scope refinement with one non-blocking Medium.
What changed since prior approve: A single ci.yml delta. The storyboard hard gate now accepts a tightly-scoped partial topology for the controllerless translator instead of requiring overall_status == passing. The reviewer confirms the accept remains fail-closed on real failures (steps_failed/tracks_failed==0, controller_detected False, no unexpected skip class, steps_passed>0) with a documented rationale, so the gate is not disarmed.
Medium findings (non-blocking):
- .github/workflows/ci.yml:706 —
missing_toolskip accepted unconditionally in the storyboard partial gate rather than scoped to the absent controller, leaving a plausible path for a real upstream capability gap to grade partial and pass.
Decision path: No critical/high findings (row 1 no). gated_paths false (row 2 no). No (deleted) high-risk reasons (row 3 no). The single Medium is a CI/infra-adjacent gate-scope concern, but it is a warn-only/scope-gap category, not a data-loss/schema/infra migration finding that row 4 targets — no destructive or schema/infra change is involved (row 4 no). High-risk files are (modified) but there is a Medium — however row 5 requires the medium to attach to a modified high-risk file; this Medium is on ci.yml, which is not a high-risk-matched path, so row 5 does not fire. Prior decision was approve, not escalate (row 6 no). No no-auto-approve team match (row 7 no). Only one Medium, well under three (row 8 no). Falls through to row 9 → approve.
The high-risk flag reflects (modified) type-system files, but the reviewer surfaced no medium-or-higher concern on any of them, so those modifications are presumed safe.
Medium findings
- .github/workflows/ci.yml:706 — missing_tool skip accepted unconditionally in storyboard partial gate
1 participant
Summary
ADCPClient.handle_webhook()HMAC path fail closed, process and route from authenticated body fields, redact webhook activity metadata, and require a literal/scoped compatibility opt-inAssetInstancediscriminated union,AssetInstanceType, and every variant through stable root,adcp.types, andadcp.types.creativenames (preserving Python's established*Contentnames where they differ from JavaScript)lookup_brand(..., fresh=True)plus bounded registry HTTP error metadata and clamped header/body retry guidance (retryAfterMs,retryAfter, andretry_after)WebhookReceiverguidance from the deprecated HMAC helperBreaking change
ADCPClient.handle_webhook()no longer accepts unsigned MCP callbacks by default. This PR is deliberately marked breaking so release automation cannot publish the change in a 7.x patch.Release automation is configured to open the SDK 8 prerelease line at
8.0.0-beta.1(8.0.0b1in Python package metadata). The bundled protocol remains AdCP 3.1.15 stable; AdCP 3.2 beta will be picked up separately.Public/conformant receivers should use
WebhookReceiverfor RFC 9421 verification and delivery deduplication. A 3.x registration that explicitly selects legacyHMAC-SHA256can continue usingADCPClientwithwebhook_secret. Receivers isolated from untrusted networks can temporarily opt in withallow_unauthenticated_webhooks=True; a multi-agent client must scope that exception by agent ID.See
MIGRATION_v7_to_v8.md.Compatibility notes
RegistryError.status_codebehavior remains intact.ImageContent,VideoContent, etc. names remain canonical; the conflicting pre-4.0*Assetaliases are not reintroduced.Expert review
Code, protocol, and security experts completed two review passes. Their findings are resolved: strict boolean/scoped webhook escape handling, authenticated-body processing and routing, telemetry redaction, overflow-safe registry retry hints, complete union semantics, stable Python asset naming, discoverable migration guidance, and correct breaking-release classification.
Validation
6404 passed, 41 skipped, 9 deselected, 1 xfailedmainat the 7.0.2 release commit; all other CI jobs passed before the review commitPart of #1031.