Skip to content

feat(spec): deprecate top-level max_results on get_signals, pin pagination precedence#3120

Merged
bokelley merged 1 commit into
mainfrom
claude/issue-3113-pin-max-results-precedence
Apr 25, 2026
Merged

feat(spec): deprecate top-level max_results on get_signals, pin pagination precedence#3120
bokelley merged 1 commit into
mainfrom
claude/issue-3113-pin-max-results-precedence

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Closes #3113

Summary

get-signals-request.json was the only paginated read endpoint carrying both a legacy top-level max_results (no cap, no default) and the standard pagination envelope (pagination.max_results, max: 100, default: 50). The schema was silent on which wins when both are present — PR #3109 implemented pagination wins in code without a normative spec anchor.

This PR adds the anchor:

  • max_results: Marked "deprecated": true + description-prefix deprecation notice per the repo's established pattern (mirrors targeting.json). Includes MUST-level precedence rule and guidance for sellers receiving only the legacy field. Queues removal for AdCP 4.0.
  • pagination: Added a sibling description (annotation-only in draft-07 strict semantics; this repo's existing account field uses the same pattern) restating the precedence rule at the envelope level.
  • docs/signals/tasks/get_signals.mdx: Updated parameter table and three code examples to use pagination.max_results instead of the top-level legacy field.

Schema audit: get_products, list_creatives, list_creative_formats, get-collection-list, get-property-list, get-media-buy-artifacts, and tasks-list all carry only pagination — no other endpoint has this drift.

Non-breaking justification: Description changes and a deprecated: true annotation. No type, structure, or required-field changes. No existing wire shapes are rejected. The MUST-level precedence rule documents what PR #3109 already implements; it creates a new conformance obligation but does not break any caller that was already following the pagination-wins behavior.

Callers to note: Any implementation sending max_results > 100 at the top level will, once it adopts the new precedence rule, shift to pagination.max_results's maximum of 100. This is a behavioral cap change for those callers and is called out in the changeset.

Pre-PR review

  • code-reviewer: approved — no blockers. Two nits noted: (1) description sibling to $ref is annotation-only under draft-07 strict semantics — acknowledged, matches existing account field pattern in this file; (2) pagination description inline-repeats max: 100, default: 50 from pagination-request.json — acceptable for readability, minor divergence risk if pagination-request.json changes.
  • ad-tech-protocol-expert: approved — non-breaking per spec. "deprecated": true boolean added to match repo pattern. Precedence rule covers both conflict case and solo-legacy-field case. Changeset bump level minor is correct (new MUST-level conformance obligation).

Follow-on (not in this PR)

The storyboard get_signals_pagination_integrity (being added in PR #3109) should gain a conflict-test step that sends both max_results and pagination.max_results with differing values and asserts the response honors pagination.max_results. Blocked on #3109 merging. Filed as a nit from the product-expert review.

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


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Independent review — ready to merge.

Verified against expert recommendations

  • Non-breaking shape. Adds `deprecated: true` annotation + description prose; doesn't remove the field. Matches the existing precedent at `static/schemas/source/core/product-filters.json:131-132` so `npm run test:schemas` tolerates the annotation under draft-07.
  • Changeset type. `minor` is correct — adds a schema-visible spec change (the `deprecated` annotation + description-level MUST). Not `patch` (which would be wrong for a spec annotation), not `empty` (which is reserved for non-protocol changes).
  • Precedence pinned in two places. Schema description AND task-reference docs both say "pagination.max_results takes precedence." SDK code-gen consumes the schema description; humans read the docs.
  • Cross-endpoint audit. Confirmed in the changeset: `get_products`, `list_creatives`, `list_creative_formats`, etc. carry only `pagination`. No other endpoint has the duplicate-field shape — `get_signals` was the lone outlier.
  • 3.x-friendly. Doesn't break 3.x callers; flagged for removal in 4.0 per the deprecation prose.

Compatibility with #3109

The training agent's `handleGetSignals` (#3109) already implements the precedence this spec change pins (pagination wins). My follow-up commit preserved the legacy 50-cap on top-level `max_results` while honoring the schema's 100-cap on `pagination.max_results`; the 50-cap is conservative under the new SHOULD-level "subject to a maximum of 100" — agents may cap below the ceiling. No code change needed in the training agent.

CI is green. Ready.

@bokelley bokelley merged commit cecca44 into main Apr 25, 2026
17 checks passed
@bokelley bokelley deleted the claude/issue-3113-pin-max-results-precedence branch April 25, 2026 13:09
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.

Spec: pin precedence for pagination.max_results vs top-level max_results on get_signals

2 participants