Repository navigation
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive, backward-compatible narrowing support with full test coverage.
What I checked:
- Adds interval/exact/pixel-ratio narrowing to
check_narrows(narrowing.py L105-130) and an optional synchronousv1_requirements_lookupto declaration/product projection. Both are additive, keyword-only params — no schema, generated-code, or public-surface break. - The new
_RANGE_FIELDSloop was traced against the existing_MAX_FIELDS/_MIN_FIELDS/_EXACT_FIELDSloops; theadd_interval_divergencededup (narrowing.py:116-125) correctly suppresses duplicate scalar/named-cap diagnostics, and the broadnot_equalsuppression cannot hide a real divergence (a not_equal on width/duration_ms_exact implies both sides exact, collapsing to the same single conflict). - Unbounded (null) endpoints, exact-wins-over-range precedence, and the max_duration_ms→duration_ms_exact fallback behave per the 11 published AdCP 3.2.7 vectors plus added boundary tests.
v1_requirements_lookupis a caller-supplied synchronous callback whose failures deliberately propagate (documented, test-covered) — no SDK-side timeout owed.- No
adcp.*public-surface signature/type changes, no generated-code hand-edits, no import-layering violations, no CI-gate disabling.
No blocking or medium findings. No gated paths, no high-risk flag, no author-team gate (review_decision REVIEW_REQUIRED but gated_paths is false, so row 2 does not fire). Falls through to row 9.
Blocking findings: none.
Medium findings: none.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent review of a clean diff, no blocking or medium findings.
This push is a small delta over a prior clean pass. The only behavioral-adjacent change is a test-isolation rename in tests/integration/test_a2a_context_id.py (_EchoHandler → _ContextIdEchoHandler), applied consistently across all 5 occurrences (definition + 4 call sites) with no stale references left. The rename avoids a handler-registration collision with another test module that was shadowing create_media_buy under full-suite collection — a sound, test-only fix with no change to library behavior or the public adcp.* surface.
Decision-table walkthrough:
- Rows 1 (critical/high): no findings — skip.
- Row 2 (gated_paths): gated_paths is false — skip (review_decision REVIEW_REQUIRED is immaterial here).
- Rows 3–5 (high_risk): high_risk is false — skip.
- Row 6 (sticky escalate): prior decision was approve, not escalate — skip.
- Row 7 (no-auto-approve team): no team match — skip.
- Row 8 (≥3 medium): zero medium findings — skip.
- Row 9: approve.
No semver-signal, forward-compat, import-layering, generated-code, credential, CI-gate, or version-translation concerns are implicated by this test rename.
The narrowing checker missed three published conflict vectors: pixel-density sets, a duration range escaping its upper bound, and an exact width mismatch. Seller-asserted references also passed through projection without a way to provide their requirements for checking.
Cover acceptance sets, exact numeric values, range containment and min/max boundaries, including legal unbounded duration endpoints and exact-duration precedence. Run all eleven unchanged AdCP 3.2.7 narrowing vectors. Add an optional synchronous
v1_requirements_lookupto declaration/product projection so callers with a provisioned catalog can check each full format reference and receive the existingFORMAT_DECLARATION_DIVERGENTadvisory at its indexed location.Refs #1749. Projection preserves seller references. An omitted lookup or missing entry remains explicitly unchecked; the SDK does not fetch or certify a catalog. Lookup failures propagate. No schemas, version pins or generated models change.
Give the context-ID integration test fixture the unique private name
_ContextIdEchoHandler. This prevents another test module’s_EchoHandlerregistration from hidingcreate_media_buyduring full-suite collection. The test assertions and library behavior are unchanged.Validation
make lint,make typecheck-all, andmake testpassed. 18621 passed, 2729 skipped, 10 deselected, 1 xfailed, 1017 warnings in 3738.16s (1:02:18); coverage 92.14%.SDK 10 forward port: #1759.