feat(testing): extend shape-drift hint to sync_creatives + preview_creative#851
Merged
Conversation
…iew_creative Closes #849. Both tools share the same drift pattern as build_creative: a handler returns a single inner shape at the top level instead of wrapping it in the tool's required array/discriminator envelope. The bare schema error ("must have required property X") doesn't tell the developer they've inverted the response shape — this hint does. - sync_creatives: top-level creative_id / platform_id / action without a creatives array (or errors / task_id for the other two valid branches) → hint names syncCreativesResponse() from @adcp/client/server. - preview_creative: top-level preview_url / preview_html without the previews[].renders[] nesting and response_type discriminator → hint names previewCreativeResponse(). interactive_url alone doesn't trigger (legal top-level sibling on the single-variant branch). Scoped per-tool so cross-tool field names can't bleed — tag_url can't trip preview_creative, preview_url can't trip build_creative. Follow-up from PR #848's DX review (gap #7). Pattern framework from #845 already caught the high-leverage case; this expansion covers the two other creative tools where the same drift will recur. Tests: 17 passing (6 pre-existing + 11 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tection, tighter tests Two expert reviews (code-reviewer + dx-expert) on PR #851. DX-expert's top finding (value-unlocker): - The hints are INVISIBLE in the default CLI output. bin/adcp.js at three render sites (1601, 2644, 2880) printed v.error but ignored v.warning. storyboard-tracks.ts::mapStepToTestStep fed only v.error into the JSON validationDetails. A developer running `adcp storyboard run my-agent` would hit the drift, see the AJV error, and never see the fix-recipe. Fixed all four sites —⚠️ prefix on CLI, `[⚠ ...]` suffix on JSON details. Code-reviewer findings: - Wrong-wrapper drift for sync_creatives: handlers sometimes emit `{ results: [...] }` (copy-paste from preview_creative batch or a generic envelope) instead of `{ creatives: [...] }`. Detector now recognizes this when the results[] items carry per-item sync shapes. - Hint wording: aligned build_creative's "platform-native shape" with the other two's "X at the top level" phrasing for family consistency. - Clarifying comments: additionalProperties: true escape case on sync_creatives; creative_id-at-top-level legitimacy on the preview_creative variant branch. DX-expert also flagged a defensive-test gap — preview_creative with ONLY interactive_url (no wrapper) must not fire the hint. Added that test. list_creatives coverage filed as #852 (plus peer list-shaped tools). Framework-shape refactor (registry map) deferred per DX — fine at 3 branches, revisit at 5. Tests: 21 passing (20 pre-existing + 1 new defensive). Typecheck and prettier clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Expert reviews (code-reviewer + dx-expert) addressed in a4db162. DX-expert's top finding — the value-unlocker:
Fixed all four sites. Terminal now prints ` Code-reviewer should-fixes landed:
DX-expert's defensive-test gap:
Filed as follow-ups (not in this PR):
Tests: 21 passing (20 pre-existing + 1 new). Typecheck + prettier clean. |
3 tasks
bokelley
added a commit
that referenced
this pull request
Apr 23, 2026
…ay) (#853) * feat(testing): extend shape-drift hint to list-shaped tools (bare-array-at-root) Closes #852. Handlers for list tools sometimes return the bare inner array at the root instead of wrapping it in the required { <key>: [...] } envelope. AJV's error ("expected object, got array") doesn't name the required wrapper — this hint does. Covers list_creatives, list_creative_formats, list_accounts, get_products. The detector now accepts `unknown` rather than Record<string, unknown> so bare-array responses can be recognized at the root. Object-path branches guard with a plain-object check at the top. LIST_WRAPPER_TOOLS is a small data map keyed by task name — extend when new list tools land. Bare arrays for unknown task names pass through silently. Some APIs legitimately return top-level arrays; we only fire on registered list tools to avoid false positives. Tests: 29 passing (21 pre-existing + 8 new). Defensive coverage includes null / primitive payloads, empty arrays, and the wrapper-present negative case. Follow-up from #851's DX review (gap #3). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address expert review — wording, gap coverage, signature cleanup Both expert reviewers (code-reviewer + dx-expert) on PR #853. DX-expert findings: - Wording inconsistency: "bare array at the root" → "bare array at the top level" so all four tool hints scan as one family grammatically. - Gap coverage: add get_media_buys and get_signals to LIST_WRAPPER_TOOLS (both have existing response helpers). Governance list tools (list_property_lists / list_collection_lists / list_content_standards) need response helpers in @adcp/client/server first — tracked as #854 so this detector can point at real SDK symbols. - JSDoc: update function list and add @param note on the `unknown` signature + the `typeof payload === 'object'` guard. - Deferred: registry refactor unifying the object-branch ifs with the LIST_WRAPPER_TOOLS table — DX called it worst-of-both-worlds at 8 total branches. Noted in the updated JSDoc; revisit at the next addition. Code-reviewer finding: - computeStrictVerdict(taskName, payload: Record<string, unknown>) was technically lying — we cast arrays to Record. validateResponse() underneath already accepts unknown. Widened the param to unknown and dropped the cast at the call site. Cosmetic but removes documentation debt. Tests: 31 passing (29 pre-existing + 2 new for the additional tools). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Closes #849. Extends the `detectShapeDriftHint` framework from PR #848 to cover two additional creative tools — `sync_creatives` and `preview_creative` — where the same "single inner shape bubbled up to the top level" pattern will occur.
What's detected
Why
DX-expert flagged this as gap #7 on PR #848's review: "Shape drift only catches `build_creative`. `sync_creatives` with platform-native fields, `preview_creative` returning raw asset URLs — same failure mode, no hint. Fine to ship narrow, but file a follow-up issue; the pattern will recur."
The framework from #845 already handles the heuristic wiring, AJV vs. Zod paths, and warning composition. This PR just adds two switch branches + eleven tests — each branch scoped so cross-tool field names don't bleed across detector logic.
Special case
`preview_creative` carries `interactive_url` as a legitimate top-level optional field on the single-variant branch. The detector ignores it unless one of `preview_url` or `preview_html` is also present at the top level — otherwise a valid response that happens to carry `interactive_url` above the `previews` array would trip the hint.
Test plan
Still deferred
🤖 Generated with Claude Code