Skip to content

fix(server): comply_test_controller returns dict to fix controller_detected: false#317

Merged
bokelley merged 2 commits intomainfrom
claude/issue-314-controller-detected-fix
Apr 30, 2026
Merged

fix(server): comply_test_controller returns dict to fix controller_detected: false#317
bokelley merged 2 commits intomainfrom
claude/issue-314-controller-detected-fix

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #314

Root Cause

The JS storyboard runner (@adcp/sdk) detects the test controller by calling comply_test_controller(scenario='list_scenarios') and checking the response shape. Its detectController function:

const data = result.data;
if (data.success && data.scenarios) {
    return { detected: true, scenarios };
}
return { detected: false };

Previously, comply_test_controller returned json.dumps(result) — a string. FastMCP wraps string-returning tools as structuredContent: {result: "<json string>"}. The runner's unwrapProtocolResponse extracts that as data = {result: "<string>"}, so data.success and data.scenarios are both undefinedcontroller_detected: false.

Returning the result dict directly causes FastMCP to emit the fields at the top level (structuredContent: {success: true, scenarios: [...]} or via content[0].text). Either path gives the runner data.success = true and data.scenarios = [...]controller_detected: true.

Confirmed locally: before the fix, the live MCP response was:

"structuredContent": {"result": "{\"success\": true, \"scenarios\": [...]}"}

After the fix:

"structuredContent": {"success": true, "scenarios": ["force_creative_status", ...]}

What Changed

  • src/adcp/server/test_controller.py: comply_test_controller closure in register_test_controller changed from -> str + json.dumps(result) to -> dict[str, Any] returning the result directly. The json import is still live (used for the 256 KB payload size check in _handle_test_controller).
  • tests/test_test_controller_context.py: removed the json.loads() call in the one test that was calling tool.fn directly; added a regression test that calls fn(scenario='list_scenarios') through the FastMCP registration path and asserts the result is a dict (not a string).

What Tested

  • pytest tests/test_test_controller_context.py tests/test_force_create_media_buy_arm_and_force_task_completion.py tests/test_server_dx.py — 75 passed
  • pytest tests/ (full suite, excluding pre-existing TLS conformance failure unrelated to this change) — 2312 passed, 0 new failures
  • ruff check src/ — passed
  • Live MCP wire test: tools/call comply_test_controller scenario=list_scenarios confirmed structuredContent now contains the parsed dict

Pre-PR review:

  • code-reviewer: approved — no blockers; flagged missing regression test (now added), nit on fn_metadata comment
  • dx-expert: approved — no blockers; clarified that detection works via the text-content fallback path when FastMCP omits structuredContent for dict returns; all four backward-compat questions resolved (no breaking callers, json import remains live, no guide update needed)

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

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


Generated by Claude Code

claude added 2 commits April 30, 2026 03:04
…tected: false

FastMCP wraps a string-returning tool as structuredContent: {result: "<json>"},
but the JS runner's response unwrapper extracts that dict as completedData —
so data.success and data.scenarios are both undefined, and detectController()
returns {detected: false}.

Returning a dict causes FastMCP to put the fields directly in structuredContent,
so the unwrapper produces {success: true, scenarios: [...]} as completedData and
detection works correctly.

Closes #314

https://claude.ai/code/session_01LuThNVrmmHfVvPbfrbWsvy
…urn path

Ensures the FastMCP registration path returns a dict (not a JSON string)
from list_scenarios so the JS runner can read data.success/data.scenarios.
Catches the structuredContent double-encoding regression from #314.

https://claude.ai/code/session_01LuThNVrmmHfVvPbfrbWsvy
@bokelley bokelley marked this pull request as ready for review April 30, 2026 13:23
@bokelley bokelley merged commit 9244d46 into main Apr 30, 2026
11 of 12 checks passed
@bokelley bokelley deleted the claude/issue-314-controller-detected-fix branch April 30, 2026 13:23
bokelley added a commit that referenced this pull request Apr 30, 2026
…) (#322)

End-to-end against npx @adcp/client adcp storyboard run now reports
overall_status: passing with 47/47 individual steps passing and
controller_detected: true (was 36/47 partial after #317).

Three fixes against examples/seller_agent.py:

1. Add four runner-fixture products to PRODUCTS — outdoor_display_q2,
   outdoor_video_q2, sports_preroll_q2, lifestyle_display_q2. The
   @adcp/client storyboard YAMLs reference these by ID without an
   explicit seed_product setup step; the seller is expected to know
   them out of the box. Pricing option IDs (cpm_standard,
   cpm_guaranteed) match what the compliance YAMLs send.

2. Validate measurement_terms in create_media_buy — reject with
   TERMS_REJECTED when max_variance_percent < 5 or measurement_window
   is not in (c3, c7). Source-of-truth is the
   measurement_terms_rejected.yaml storyboard which probes with
   c30 + 0% (rejected path) then retries with c7 + 10% (accepted
   path). Acceptance threshold matches the runner's relaxed shape.

3. Persist targeting_overlay (and friends) on packages — both
   create_media_buy and update_media_buy now preserve
   targeting_overlay, creative_assignments, creatives, and
   measurement_terms on the persisted package state, then surface
   them on get_media_buys. Storyboard
   inventory_list_targeting/verify_create_persisted round-trips
   property_list.list_id; the swap variant exercises the update
   parity check.

Once this lands, the storyboard CI job (#309, currently
continue-on-error: true) is promotable to required.

Closes #319

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

storyboard: investigate why controller_detected stays false despite list_scenarios returning all 12 scenarios

2 participants