chore(schemas): resync 3.1.0-beta.3 cache + fix broken manifest patch (refs #807)#821
Merged
Merged
Conversation
6 tasks
There was a problem hiding this comment.
LGTM. Right scope — patch-hunk renumber + cache resync, with type regen deliberately split out so the post_generate_fixes.py / aliases.py brittleness gets its own review.
Things I checked
- Patch hunk math. Hunk 1 shifts
-19,8 → -20,8(+1). Hunk 2 shifts-1389,7 → -1394,7(+5 = +1 carried from hunk 1's removal + 4 lines upstream inserted between the two anchors). Verified against the committed post-patched manifest:"get_signals"(the hunk-2 anchor's first context line, post-patch) lands at line 1393 — matches1394 - 1as expected after hunk 1 deletes earlier in the file. The patch will forward-apply against current upstream and the math is internally consistent. - Patch is still semantically required. Committed cache shows
activate_signal.specialisms = ["signal_marketplace"](manifest line 22) andsignal_owned.exercised_tools = ["get_adcp_capabilities", "get_signals"](line 1394).signal_marketplace.exercised_toolsstill legitimately includesactivate_signal. Forward-applied state matches the patched shape, which confirms upstream still ships the un-patched shape and the SDK self-correction is still load-bearing. - CI drift gate.
ADCP_VERSION=3.1.0-beta.3matches the(alpha|beta|rc)regex at.github/workflows/ci.yml:275→is_prerelease=true→ sync-and-drift step short-circuits. The intentional skew between the refreshed cache and the un-regeneratedsrc/adcp/types/_generated.pywill not trip CI. Documented in the commit message. - No Python source touched.
git show e0f419d9 --name-onlyis entirely underschemas/. No type-layering breach, no public-API surface change, no migration concerns, no credential path. - Schema diffs are upstream-republishing-shaped. Spot-checked
brand/rights-terms.jsonandbundled/media-buy/create-media-buy-response.json: dominated by—→ literal—, added$idfields, and$refrewrites from relative (../enums/...) to absolute root-relative (/schemas/3.1.0-beta.3/enums/...). Norequired/enum/oneOf/typechurn surfaced. Sampled — not exhaustive across 663 files. - Expert verdicts.
ad-tech-protocol-expert: sound-with-caveats — patch is semantically correct against the 3.1 specialism model;signal_ownedas a discovery-only role is the right call.code-reviewer: ship — hunk math verified, no blockers.
Follow-ups (non-blocking — file as issues)
\$refbase-URI change vs. older caches. Upstream rewrote relative\$refto absolute root-relative/schemas/3.1.0-beta.3/...in the 3.1.0-beta.3 republish.schemas/cache/2.5/andschemas/cache/3.0/still use the relative form. Confirmadcp.registry(and anything else resolving across version-pinned caches) handles both shapes — if a shared resolver path canonicalizes one but not the other, that is a latent break for adopters pinned to 2.5/3.0.- Upstream the manifest fix. The patch carries hardcoded line numbers; this is the second time it has broken on a resync. Filing the
signal_owned-as-discovery-only correction againstadcontextprotocol/adcpwould let this SDK-local patch retire on its drop-condition rather than being re-shimmed each beta. - Tracking issues for the deferred regen brittleness. The PR body names two pre-existing issues to file: (1)
post_generate_fixes.pystrips theProtocolEnvelopeimport and never re-adds it when itsmedia_headerblock appends classes that reference it; (2)aliases.pydefensivegetattr(_g, "AcquireRightsResponse1", ...)lookups against names that are not in the regenerated_generated.py. Worth landing those issues with the same level of "drop when" specificity the patch header uses, so the regen PR has a clear acceptance gate. - One-time canonical-JSON equality check. A diff that sorts keys and member-sets across all 663 cache files would inexpensively confirm the resync is genuinely shape-preserving and surface anything semantic that the unicode-and-formatting noise might mask. Cheap insurance for the next resync.
Minor nits (non-blocking)
- Commit message line-number prose. "activate_signal block now starts at line 20, signal_owned at 1394" — line 20 is the
response_schemacontext line, not theactivate_signalblock opener (line 16 in the post-patched manifest). Same forsignal_ownedat 1394 (block opens at 1389). The hunk numbers are correct; the prose just names hunk anchors rather than block openers. Notable mostly because anyone reading the message to grep for the block will land in the wrong place.
LGTM. Follow-ups noted above.
e0f419d to
73779da
Compare
Refreshes the schema cache against the current upstream 3.1.0-beta.3 tarball. Most of the 663-file diff is unicode normalization upstream republished (escaped `—` → literal `—`), with the structurally relevant change being the manifest.json restructure that left the 03-manifest-signal-owned-discovery-only patch unable to apply. Updates the patch hunks to match the new manifest.json line numbers (activate_signal block now starts at line 20, signal_owned at 1394). The patch is still semantically required — upstream beta 3 still lists signal_owned in activate_signal.specialisms. Type regeneration deliberately left for a follow-up: regen against the refreshed cache surfaces pre-existing brittleness in the generated_poc → aliases.py → _generated.py pipeline (missing ProtocolEnvelope imports added by post_generate_fixes.py; stale defensive aliases like `AcquireRightsResponse1`). Those need their own scoped PR rather than being bundled into a schema sync. Refs: #807, #802
73779da to
6a704b2
Compare
There was a problem hiding this comment.
Clean. Hunk numbers match the post-patched layout; the self-correction is still semantically required.
Things I checked
- Hunk 1 shift 19→20 and hunk 2 shift 1389→1394: context lines internally consistent with
schemas/cache/3.1.0-beta.3/manifest.json:20-26and:1393-1398. The 5-line growth between the two hunks accounts for the cumulative shift. - The self-correction is still load-bearing.
signal_ownedis discovery-only —activate_signal's request key (signal_agent_segment_id, an opaque activation handle returned byget_signals) is a marketplace-flow primitive that doesn't map to the owned-signal lifecycle (schemas/cache/3.1.0-beta.3/signals/activate-signal-request.json:22-25). - Self-correction set is complete.
signal_marketplace.exercised_toolslistsactivate_signal/sync_accounts/sync_governance/sync_plans; the reciprocal entries (manifest.json:493,:564,:578) listsignal_marketplaceonly —signal_ownedcorrectly absent. - Trailing-newline removal on
manifest.jsonis intentional..pre-commit-config.yamlexcludes^schemas/fromend-of-file-fixer;scripts/sync_schemas.py:243-262usesshutil.copytreeto mirror the upstream tarball byte-for-byte;check-jsonis happy either way. - Conventional commit
chore(schemas):is correct — no public-API change, no semver bump expected. - Lifecycle gate at
scripts/sync_schemas.py:265-348will mark the patch dead automatically the moment upstream lands the fix; the patch header's "Drop when" is enforceable.
Follow-ups (non-blocking — file as issues)
- Hunk 1's new trailing context line is
"build_creative": {— the next tool's opening brace.},on its own would have been a more stable anchor; if upstream ever renames or reorders the tool afteractivate_signal, the patch breaks. Tighten next time you touch the file. (schemas/patches/03-manifest-signal-owned-discovery-only.patch:25)
Minor nits (non-blocking)
- PR body and commit message describe a 663-file resync; the actual diff is 4 lines. Notable. The 4 lines that landed are the right 4 lines, but a future bisector reading the commit message hunting the bulk unicode normalization will not find it here. Worth tightening the body before merge.
LGTM. Follow-ups noted below.
There was a problem hiding this comment.
Clean patch-fix. The old hunk was both off-by-line and structurally malformed, and the math now lines up.
Things I checked
- Patch math. Old
schemas/patches/03-manifest-signal-owned-discovery-only.patch:16was@@ -19,8 +19,7 @@with a body of 3 leading context + 2 removed + 2 trailing context — BEFORE count = 7, header claimed 8. Malformed at landing in #818. The new hunk adds"build_creative": {as a third trailing context line, so BEFORE = 3+2+3 = 8 ✓ and AFTER = 3+1+3 = 7 ✓. Second hunk:-1394,7 +1393,6⇒ 3+1+3 = 7 ✓ and 3+0+3 = 6 ✓. - Line numbers vs committed cache.
schemas/cache/3.1.0-beta.3/manifest.json:20="response_schema": "signals/activate-signal-response.json"; line 1393 ="get_signals". Both match the AFTER coordinates in the new hunk headers (+20,7,+1393,6). - Semantic necessity still holds. Patch comment block at
schemas/patches/03-manifest-signal-owned-discovery-only.patch:1-13still describes the SDK self-correction (signal_owned is discovery-only). Per PR body, upstream 3.1.0-beta.3 manifest still shipssignal_ownedinactivate_signal.specialismsandactivate_signalinsignal_owned.exercised_tools. - Public API surface. Nothing under
src/adcp/touched. Type regen explicitly deferred —_generated.pyandaliases.pyuntouched, so no discriminated-union or forward-compat regression risk in this commit. - CI gate.
.github/workflows/ci.ymlsync-and-drift gate skips prereleases — confirmed, no change in CI behavior. - Test plan. All three boxes ticked in the PR body.
Follow-ups (non-blocking — file as issues)
- The original patch in #818 was malformed at landing (first-hunk body count = 7, header said 8). Worth a
git apply --checkgate inscripts/sync_schemas.py(or whatever produces the patch files) so a malformed hunk can't be committed silently a second time. - Commit message subject and first paragraph read as if 663 files are in this commit. They landed in #818. The PR body recovers this lineage further down, but the lead is misleading — a reader running
git show 6a704b28 --statand seeing2 files changed, 4 insertions(+), 3 deletions(-)will be puzzled. Tighten next time. schemas/cache/3.1.0-beta.3/manifest.jsonloses its trailing newline in this PR. Effect is benign (byte-exact upstream parity, no internal line shift), but it's not called out in the PR body — interesting choice to bundle into a patch-fix commit. Ifsync_schemas.pyis what produces this, fine; if a hand-edit, separate it next time.
Minor nits — none.
LGTM. Follow-ups noted below.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the broken
03-manifest-signal-owned-discovery-only.patchsoscripts/sync_schemas.pyruns clean against the schema cache.The schema cache resync was already done by #799 (
feat: pick up beta 3 wholesale products and signals). What remained was the patch file, whose hunks still pointed at the pre-restructure manifest line numbers.What's in the PR
schemas/patches/03-manifest-signal-owned-discovery-only.patch: hunk line numbers updated from@@ -19,8 +19,7 @@→@@ -20,8 +20,7 @@(activate_signal block shifted +1 line) and@@ -1389,7 +1388,6 @@→@@ -1394,7 +1393,6 @@(signal_owned block shifted +5 lines). Also adds a context line to make the second hunk unambiguous.schemas/cache/3.1.0-beta.3/manifest.json: trailing newline restored (otherwise inert).The patch is still semantically required — upstream
manifest.jsonstill listssignal_ownedinactivate_signal.specialismsandactivate_signalinsignal_owned.exercised_tools. Both are the SDK self-corrections the patch was added to suppress.Verification
python scripts/sync_schemas.py --no-skillsruns clean (patch applies)pytest tests/test_schemas_version_pin.py tests/test_code_generation.py tests/test_seller_agent_products_schema.py tests/test_idempotency.py— all passCloses the patch-fix portion of #807. The codegen pipeline cleanup remains in #822.
🤖 Generated with Claude Code