feat(media-buy): reject get_products products without pricing_options

[EmmaLouise2018]

Summary

pricing_options is a required, minItems: 1 field in AdCP 3.1 — a product that advertises no pricing model is non-transactable. The client now drops such products from completed get_products responses before callers and completion handlers see them.

• New enforceProductPricingOptions() wired in at the top of applyProductPropertyPolicy() — the universal completion chokepoint — so it covers all five paths (sync executeAndHandle, polling waitForCompletion, track, webhook, and the second executeTask path) with one integration point, and fires even when no property policy is configured.
• Runs independent of the response validation mode, so unpriced products are removed even under responses: 'warn' | 'off'.
• Mutates the result in place (matching the existing property-policy filter path) to preserve the non-enumerable match accessor.
• Outcome recorded in result.metadata.productPricingPolicy and a product_missing_pricing_options debug-log notice; the webhook path forwards the same summary on WebhookMetadata.

Config: default-on; opt out with validation.rejectProductsWithoutPricingOptions: false.

Why default-on

pricing_options is spec-required (minItems: 1), so a compliant seller never trips this. Returning an unpriced product to a buyer is a correctness bug. Bumped minor, consistent with how the repo treats spec-conformance tightening (cf. fix(compliance): emit stripped field notices). Opt-out is provided for callers deliberately inspecting malformed seller responses.

Changes

• src/lib/core/SingleAgentClient.ts — enforceProductPricingOptions(), config option, helpers, wiring.
• src/lib/core/ConversationTypes.ts / src/lib/core/AsyncHandler.ts — productPricingPolicy on TaskResultMetadata and WebhookMetadata.
• test/lib/product-pricing-options-rejection.test.js — new: default drop, empty-array = missing, opt-out, all-priced no-op, webhook path.
• Updated 3 fixture files whose products were spec-invalid (missing pricing_options).

Testing

• npm run typecheck — clean
• npm run lint — 0 errors
• Full fast suite — 12034 pass, 0 fail
• Changeset added (minor)

[aao-ipr-bot]

Approving. Buyer-side enforcement of a spec-required field, wired at the one completion chokepoint so all five paths inherit it — right shape, and the disclosure (metadata + typed debug notice + opt-out) keeps it a witness, not a translator.

Premise checks out. ad-tech-protocol-expert confirmed pricing_options is in the Product schema's top-level required array and carries minItems: 1, identically in AdCP 3.0.0 and 3.1.0 (adcontextprotocol/adcp core/product.json on main and the v3.0.0 tag; corroborated by core.generated.ts:3748). So treating [] as equivalent to missing is spec-correct, and the cross-version worry is a non-issue: a 3.0 seller is held to the same constraint, and the v2.5 legacy layer types the field as required too. No supported wire version emits a spec-valid unpriced product this filter would wrongly drop.

Things I checked

• Chokepoint claim is true. enforceProductPricingOptions is the first statement in applyProductPropertyPolicy, and every completion path routes through it: sync executeAndHandle (SingleAgentClient.ts:1696), polling waitForCompletion (1956), track→applyProductPropertyPolicyToTaskInfo (1941→1979), webhook (2031), second executeTask (3101). Fires even with no property policy configured.
• match accessor survives the in-place mutation. attachMatch defines match non-enumerably on the TaskResult object, not on result.data; reassigning result.data to a spread clone while keeping result identity matches the existing property-policy filter path. Comment's claim is accurate.
• Cast isn't trusted. as unknown as GetProductsResponse is immediately re-read via (response as { products?: unknown }).products and guarded with Array.isArray(...) + length checks. products: undefined, empty array, all-unpriced, and all-priced are each handled and tested.
• Webhook metadata forwarding is correct. productPricingPolicy is spread onto WebhookMetadata right beside productPropertyPolicy in applyProductPropertyPolicyToWebhookResult; the webhook test asserts handlerCalls[0].metadata.productPricingPolicy.rejected_count.
• Changeset present (reject-products-without-pricing-options.md, minor); fixture updates add pricing_options to four previously spec-invalid product fixtures — necessary to keep them transactable, not masking. code-reviewer: no Blocker/High/Medium.

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

• Reconcile the default with filterInvalidProducts. This is the second product-dropping toggle in the validation struct, default-on, two lines below one (filterInvalidProducts, SingleAgentClient.ts:464-476) that defaults off. Both ad-tech-protocol-expert and dx-expert flagged the asymmetry: an adopter who reads the config learns the SDK's existing product-dropping behavior is opt-in, then gets a second one that isn't. Default-on is defensible here (the field is spec-required, the product is non-transactable) — but pick one discipline for buyer-side spec-conformance dropping and a one-line cross-reference in the filterInvalidProducts JSDoc kills the surprise.
• Changeset under-discloses the upgrade impact. The prose explains the mechanism thoroughly but never says the load-bearing sentence: adopters talking to non-strict/noncompliant sellers may receive fewer products after this minor bump. Compliant sellers are unaffected; the slice that isn't gets a silent count drop with only a type: 'warning' debug log and a metadata field as breadcrumbs — passively discoverable, found only if you already suspect products went missing. Add the upgrade note + pointer to result.metadata.productPricingPolicy. minor is borderline-but-acceptable with that sentence.
• Track path filters but doesn't disclose. applyProductPropertyPolicyToTaskInfo (SingleAgentClient.ts:1999-2000) drops the unpriced products via policyResult.data, but TaskInfo has no metadata/debug slot, so productPricingPolicy and the notice are discarded on this one path. Pre-existing and consistent — productPropertyPolicy is dropped the same way — so the "all five paths" claim is true for filtering, not for disclosure. Worth one line in the PR body so it isn't read as full coverage.

