Conversation
There was a problem hiding this comment.
Approving. Clean inline-to-named extraction — every new schema is byte-equivalent to the shape it replaced, registered in index.json, and the parent $refs through cleanly. Right shape for SDK generators.
Things I checked
- All seven extracted shapes match the inline originals field-for-field.
forecast-dimension-signal.jsonpreserves theanyOfrequiringsignal_refORsignal_idand theallOfgatingsignal_valuebypresence.forecast-dimension-geo.jsonpreserves the fourif/thenblocks pergeo_level. committed-metric.jsonanddelivery-metric-aggregate.jsoncarrydiscriminator: { propertyName: "scope" }+oneOf, matching the existing accepted patterns incore/assets/asset-union.jsonandcore/activation-key.json. Thevendorbranch ondelivery-metric-aggregate.jsonkeepsqualifier(closed) andmeasurable_impressions(minimum: 0).forecast-point-dimensions.jsonouteroneOfis now$ref-only across the six dimension schemas; each target hasproperties.kind = { const: ... }+kindinrequired, so the audit walker's const-property-discriminator path should classify it asdiscriminated. CI is the gate —scripts/oneof-discriminators.baseline.jsonhas no new entries, so any newnarrowable/dangerousfinding fails rather than silently ratcheting.- Enum opening on
comply_test_controller.scenario,list_scenarios.scenarios[], andcompliance_testing.scenarios[]is the schema catching up to existing spec language — bothcomply-test-controller-request.json:528andcomply-test-controller-response.json:51already mandated "Runners and sellers MUST accept unknown scenario strings."minoris the correct bump. - Server-side:
server/src/training-agent/comply-test-controller.tsderives its tool-input enum from a localSCENARIO_ENUM, not the spec schema's closed enum. No code path depends on the removed enum. - All new schemas registered in
static/schemas/source/index.json. Changeset present at.changeset/schema-ergonomics-sdk-codegen.mddeclaringminor. - Test 4B (
x-adcp-open-payloadboolean-only) and Test 11B (custom compliance scenarios) verify what they claim — 11B explicitly asserts the absence ofscenario.enumon the three opened surfaces before round-tripping a custom value.
Follow-ups (non-blocking — file as issues)
docs/protocol/get_adcp_capabilities.mdx:525still inlines the scenario list as the "current values." Informational only after this PR, but awhats-new-in-3-1.mdxnote would help SDK consumers downgrade their codegen target from a typed enum tostring.x-adcp-open-payload: falseonget-products-response.filter_diagnostics: the newschema-extensions.mdxtable says omission already covers "unclassified." Reservefalsefor genuinely extension-tolerant fields with known structure, otherwise it's documentary noise.- Test 11B could also assert
scenario.type === \"string\"to harden against a future shape change that drops the enum AND the type.
Minor nits (non-blocking)
account-with-authorization.jsonadditionalProperties: true. Draft-07 default is permissive, so the explicittrueis a no-op. The forecast-dimension extractions useadditionalProperties: false; the explicittruehere is inconsistent house style but not a defect. Either drop it or keep — purely cosmetic.
Safe to merge once CI greenlights the oneOf audit.
There was a problem hiding this comment.
LGTM. Pure SDK-codegen refactor — extracts 13 inline shapes into named core schemas without changing wire output, and the one real behavior change (compliance scenario enum → open string) brings the schema back into agreement with the spec contract that was already MUST-open.
Things I checked
- Extraction fidelity. Walked the 7 modified parents (
package.json,get-media-buy-delivery-response.json,signal-targeting-rules.json,vendor-metric-optimization.json,canonical-projection-ref.json,forecast-point-dimensions.json,list-accounts-response.json) against the 13 newcore/*.jsonfiles. Samerequired,additionalProperties, nestedoneOf/allOf/if-then, descriptions. Discriminators preserved:committed-metric.json:7anddelivery-metric-aggregate.json:7carry top-leveldiscriminator: { propertyName: "scope" }matching the inline shapes they replaced. - Index registration. Every new schema registered in
static/schemas/source/index.json(13 new entries — 29, 41, 85, 213, 217, 221, 225, 229, 233, 265, 465, 509 — plus the existing ones still resolve). - Open-payload annotation count.
x-adcp-open-payload: trueat exactly 4 sites:core/protocol-envelope.json:57,core/vendor-metric-value.json:33,governance/check-governance-request.json:36,compliance/comply-test-controller-response.json:506. Wire-irrelevant, draft-07 §6 unknown-keyword semantics — same precedent asx-adcp-hoist. - Schema/docs coherence on compliance scenarios.
docs/protocol/get_adcp_capabilities.mdx:525now mirrors the open-string contract instatic/schemas/source/protocol/get-adcp-capabilities-response.json:1507-1530. Pre-PR docs already said "Runners MUST accept unknown scenario strings" — the closed enum was the spec/schema mismatch, not the broadening.minorchangeset is the right classification. oneOfaudit walker.scripts/audit-oneof.mjsdoes one-hop ref resolution;oneOf: [{ $ref: ... }]where each target carriesconst/enumon the discriminator property resolves todiscriminated, identical to the inline form. No baseline change expected, and the PR confirmsnpm run test:oneof-discriminatorspasses.- Test coverage.
tests/schema-validation.test.cjs:217-240,300-310recursively walks the schema graph asserting everyx-adcp-open-payloadis boolean; the compliance-open-string test compiles the three schemas async and validatesseller_custom_fixture_resetround-trips through request, list-scenarios response, and capabilities.
Follow-ups (non-blocking — file as issues)
static/schemas/source/media-buy/get-media-buy-delivery-response.json:357-432—by_package[].missing_metrics.itemsis still inline and uses the samescope: standard | vendordiscriminator. Can't directly reusecommitted-metric.json(differentrequiredset, nocommitted_at), but worth extracting tocore/missing-metric.jsonso the trio committed/aggregate/missing has the same SDK-stable shape.static/schemas/source/media-buy/get-media-buy-delivery-response.json:454-558—by_catalog_item,by_creative,by_keyword,by_geoitems areallOf [delivery-metrics + { type: object, ... }]wrappers that fit the same extraction pattern.- Changelog note worth adding: SDKs that previously generated a TS string-literal union from the compliance scenario enum will lose exhaustiveness on regeneration. Existing strings still match; the call-out helps integrators understand why their type narrowing changed.
docs/spec-guidelines.md:299-326— the nullable scalar section is correct for plain scalars but doesn't cover nullable enums, where draft-07 requires both\"enum\": [..., null]and\"type\": [\"string\", \"null\"]. Common authoring trap; one example would prevent it.
Minor nits (non-blocking)
- Positional
oneOf[0]access in the open-string test.tests/schema-validation.test.cjs:687readscomplyResponseSchema.oneOf?.[0]?.properties?.scenarios?.itemsand assumes branch index 0 isListScenariosSuccess. Stable today, fragile under future reordering —find(b => b.title === 'ListScenariosSuccess')is cheap insurance. whats-new-in-3-1.mdx:242paragraph density. Three new #5168 bullets land in one run-on**bold**lead-in chain. Splitting the SDK-codegen items into their own sub-paragraph would help the skim path.
Approving on the strength of the extraction fidelity sweep plus the spec-vs-schema reconciliation on the compliance contract.
|
Issue #5180 proposes extracting the remaining delivery-metric inline shapes ( Generated by Claude Code |
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Extract-to-$ref refactor with byte-identical bodies on the wire; the only behavior change — opening comply_test_controller.scenario and compliance_testing.scenarios[] from closed enum to bare string — is the schema catching up to the 3.0-documented "Runners MUST accept unknown scenario strings" contract, which is correctly a minor on the 3.1 pre-mode line.
Things I checked
- Wire-shape preservation across all eight extractions. Properties, required arrays, defaults,
allOf/if-thenconditionals,additionalProperties, and the pre-existingdiscriminatorkeyword are preserved verbatim onaccount-with-authorization.json,canonical-projection-slot-override.json,committed-metric.json,delivery-metric-aggregate.json, the sixforecast-dimension-*.json,signal-selection-group-rule.json, andvendor-metric-optimization-supported-metric.json. Inline copies inpackage.json,get-media-buy-delivery-response.json,forecast-point-dimensions.json,signal-targeting-rules.json,canonical-projection-ref.json,vendor-metric-optimization.json, andlist-accounts-response.jsonare replaced 1:1 with$refpointers. - Discriminator preservation.
committed-metric.jsonanddelivery-metric-aggregate.jsonkeepdiscriminator: { propertyName: "scope" }+oneOfwithscope: constbranches. The five rate-styleif/thenrequireds ondelivery-metric-aggregate.json:99-148(viewable_rate,completion_rate,cost_per_acquisition,roas) are duplicated verbatim from the inline original.forecast-point-dimensions.jsonreduces to aoneOfof six$refentries; each extracted file still carrieskind: const: <value>so the discriminator is preserved at the property level. additionalPropertiessemantics.signal-selection-group-rule.jsonandcanonical-projection-slot-override.jsonpreserve theiradditionalProperties: true; the newx-adcp-open-payload: trueannotations land only on objects already declaringadditionalProperties: true(protocol-envelope.json:payload,vendor-metric-value.json:breakdown,check-governance-request.json:payload, thecomply-test-controller-response.jsondecoded-JSON arm).- Schema-vs-docs coherence.
docs/protocol/get_adcp_capabilities.mdxdescribes the open-string contract with SHOULD-include-canonical / MAY-include-custom.docs/reference/schema-extensions.mdxadds thex-adcp-open-payloadsection with explicittrue/false/omitted semantics.docs/reference/whats-new-in-3-1.mdxcalls out both ergonomics and open-scenario widening.docs/spec-guidelines.mdadds the nullable-scalars section and explicitly rejects OpenAPI-stylenullable: truein draft-07 source. - Changeset.
.changeset/schema-ergonomics-sdk-codegen.mdisminor. Right shape — extractions and thex-adcp-open-payloadmarker are non-breaking; the enum-to-string widening accepts every prior value plus more. Beta line absorbs this as3.1.0-beta.N. - New tests. Test 4B asserts
x-adcp-open-payloadvalues are boolean; Test 11B assertsscenarioisstring(notenum) on the request, list-scenarios response items, and capabilities, and round-trips a customseller_custom_fixture_resetscenario through all three schemas.5473/5473schema tests pass. protocol-envelope.json:payloadgainsx-adcp-open-payload: true— correctly marks the decoded payload arm as intentionally open without changing itsadditionalProperties: trueshape.
Follow-ups (non-blocking — file as issues)
x-adcp-open-payload: falseis documented but unused.docs/reference/schema-extensions.mdxdefines thefalsesemantics, but no schema in this PR sets it. Either land one canonical use site (a structured object that's extension-tolerant but not free-form) or markfalseas reserved-for-future-use so generators don't write dead handling for a value they'll never see.- Test 4B only enforces type, not allowed values.
x-adcp-open-payloadis documented astrue/false/ omitted. A boolean-only assertion is correct today but won't catch a future tightening that forbidsfalse. Cheap to upgrade to an allow-list when the spec narrows. - Open-scenario widening deserves a release-note callout, not just a Misc bullet. It's the only field-level type widening in 3.1; buyers/sellers running literal-union codegen need the heads-up. The whats-new entry at
docs/reference/whats-new-in-3-1.mdx:248covers the migration, but consider promoting it.
Minor nits (non-blocking)
- Brittle branch lookup in the compliance-scenario test.
tests/schema-validation.test.cjs:687locates the list-scenarios branch viacomplyResponseSchema.oneOf?.find(branch => branch.title === 'ListScenariosSuccess'). Optional chaining makes a futuretitlerename or restructure intoallOfsilently downgrade the assertion to a no-op —listScenariosResponse?.properties?.scenarios?.items?.enumis undefined either way. Add an explicitif (!listScenariosResponse) return 'ListScenariosSuccess branch missing — selector out of date';so a rename trips the test instead of passing it. - Extracted-file descriptions are thinner than parent prose. New
forecast-dimension-*.jsonfiles have one-line root descriptions; the parentforecast-point-dimensions.jsoncarries the dispatch-on-kindcontext. SDK doc-generators that read rootdescriptionwill get the slim string. Cosmetic — but a one-line pointer to the parent ("Variant of ForecastPoint dimensions; see forecast-point-dimensions.json for dispatch rules.") would help adopters reading the bundled type docs cold.
Approving on the strength of the byte-identical extractions plus the new tests that pin both the open-payload boolean contract and the open-string scenario contract.
|
Issue #5181 proposes clarifying Generated by Claude Code |
There was a problem hiding this comment.
LGTM. Clean ergonomics refactor: extract inline shapes into named $ref schemas without changing wire validation, plus one additive widening (open-string compliance scenarios) and one new documentary annotation (x-adcp-open-payload). The architectural principle holds — keep the wire stable, give SDK generators stable type names to land on.
Things I checked
- Refactor fidelity. Twelve extracted schemas are byte-equivalent to the inline shapes they replaced —
required,additionalProperties,if/thengates,oneOfbranches, descriptions, andx-entityannotations all preserved (code-reviewerverified). Spot-checkedcore/delivery-metric-aggregate.json:6-9(discriminator preserved at the new root) andcore/forecast-point-dimensions.json:13-37(collapses 250+ lines of inlineoneOfinto six$reflines, same six discriminator branches in the same order). - Discriminator integrity.
core/committed-metric.jsonandcore/delivery-metric-aggregate.jsonboth carrydiscriminator: {propertyName: "scope"}at the new top-level withconst: "standard"/const: "vendor"on each branch andadditionalProperties: false.scripts/oneof-discriminators.baseline.jsontracks only undiscriminated entries — no baseline update required. - Index completeness. All twelve new files registered in
static/schemas/source/index.jsonunder the right namespaces. - Open-payload annotation pairing. All four sites (
core/protocol-envelope.json:57,core/vendor-metric-value.json:33,governance/check-governance-request.json:36,compliance/comply-test-controller-response.jsonrecorded-callpayload) paired withadditionalProperties: trueand the mixed-arm case explicitly carved out in the description prose. - Open-scenario semver.
minoris the right call — the wire contract was already open perdocs/protocol/get_adcp_capabilities.mdx("Runners MUST accept unknown scenario strings"). Typed-SDK consumers will notice the union widen tostringon regeneration;docs/reference/whats-new-in-3-1.mdxcalls that out with a migration note. Schema is being brought into agreement with documented runner behavior. - Test coverage.
tests/schema-validation.test.cjsTest 4B recurses arrays-by-index and object-entries to catch any non-booleanx-adcp-open-payload; Test 11B asserts no.enumat the three open-scenario sites and round-trips a custom scenario string through Ajv withdiscriminator: true. A future enum re-introduction fails the latter. - Schema-vs-docs coherence.
docs/protocol/get_adcp_capabilities.mdx:525prose now matches the open schema.docs/spec-guidelines.mdnullable-scalar convention ({"type": ["string", "null"]}) is consistent with howcore/forecast-dimension-signal.json:34already encodessignal_value.
Follow-ups (non-blocking — already tracked)
x-adcp-open-payload: falseships defined-but-unused. The doc concedes the line betweenfalseand omission is fuzzy whenadditionalProperties: true. Notable; usage/test-policy already tracked in #5181.- Remaining delivery-metric extraction tracked in #5180.
- Python SDK regeneration tracked in adcontextprotocol/adcp-client-python#902.
Approving on the strength of byte-equivalent extraction plus a tightened schema-authoring lint that locks the open-scenario contract in.
|
Confirmed: not folding the x-adcp-open-payload false policy decision into this PR. It is tracked as #5181; this PR keeps the documented annotation plus boolean authoring lint and leaves the usage/test-policy decision for follow-up. |
Closes #5168.
Summary
x-adcp-open-payloadfor intentionally open JSON payload surfaces, including explicittrue,false, and omitted semantics.comply_test_controllerrequest/response andget_adcp_capabilities.compliance_testing.scenariosso the schema matches the open-for-extension compliance contract.x-adcp-open-payloadannotations.Expert Review
Validation
npm run build:schemasnpm run test:schema-utf8npm run test:schemasnpm run test:extension-schemasnpm run test:docs-navnpm run test:oneof-discriminatorsnpm run test:composednpm run lint:schema-linksgit diff --checknpm run test:unit,npm run test:test-dynamic-imports,npm run test:callapi-state-change,npm run typecheckCoverage Note
x-adcp-open-payloadvalues. Downstream SDK/OpenAPI generator consumption remains owned by the generator repos.x-adcp-open-payload: falseusage/test-policy follow-up tracked in Clarify x-adcp-open-payload false semantics and tests #5181.