feat: add idempotency key protocol, skill updates, and test scenarios - #49
feat: add idempotency key protocol, skill updates, and test scenarios#49Jinyi-S wants to merge 1 commit into
Conversation
ADS-3000: Add shared idempotency reference document ADS-3001: Update draft, build-campaign, campaigns, ads skills and request builder agent to include Idempotency-Key header on create requests and reference the shared protocol ADS-3002: Add 9 test scenarios (22-30) covering key generation, hierarchy, retry, error handling, and auto-execute mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e89e8fc to
756baab
Compare
amurph491
left a comment
There was a problem hiding this comment.
Architecture Review
This PR has the right intent but the wrong architecture. Three significant issues:
1. The -H passthrough won't work with api-request.sh
The api wrapper only accepts positional args: <skill> <METHOD> <path> [json_body] (scripts/api-request.sh lines 108–117). It doesn't parse -H flags. So when a skill does:
api POST "ad_accounts/{ad_account_id}/ad_sets" \
-H "Idempotency-Key: <uuid>" \
'{...}'-H gets slotted into $4 (the BODY variable), and the actual JSON body becomes $5 and is silently ignored. Every create command in this PR would send -H as the request body instead of the JSON payload. This is a breaking change.
2. Header management belongs in the wrapper, not scattered across skills
The whole point of api-request.sh is to centralize header management — Authorization, X-Spotify-Ads-Sdk, X-Spotify-Ads-Skill, and Content-Type are all handled there. Skills just call api POST "path" '{body}' and don't think about headers. This PR breaks that pattern by requiring every skill to manually add a header, creating 6+ places to maintain the same logic.
The correct fix: add automatic key generation to api-request.sh for POST requests on the 6 supported create endpoints. Skills wouldn't change at all — the wrapper detects that the path matches a create endpoint, generates a UUID, and includes the header automatically. Zero skill changes needed.
3. The term "idempotency" must not be user-facing
The term appears in AGENTS.md, 5 skill files, the agent instructions, 9 test scenario titles, and the reference doc. Non-technical users will encounter this term and be confused by it. This protection should be completely invisible — on by default, with no user-facing terminology. Advanced users can opt out only if they specifically ask for it.
Recommended Approach
-
api-request.sh— detect POST to the 6 create endpoints and auto-inject the key header viauuidgen. Add a flag (e.g.--no-dedup-key) or read a setting so advanced users can disable it if they specifically ask. On by default, no user action needed. -
Skills — no changes. They keep calling
api POST "path" '{body}'exactly as they do today. -
AGENTS.md retry safety — keep the existing guidance about not auto-retrying POST/PATCH. The wrapper handles the header; retry semantics can stay documented in an internal reference doc renamed to something like
references/create-retry-safety.md(not "idempotency"). -
Test scenarios — rewrite to test that the wrapper adds the header automatically (not that each skill manually includes it), and remove technical terminology from scenario names.
Summary
Add idempotency key support to all create-oriented skills so the plugin prevents duplicate entity creation on retries.
skills/api-reference/references/idempotency.md) covering key generation, retry rules, multi-entity hierarchies, and error handlingIdempotency-Keyheader on create requests and reference the shared protocolChanges
skills/api-reference/references/idempotency.mdskills/drafts/SKILL.mdskills/build-campaign/SKILL.mdskills/campaigns/SKILL.mdskills/ads/SKILL.mdagents/spotify-ads-request-builder.mdtests/test-scenarios.mdContext
The Ads API now supports optional
Idempotency-Keyheaders on 6 POST endpoints (3 draft + 3 direct creation). The server-side implementation is complete and shadow mode is enabled. This PR updates the plugin to start sending the header.Checklist