Skip to content

fix: expose requested AdCP version resolver#855

Merged
bokelley merged 1 commit into
mainfrom
bokelley/address-pr-851-comments
May 25, 2026
Merged

fix: expose requested AdCP version resolver#855
bokelley merged 1 commit into
mainfrom
bokelley/address-pr-851-comments

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

What Changed

Adds a public server-facing helper, resolve_requested_adcp_version(), for adopters that need to apply the same request-version contract as the server dispatcher when shaping handler output.

The helper:

  • resolves explicit adcp_version first and normalizes it to release precision
  • maps legacy adcp_major_version to the base minor when supported
  • treats omitted version signals as the SDK's unnegotiated default, currently 3.0
  • rejects an unnegotiated default that is not present in the caller's supported set

It is re-exported from adcp.server alongside UnsupportedVersionError, and tests cover the public import path, explicit 3.1 resolution, legacy 3.0 fallback, and unsupported-default rejection.

Why

Issue #851's follow-up noted that the Python SDK still needed a clear, documented version-resolution contract after the 6.1.1 beta improved 3.0/3.1 response shaping. This makes that contract available to adopter handler code without creating a second parser separate from detect_wire_version().

The helper documentation is explicit that it resolves envelope fields only. It does not run the dispatcher's tool-specific legacy shape probes for adapter-routed versions such as 2.5.

Expert Review

  • Protocol reviewer found one edge case: the helper could return 3.0 even when supported=("3.1",). Fixed by rejecting unsupported defaults with UnsupportedVersionError.
  • Code reviewer independently flagged the same edge case and asked that the helper's scope be narrowed/documented relative to dispatcher legacy-shape probing. Fixed in the docstring.

Validation

  • PYTHONPATH=src pytest tests/test_validation_envelope.py tests/test_schema_validation_server.py tests/test_adcp_version_wire.py tests/test_dispatcher_version_routing.py -q
  • PYTHONPATH=src ruff check src/ tests/test_validation_envelope.py
  • PYTHONPATH=src mypy src/adcp/server/
  • Commit hook: black, ruff, mypy, bandit, and standard pre-commit hygiene checks passed

Also ran PYTHONPATH=src pytest tests/ -q before the final edge-case patch. It reported one unrelated timing-sensitive failure in tests/test_permission_denied_budget.py::test_branch_parity_absorbs_registry_variance; rerunning that single test immediately passed.

@bokelley bokelley changed the title [codex] expose requested AdCP version resolver fix: expose requested AdCP version resolver May 25, 2026
@bokelley bokelley marked this pull request as ready for review May 25, 2026 03:32
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Follow-ups noted below. Clean, contained additive surface that packages the dispatcher's envelope-field contract for adopters without forking it — right shape: delegates to detect_wire_version and only adds the unnegotiated-default branch on top.

Things I checked

  • Helper at src/adcp/validation/envelope.py:134-162 mirrors the dispatcher's envelope-resolution path (src/adcp/server/mcp_tools.py:2101-2182) step-for-step, minus the tool-specific legacy shape probes — and the docstring is explicit about that exclusion.
  • New "default not in supported" rejection at envelope.py:160-161 closes the edge case both prior reviewers flagged. tests/test_validation_envelope.py:89-93 covers it.
  • Additive change only — no removed or renamed public exports, no signature changes. feat:-shaped under conventional commits but not breaking, so ! is not required.
  • __all__ ordering in src/adcp/server/__init__.py:271,280 is consistent with the surrounding block (case-insensitive, not strict ASCII).
  • ad-tech-protocol-expert: sound-with-caveats — no wire-shape footgun for 3.0-vs-3.1 response mismatch, helper compresses dispatch step 1 + step 3 correctly.
  • code-reviewer: sound-with-caveats — semantically equivalent to the dispatcher's envelope-only contract, no layering breaches.

Follow-ups (non-blocking — file as issues)

  • Commit prefix. This adds a new public function and re-exports a new error class. Conventional commits says that is feat:, not fix:. release-please will categorize it as a patch when it should be a minor. Not blocking the merge, but worth catching at the next iteration so the changelog matches the surface.
  • wire_value=default on the unnegotiated-default rejection. envelope.py:161 raises UnsupportedVersionError(default, supported) for a buyer that claimed nothing. UnsupportedVersionError.wire_value is documented as "the original wire value" (envelope.py:52), and the dispatcher echoes it as details.claimed_version (mcp_tools.py:2133). The dispatcher does not currently hit this path — only the new public helper can — but if an adopter pipes this exception into the same telemetry, claimed_version: "3.0" will show up for a request where the buyer claimed nothing. Consider either a sentinel wire_value=None + separate default attribute, or a dedicated UnsupportedDefaultError subclass. Low-impact for now.
  • SUPPORTED_WIRE_VERSIONS includes 2.5 via LEGACY_ADAPTER_VERSIONS. The helper cannot legitimately resolve to 2.5 via envelope fields (2.5 buyers don't carry adcp_version / adcp_major_version), so the legacy entries in the default supported= are dead weight here and invite an adopter to assume this helper handles 2.5 routing. The docstring warns, but the function name does not. Either rename the helper or narrow the default supported= for this entry point.
  • Docs drift. New public export in adcp.server — confirm AGENTS.md / README.md / llms.txt / skills/*/SKILL.md reference the helper where adopters will look for the version-resolution contract. Internal-only addition is fine at minor-release cadence; worth a quick pass before the next 6.2 promotion.

Minor nits (non-blocking)

  1. Docstring overclaims parity with detect_wire_version. envelope.py:153-155 says the new helper raises "just like detect_wire_version". detect_wire_version never raises for the no-claim case; it returns None. The new helper's strict-default branch has no analog. Tighten the wording.
  2. Test module docstring is stale. tests/test_validation_envelope.py:1 still says "Tests for adcp.validation.envelope.detect_wire_version" though four new tests cover resolve_requested_adcp_version. Drop the function suffix.
  3. Coverage gaps in the new tests. No test exercises explicit adcp_version-rejection routed through the new helper (e.g. {"adcp_version": "9.9"}), and no test passes a custom default= to confirm the override path. Both are cheap to add.

LGTM. Follow-ups noted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant