Bug: manual-approval update_media_buy is accepted but never applied, and returns a non-spec envelope

[estofko-scope3]

Child of #1531. Verified at HEAD a55db82 (pin adcp==5.7.0 / AdCP 3.1.0-beta.3).

Impact

With manual_approval_required=true, every update_media_buy (budget, dates, pause/resume, creative assignments, cancellation) is accepted, reported successful, and silently discarded. manual_approval_operations defaults to ["create_media_buy", "update_media_buy", "add_creative_assets"] (src/adapters/base.py:231), so update-gating is on by default for any tenant that enables manual approval — not an opt-in edge case.

What actually happens (traced)

Request side (works):

• The manual-approval branch (src/core/tools/media_buy_update.py:376-410) creates a workflow step (requires_approval), stores the request (request_obj=req), creates an ObjectWorkflowMapping(action="update"), and returns UpdateMediaBuySuccess(affected_packages=[]).

Approval side (broken):

• The branch never sets media_buy.status="pending_approval" (the update path only ever writes pending_creatives).
• Both admin approval handlers match the mapping by object_type=="media_buy" only, ignore mapping.action, and gate execution on status=="pending_approval" (src/admin/blueprints/workflows.py:199, src/admin/blueprints/operations.py:368). For an already-live buy that condition is never true.
• The only executor available is execute_approved_media_buy — the create path. No code reads the stored request back to replay an update.
• The step is set status="approved" and committed before the check (workflows.py:165-173); the else-branch flashes "Workflow step approved successfully" and no-ops (workflows.py:248-252). The operator is told it worked, the step records "approved", nothing is applied.

Buyer side (broken):

• UpdateMediaBuySuccess carries no status and no task_id/workflow_step_id (src/core/schemas/_base.py:435, :450-453), so A2A reports completed, not submitted, and the buyer has nothing to poll get_task with.
• Approval uses workflow_repo.update_status (src/core/database/repositories/workflow.py:278), a pure DB write with no webhook — no completion notification is delivered.

Record corrections

• Not blocked by Architecture Proposal: Decouple Adapter Execution from Request Handling (Low prioity) #1069. Architecture Proposal: Decouple Adapter Execution from Request Handling (Low prioity) #1069 (Post-2.0/Low) targets the auto-create path and states the manual-approval defer-to-approval pattern is already correct. platform_line_item_id not persisted #1037 (closed) fixed the same class on the create manual-approval path without Architecture Proposal: Decouple Adapter Execution from Request Handling (Low prioity) #1069.
• Secondary "pause leaves workflow step in in_progress" bug is fixed (src/core/tools/media_buy_update.py:551; regression test tests/unit/test_update_media_buy_behavioral.py:546).
• Original proposed fix (populate affected_packages on the success shape) is superseded by the pinned spec's dedicated submitted arm.

Contract (spec-grounded, full end-to-end)

Spec: tests/fixtures/adcp_schemas_pinned/media-buy/update-media-buy-response.json → UpdateMediaBuySubmitted (required: [status, task_id]; description: "operator re-approval for mid-flight changes"). Obligations: docs/test-obligations/UC-003-update-media-buy.md ALT-MANUAL-APPROVAL-REQUIRED-01..05 (BR-RULE-017/020).

1. On a gated update, persist the proposed state and return UpdateMediaBuySubmitted with status="submitted", task_id (= workflow step id), implementation_date: null. No empty synchronous success.
2. get_task(task_id) returns the pending state; the buyer can poll.
3. On operator approval, the stored update is applied via the adapter (the same dispatch seam as the auto path) — obligation -03, "adapter is called at approval time, not request time".
4. On completion, the buyer is notified (webhook carrying the spec-canonical envelope) and the workflow step is finalized completed.
5. On rejection, the buyer is notified; no changes applied.
6. Applies to all mutation types, including campaign-level pause/resume ("pause may require manual approval → submitted").

Implementation constraints (landmines)

• Branch on mapping.action. Do NOT route updates through execute_approved_media_buy — it runs the adapter create path + approve_order + forces status="active" (src/core/tools/media_buy_create.py:918/476/500) → duplicate ad-server order and clobbered paused.
• Do not reuse create's status="pending_approval" as the update pending marker — the create approval handler keys off it and would fire the create executor. Use a distinct pending representation for updates.
• Replay must bypass the approval gate. _update_media_buy_impl has no bypass parameter (src/core/tools/media_buy_update.py:13); re-invoking it re-hits the gate → infinite defer. Factor the post-gate apply logic into a separately-callable path.
• Concurrency decision required. Today multiple pending updates accumulate as separate steps/mappings (the gate does not dedupe/replace); revision (optimistic concurrency) is unimplemented. Decide overwrite-vs-queue and stale-state handling; coordinate with idempotency (feat: Add idempotency_key support on update_media_buy and sync_creatives #1075 / idempotency_key required on update_media_buy: schema requires it, impl leaves it optional — how to proceed? #1470).

Tests (existing coverage is request-only)

tests/unit/test_update_media_buy_behavioral.py::TestUC003ManualApproval -03/-04/-05 pass at HEAD but assert only the request half (_update_media_buy_impl returns success, adapter not called now, step created). None drive the approval flow. Required additions:

• Drive the admin approve endpoint → assert adapter update called, DB reflects the change, completion webhook fired, step completed.
• Assert the response is UpdateMediaBuySubmitted with a task_id that get_task resolves.
• A rejection test: no changes applied + buyer notified.

Scope boundary

Owns the manual-approval update path end-to-end (server apply + update-response envelope + task_id). Auto-path adapter dispatch is #1089/#1529/#1261; create-response and delivery envelopes remain #1305. All under #1531.

[KonstantinMirin]

Root cause

_update_media_buy_impl in src/core/tools/media_buy_update.py (line 268-283) checks if manual approval is required and returns immediately with affected_packages=[] — skipping all update logic. No validation, no persistence, no signal to the client that approval is pending.

The manual approval flow should:

Step	What should happen	What actually happens
1	Check approval path	Done
2	Create workflow step (requires_approval)	Done
3	Persist the pending update request	Skipped
4	Send notification to Seller	Skipped
5	Return success with implementation_date: null	Returns empty affected_packages=[] with no indication of pending state
6+	Seller reviews, approves/rejects, system applies	No pending request exists to approve

Secondary bug: campaign-level pause/resume (line 400-442) returns without marking the workflow step as completed, leaving it stuck in in_progress forever. This happens regardless of approval settings.

Protocol behavior

Per the task-status lifecycle, when approval is needed:

• Protocol envelope status should be submitted (not completed)
• implementation_date should be null (pending approval)
• Buyer can poll tasks/get to check progress
• After seller approves: system applies update and notifies buyer via webhook

The AdCP spec defines UpdateMediaBuyAsyncResponseInputRequired with reason: APPROVAL_REQUIRED for exactly this case.

Proposed fix

Follow the existing create_media_buy approval pattern: run validation, compute affected packages, persist the pending update (update raw_request on the MediaBuy with the new proposed state as a full snapshot, set status to pending_approval), send seller notification, return populated affected_packages with implementation_date=null and envelope status submitted.

Open question: which media buy statuses should allow updates?

The protocol defines four media buy statuses:

Protocol Status	Description
pending_activation	Created but not yet activated
active	Currently running
paused	Temporarily paused
completed	Finished running

Internally, salesagent has additional states that all map to pending_activation for the buyer:

DB Status	Admin UI State	Protocol Status
draft	draft	pending_activation
pending_approval	needs_approval	pending_activation
pending_creatives	needs_creatives	pending_activation
scheduled	scheduled	pending_activation
active	live	active
paused	paused	paused
completed	completed	completed
failed	failed	(no mapping)

Note: get_media_buy_delivery (the buyer-facing read endpoint) ignores the DB status entirely and recomputes from flight dates.

Currently there are no status guards in _update_media_buy_impl. The only place that might reject an invalid update is the adapter downstream (e.g. GAM rejects updates to archived line items). The UC-003 preconditions also don't specify status restrictions — this is a gap.

Questions:

• Should we restrict updates to active + paused + pending_activation?
• Should pending_approval allow updates (overwriting the pending request)?
• Should updating a completed media buy be rejected outright?
• Should this gap be registered in the protocol spec?

[KonstantinMirin]

Update: Further tracing revealed that update_media_buy also lacks the protocol-level status wrapper that create_media_buy uses (via CreateMediaBuyResult). Even after fixing the approval gate, the A2A path would still report the task as completed instead of submitted. This is part of a broader architectural issue where TaskStatus (protocol envelope concern) is mixed into domain responses rather than being set by the protocol layer. We'll fix update_media_buy to match the existing pattern for now; proper separation is tracked separately.

[KonstantinMirin]

@bokelley I would appreciate some help here as the mapping of statuses is not clear from the specs and implementation. What was the original intent?

[bokelley]

Thanks for the thorough analysis, @KonstantinMirin. The proposed fix is right — follow the create_media_buy approval pattern.

On the status mapping question: I've pushed a spec clarification to adcontextprotocol/adcp that addresses the precondition gap in UC-003. Summary of what changed:

Allowed states for update_media_buy:

• active, paused, pending_activation (and any internal sub-states that map to it) → updates allowed
• completed → reject with INVALID_STATE
• pending_activation covers all pre-activation internal states from the buyer's perspective; overwriting a pending approval request is valid (seller hasn't acted yet)

The spec was also wrong about affected_packages: the previous example showed affected_packages: [] for the approval-pending case, which matches the buggy implementation rather than the intended behavior. The spec now requires affected_packages to contain the proposed package state when approval is pending — not empty.

failed state: not exposed to buyers via the protocol. Left as an implementation concern for now; if salesagent needs to surface terminal failures to orchestrators, that's a separate discussion worth its own issue.

The TaskStatus wrapper separation is tracked separately per your note — agree it shouldn't block this fix.