Minor nits (non-blocking)

1. Mode-independence inside a validation block reads slightly off. rejectProductsWithoutPricingOptions lives under validation but the JSDoc has to clarify it ignores validation.responses. Shared with its neighbors, so leave it — but if validation keeps accreting mode-independent product filters, a future productFilters sub-struct would read better.

LGTM. Follow-ups noted.

[bokelley]

Reviewed the updated branch after clearing CI blockers and merging current main. Local validation: npm run format:check, npm audit --audit-level=high, npm run build:lib, and NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/product-pricing-options-rejection.test.js test/lib/agent-client-in-process.test.js test/lib/product-property-policy-client.test.js test/lib/v2-getproducts-autowire.test.js all pass. The remaining npm audit item is the existing moderate js-yaml advisory below the CI high-severity gate.

[aao-ipr-bot]

Mechanically clean, well-tested, and the single-chokepoint wiring is the right call — but it ships a new default-on behavior I want the author on record about before it merges. The principle in tension is witness, not translator: the SDK now silently removes wire data by default, on a path the caller can't see unless they read result.metadata.productPricingPolicy.

Things I checked

• All five completion paths covered. enforceProductPricingOptions is the first statement in applyProductPropertyPolicy (src/lib/core/SingleAgentClient.ts:1747), the shared chokepoint for sync (:1696), second executeTask/track (:3101), polling (:1956), and webhook (:2031). One integration point, correct.
• Null/cast safety. productHasPricingOptions / productIdForPricingDiagnostics both guard !product || typeof product !== 'object' before access and narrow product_id with typeof id === 'string'. No deref risk.
• Mutation ordering is safe. Pricing enforcement reassigns result.data and metadata first; the property-policy block then re-reads result.data into a fresh response const (:1728) and mutates result.metadata.productPropertyPolicy as a key-add (:1851), so productPricingPolicy survives when both policies are active. Webhook forwarder carries both keys independently.
• result.data = {...response, products: kept} as T mirrors the existing property-policy filter at :1870-1875 — same spread, same cast, sound on the get_products-guarded path.
• Changeset present, minor. Defensible — additive config + spec-conformance tightening, consistent with fix(compliance): emit stripped field notices. No removed/renamed export, no param flip.
• Tests cover default drop, empty-array-as-missing, opt-out, all-priced no-op, and the webhook path. They assert observable behavior, not internals. Three fixtures correctly backfilled with pricing_options. (Could not run the suite locally — sandbox blocks it; PR body reports 12034 pass.)

Open question — what flips this to approve

Two experts ran in parallel. code-reviewer: "ready to push." ad-tech-protocol-expert: "sound-with-caveats, leaning unsound on the default-on shape." The divergence is the review.

1. minItems: 1 is unverified, and the empty-array branch may over-reject. The SDK's own generated schema is pricing_options: z.array(PricingOptionSchema) (src/lib/types/schemas.generated.ts:9331) — required, but no .min(1), and the codegen emits zero array .min() anywhere, so it's silent on whether [] is valid. The PR body and changeset both assert minItems: 1. If upstream product.json only marks pricing_options required (not minItems: 1), then dropping pricing_options: [] removes a schema-valid product. Confirm against adcontextprotocol/adcp product.json and cite it — spec → mock → SDK, per CLAUDE.md. (Dropping [] is semantically defensible — zero pricing models is non-transactable either way — but the changeset shouldn't claim a constraint we can't point to.)

2. Default-on, validation-mode-independent silent removal is a new shape. The closest precedent, filterInvalidProducts, is @default false and mode-gated (SingleAgentClient.ts:474). productPropertyPolicy is not precedent here — it filters on buyer authorization, is buyer-configured, and defaults to inert. This PR is the first default-on removal that fires even under responses: 'off'. A witness surfaces seller non-conformance as a notice and lets the caller decide; it doesn't delete the row by default. Two viable shapes that would flip me to approve: (a) flip the default to false, keeping the detection + productPricingPolicy metadata + debug notice as the default behavior; or (b) fold this into filterInvalidProducts as one more validity check, so there aren't two parallel product-removal mechanisms with opposite defaults. If you keep it default-on, make the spec-required case (minItems: 1 confirmed) the explicit, documented justification.

Follow-ups (non-blocking)

