Skip to content

feat(adcp): support ADCP_BASE_URL env override in sync_schemas.py#285

Draft
bokelley wants to merge 2 commits intomainfrom
claude/issue-284-adcp-base-url-env-override
Draft

feat(adcp): support ADCP_BASE_URL env override in sync_schemas.py#285
bokelley wants to merge 2 commits intomainfrom
claude/issue-284-adcp-base-url-env-override

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #284

Summary

Adds ADCP_BASE_URL env var support to scripts/sync_schemas.py, matching the JS SDK (@adcp/client/scripts/sync-schemas.ts:23). Cross-SDK CI can now set the var once to point both clients at a fixture CDN or pre-release bundle server, without editing source.

  • _ADCP_BASE = os.environ.get("ADCP_BASE_URL", "https://adcontextprotocol.org").rstrip("/") — default unchanged; trailing slashes stripped to prevent a doubled /protocol path segment that would silently 404
  • BUNDLE_BASE_URL = _ADCP_BASE + "/protocol" — all three fetch sites (fetch_bundle, fetch_signature_sidecars) pick up the override automatically
  • Prints ! ADCP_BASE_URL override active: <base> when the override is active, before the fetch banner, using the established ! sigil convention
  • Module docstring updated to list both ADCP_BASE_URL and ADCP_SKIP_SIGNATURE env vars together for discoverability
  • 3 new tests: default value (env absent), override applied, trailing-slash stripped; all use monkeypatch + fresh module load so the test is correct even when ADCP_BASE_URL is set in the environment when pytest starts

What was tested

  • pytest tests/test_sync_schemas.py — 18 passed
  • pytest tests/ --ignore=tests/integration --ignore=tests/conformance/signing/test_ip_pinned_transport.py — 2180 passed, 21 skipped

Pre-PR review

  • code-reviewer: approved — blocker (env-sensitive test_default_value) fixed; nits noted below
  • dx-expert: approved — no blocker remains, no new blocker introduced

Nits surfaced (not fixed — reviewer discretion)

  • No runtime guard if ADCP_BASE_URL includes the /protocol suffix (e.g. https://fixture.example.com/protocol → doubled path, silent 404). The docstring warns against it; a ValueError guard would be stronger.
  • urlopen accepts file:// URIs via ADCP_BASE_URL; acceptable for a dev tool given the SHA-256 + Sigstore chain validates integrity, but worth noting.
  • Fresh module names (sync_schemas_fresh, sync_schemas_fresh2) use a numeric suffix; request.node.name-derived names would be cleaner but not necessary.

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_01KXfYPHYKr3R6EDqUbbo9ty


Generated by Claude Code

claude added 2 commits April 26, 2026 00:09
Matches the JS SDK pattern so cross-SDK CI can set ADCP_BASE_URL once
to point both clients at a fixture CDN or pre-release bundle server.
Trailing slashes on the env var value are stripped to prevent a doubled
/protocol path segment. Adds a print when the override is active.

Closes #284

https://claude.ai/code/session_01KXfYPHYKr3R6EDqUbbo9ty
… ! sigil

test_default_value was relying on the already-loaded _mod which bakes in
whatever ADCP_BASE_URL was when pytest started — exactly the CI case this
feature enables. Switched to the same fresh-module-load pattern the other
two env-override tests already use, with monkeypatch.delenv to guarantee
the default code path. Also align the override print line with the
established ! sigil convention.

https://claude.ai/code/session_01KXfYPHYKr3R6EDqUbbo9ty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sync_schemas.py: support ADCP_BASE_URL env override for parity with JS SDK

2 participants