Skip to content

Conversation

@bokelley
Copy link
Collaborator

@bokelley bokelley commented Oct 28, 2025

Summary

Fixed bug where mock adapter and create_media_buy tool were improperly handling pending/async responses, causing confusion about media buy state and returning fake IDs to clients.

Problem

The mock adapter was returning fake media_buy_id values like "pending_question_..." or "pending_step_..." for async/pending operations. This is wrong because:

  1. The media buy doesn't exist yet - it's being processed
  2. Fake IDs confuse clients - they look real but aren't queryable
  3. Violates A2A protocol design - pending operations should return minimal domain data with protocol envelope indicating status

Additionally, media_buy_create.py was trying to save non-existent media buys to the database and process packages even when the operation wasn't complete.

Root Cause Analysis

Architecture: AdCP protocol uses two layers:

  1. Domain layer (CreateMediaBuyResponse) - Business data
  2. Protocol layer (ProtocolEnvelope) - Wraps with status field

For pending/async operations:

  • Domain response should have media_buy_id=None (doesn't exist yet)
  • Domain response should have packages=[] (not created yet)
  • Protocol envelope provides status="working" or "input-required"
  • Protocol envelope provides task_id (workflow_step_id) for tracking

The client uses task_id to poll or wait for webhook, NOT a fake media_buy_id.

Solution (3 commits)

Commit 1 (1d42301): Initial defensive fix - ensure mock adapter returns packages with package_id

  • Added package population in async/question paths
  • Prevents immediate error but doesn't fix architecture issue

Commit 2 (924c408): Skip package processing for pending responses

  • Detects pending by checking media_buy_id.startswith("pending_") or None
  • Skips package merge/validation for pending
  • Proper architectural separation

Commit 3 (aaaea9c): Remove fake media_buy_id from pending responses

  • Mock adapter returns media_buy_id=None for async/question scenarios
  • Mock adapter returns packages=[] for pending
  • Tool logic handles media_buy_id=None properly
  • Database persistence skipped for pending (nothing to save yet)

How It Works Now

For completed operations:

{
  "status": "completed",
  "payload": {
    "media_buy_id": "mb_abc123",
    "packages": [{...}]
  }
}

For pending/async operations:

{
  "status": "working",
  "task_id": "workflow_step_xyz",
  "message": "Processing your media buy...",
  "payload": {
    "buyer_ref": "buyer_123",
    "media_buy_id": null,
    "packages": []
  }
}

Client uses task_id to track, not a fake media_buy_id.

Testing

  • Unit tests pass
  • Syntax validated
  • Code inspection confirms proper handling
  • Pre-commit hooks pass

Impact

  • Fixes production error on wonderstruck.sales-agent.scope3.com
  • Removes confusing fake IDs from client responses
  • Properly aligns with AdCP protocol envelope architecture
  • No breaking changes for clients (they should check status field anyway)
  • Backwards compatible with existing code

🤖 Generated with Claude Code

… paths

Problem:
The mock adapter was returning CreateMediaBuyResponse without the packages
field populated in certain code paths (async mode, question asking scenario).
This caused errors in media_buy_create.py when it tried to extract package_id
from empty response.packages list.

Root Cause:
- async mode (_create_media_buy_async): Returned response without packages
- question asking scenario: Returned response without packages
- Both paths returned CreateMediaBuyResponse(packages=[]) by default

When response.packages was empty, media_buy_create.py would iterate over
req.packages but find no matching adapter_packages, resulting in:
"Adapter did not return package_id for package 0. Cannot build response."

Solution:
- Added package population in async mode return path (lines 527-535)
- Added package population in question asking return path (lines 406-414)
- Added defensive package_id validation in normal path (lines 723-731)
- Added debug logging to track package_id through response creation

All code paths now ensure packages with package_id are included in response.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
bokelley and others added 2 commits October 28, 2025 16:18
The previous fix added packages to pending responses in the mock adapter,
but the real issue was that media_buy_create.py was trying to process
packages even for pending/async responses.

This commit detects pending responses (media_buy_id starting with 'pending_')
and skips the package merging/validation logic for those cases.

For pending responses:
- The adapter returns minimal data (possibly empty packages)
- The protocol envelope sets status='working' or 'input-required'
- The client knows the operation isn't complete yet

This is the architecturally correct fix per the AdCP protocol envelope design.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
As discussed, pending responses should NOT have a media_buy_id because
the media buy doesn't exist yet. Returning fake IDs like 'pending_xyz'
is confusing to clients.

Changes:
1. Mock adapter now returns media_buy_id=None for async/question scenarios
2. Mock adapter returns empty packages for pending responses
3. Tool logic updated to handle media_buy_id=None as pending indicator
4. Database persistence skipped for pending responses (no media buy to save yet)

For pending/async operations:
- The protocol envelope provides task_id (workflow_step_id) for tracking
- Client uses task_id to poll status or wait for webhook
- When operation completes, THEN a real media_buy_id is generated

This aligns with A2A protocol design where incomplete operations return
minimal domain data with status='working' in the protocol envelope.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@bokelley
Copy link
Collaborator Author

Closing in favor of clean PR without schema files. See new PR.

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.

3 participants