• productPricingPolicy.ok is dead (ConversationTypes.ts) — hardcoded true, never false. Drop it or document the shape-parity intent.
• track path doesn't surface the summary. applyProductPropertyPolicyToTaskInfo returns {...taskInfo, result: policyResult.data} and discards policyResult.metadata/debug_logs (pre-existing TaskInfo limitation, shared with productPropertyPolicy). Products are dropped but the PR body's "recorded in result.metadata.productPricingPolicy" doesn't hold on track specifically. Tighten the claim or the path.
• PR is CONFLICTING/DIRTY against current main (lockfile shows 9.0.0; main is 9.5.0). Rebase before merge — the undici/lockfile delta is base-staleness, not an intended change.

Resolve #1 (cite the spec) and answer #2 (the default shape) and I'll approve.

[aao-ipr-bot]

Clean, contained change wired at the one right chokepoint. enforceProductPricingOptions() lands at the top of applyProductPropertyPolicy() (src/lib/core/SingleAgentClient.ts), so it covers all five completion paths — sync, polling, track, webhook, second executeTask — with a single integration point and fires even when no property policy is configured. Fail-closed on a non-transactable product beats fail-open. Spec premise verified: pricing_options is required, minItems: 1 in AdCP 3.1 (tools.generated.ts:1453, schemas.generated.ts:9331), so a compliant seller never trips this.

Things I checked

• Path coverage. Wired before the policyConfig === false early-return, so enforcement runs independent of property-policy config and of the response validation mode. Polling/track route through applyProductPropertyPolicyToTaskInfo/waitForCompletion, webhook through applyProductPropertyPolicyToWebhookResult — all call the chokepoint. Confirmed.
• match accessor survival. The result.data = {...response, products: kept} reassignment is safe — match is attached non-enumerably to result, not result.data (match.ts:101-108), and match() reads this.data at call time so it reflects the filtered list. Identical to the existing property-policy filter path (SingleAgentClient.ts:1909-1913).
• Ordering. Pricing enforcement reassigns result.data first; the property policy then re-reads const response = result.data (:1767) and filters the already-pricing-filtered survivors. Property policy sets metadata.productPropertyPolicy by direct assignment, so it doesn't clobber the productPricingPolicy key. No interaction bug.
• Empty-array vs undefined. productHasPricingOptions requires Array.isArray(options) && options.length > 0 — both [] and undefined count as missing. Matches the minItems: 1 spec. Test covers it.
• Webhook forwarding. productPricingPolicy is forwarded onto WebhookMetadata (:2199-2200, type in AsyncHandler.ts). Test asserts the webhook handler sees the filtered list and the summary.
• Changeset present (.changeset/reject-products-without-pricing-options.md), descriptively named, opt-out documented. Three fixtures correctly backfilled with spec-required pricing_options.

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

• minor vs major is a real tension — confirm with the maintainer. ad-tech-protocol-expert: this is the first SDK path that silently drops items from a public method's response by default with no buyer request driving it. A caller inspecting a non-compliant seller receives fewer products than before with no code change. The PR cites spec-conformance-tightening precedent for minor and the dropped data is spec-invalid with an opt-out, which is why I'm not blocking — but the bump is the one judgment call here. If there's any doubt, major is the conservative read.
• Default-on diverges from the sibling. filterInvalidProducts defaults to false (opt-in, :495); this defaults to true. The PR body justifies default-on deliberately, so it's a choice, not an oversight — but two adjacent product-filtering knobs with opposite defaults is a DX trap worth reconciling. Consider whether a flag-and-keep middle mode (matching productPropertyPolicy's three modes) would preserve the witness posture better than a binary drop.
• Wire-version / legacy-seller interaction. Enforcement keys only on taskType === 'get_products' with no version gate. normalizeResponseToV3 runs first on the legacy/poll paths, but confirm a pre-3.1 pin or legacy seller whose products legitimately predate required pricing_options doesn't get every product nuked by default. Cross-check docs/development/WIRE-VERSION-COMPAT.md.
• taskInfo path drops the summary. applyProductPropertyPolicyToTaskInfo (:2038-2039) propagates only policyResult.data back onto taskInfo.result; productPricingPolicy is discarded because TaskInfo has no metadata channel. Filtering still happens — this mirrors how productPropertyPolicy's summary already behaves on that path — but the PR-body claim that the outcome is recorded "on every completion path" isn't literally true there. Tighten the prose or add a filtering assertion for that path.

Minor nits (non-blocking)

1. The "mutate in place" comment doesn't mutate in place. SingleAgentClient.ts — the comment says "Mutate in place ... so the non-enumerable match accessor ... survives," but the code spread-replaces result.data with a fresh object. The accessor survives because it lives on result, not because of in-place mutation. Effect is correct and matches the existing filter path; the rationale in the comment is the part that's off.
2. type: 'warning' vs "debug-log notice." The emitted log entry is type: 'warning' while the docs/changeset call it a "debug-log notice." Consistent with the input_schema_field_stripped precedent, so leave it — just a prose/label mismatch.

LGTM. Follow-ups noted below — the minor/major call is the one I'd want a maintainer eye on before this ships.