Skip to content

Commit 482c324

Browse files
bokelleyclaude
andcommitted
fix(server): protocol-polish follow-ups from PR #341 review
ad-tech-protocol-expert called out three follow-ups from PR #341. This PR ships #2 + #3 with concrete fixes; #1 (MCP structuredContent migration) is documented as known-bridge — needs lowlevel Server.call_tool refactor. #3 — Truncation marker buyer-parseability. Pre-fix the MCP ToolError text-suffix path emitted bare '...' as the truncation marker, making buyer json.loads throw JSONDecodeError with no signal of why. Now emits a sentinel object: {_truncated: true, _reason: 'size' | 'non_serializable', _partial: '...'} — ALWAYS valid JSON. Buyers split on '\nDetails: ' prefix, json.loads the tail, and branch on the marker. Iterative fit replaces naive head-room subtract so quote/backslash escaping in _partial doesn't blow past the cap. #2 — caused_by.type Python-only debug breadcrumb. Documented in _internal_error_details that this field is a debug hint for the seller dev reading their own server logs, NOT a cross-language contract. JS sellers won't emit Python-typed exception names. Buyers wanting structured retry/fix/abandon semantics should read 'recovery' (terminal/correctable/transient), which IS the spec contract. #1 — MCP structuredContent migration. Documented as bridge in _to_mcp's docstring with the file:line citation of FastMCP's lossy _make_error_result. The protocol-correct fix needs lowlevel Server.call_tool registration so we can return CallToolResult directly with structuredContent=adcp_error + isError=True; deferred to a separate effort because it touches the FastMCP→ lowlevel boundary throughout serve.py. Tests: 1 updated (truncation now asserts parseable sentinel), 1 new (unserializable details emits sentinel, not malformed JSON). Test count: 2883 (was 2882). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 4938ee1 commit 482c324

3 files changed

Lines changed: 151 additions & 20 deletions

File tree

src/adcp/decisioning/dispatch.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,20 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]:
364364
(typo-shaped) from ``KeyError`` (missing-config-shaped) from
365365
``ConnectionError`` (network-shaped) at a glance.
366366
367+
**``caused_by.type`` is a debug breadcrumb, not a wire contract.**
368+
The value is Python's exception class name verbatim
369+
(``"AttributeError"``, ``"KeyError"``, ``"ValidationError"``).
370+
Buyers built against the JS SDK won't see Python-flavoured class
371+
names from JS sellers — only Python sellers leak Python types.
372+
Treat this field as "hint to the seller dev reading their own
373+
server logs," NOT as something to branch on programmatically
374+
cross-language. The AdCP spec at ``schemas/cache/core/error.json``
375+
keeps ``details`` as ``additionalProperties: true`` so this is
376+
spec-compliant; it's just not portable. Buyer agents that want
377+
structured retry/fix/abandon classification should read
378+
``recovery`` (terminal/correctable/transient) which IS the
379+
cross-language contract.
380+
367381
Truncation is defense-in-depth against an adopter who throws on
368382
secret material and ends up with a repr that includes the secret
369383
value verbatim. The full traceback is in the server log via

src/adcp/server/translate.py

Lines changed: 92 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -234,11 +234,23 @@ def _to_mcp(
234234
235235
When ``details`` is non-empty, the JSON-serialised payload is
236236
appended after a ``\\nDetails: `` line. Buyer agents can split on
237-
that prefix and ``json.loads`` the rest. AudioStack Emma P0:
238-
pre-fix the wire said "see details for cause" but the dispatch's
237+
that prefix and ``json.loads`` the rest — the result is ALWAYS
238+
valid JSON (truncation/serialization failures emit a sentinel
239+
object, never a bare ``...``). AudioStack Emma P0: pre-fix the
240+
wire said "see details for cause" but the dispatch's
239241
``details.caused_by`` (and #341's ``validation_errors``) never
240242
reached MCP buyers — only A2A. Now both transports surface the
241243
structured breadcrumb.
244+
245+
**Bridge, not endpoint.** The protocol-correct shape is MCP's
246+
``CallToolResult.structuredContent`` (carries ``isError=True``
247+
AND a structured ``adcp_error`` object on the same envelope —
248+
see ``mcp.types.CallToolResult``). FastMCP's
249+
``_make_error_result`` (``mcp/server/lowlevel/server.py:467``)
250+
drops ``structuredContent`` for error results, so we can't reach
251+
that channel via FastMCP's ``ToolError`` raise path. Migrating
252+
to lowlevel ``Server.call_tool`` registration would unlock it;
253+
until then this text-suffix is the working bridge.
242254
"""
243255
if field:
244256
text = f"{code}[{field}]: {message}"
@@ -247,20 +259,86 @@ def _to_mcp(
247259
if suggestion:
248260
text += f"\nSuggestion: {suggestion}"
249261
if details:
250-
# Truncation: the entire MCP error is one ``text`` field, so a
251-
# huge details dict bloats every error response. Cap the JSON
252-
# payload at 8 KB — generous enough for the structured
253-
# breadcrumb shapes (caused_by + validation_errors typically
254-
# under 2 KB) but bounded against an adopter who fills details
255-
# with raw repr or DB query strings.
262+
text += f"\nDetails: {_serialize_details_for_mcp(details)}"
263+
return ToolError(text)
264+
265+
266+
#: Cap on the JSON-serialised ``details`` payload appended to MCP
267+
#: ToolError text. Generous enough for typical
268+
#: ``caused_by`` + ``validation_errors`` shapes (under 2 KB) and
269+
#: bounded against an adopter who fills ``details`` with raw repr
270+
#: or DB query strings. Not configurable today; if an adopter
271+
#: needs more, an env-var escape hatch is the right next step.
272+
_MCP_DETAILS_MAX_BYTES = 8192
273+
274+
275+
def _serialize_details_for_mcp(details: dict[str, Any]) -> str:
276+
"""Serialise ``details`` to a JSON string suitable for embedding
277+
in the MCP ToolError text payload.
278+
279+
The output is ALWAYS valid JSON — even when truncation fires
280+
or ``json.dumps`` raises. Buyer agents can split on the
281+
``\\nDetails: `` prefix and ``json.loads`` the tail without
282+
branching on serialization quality. Truncation is signalled via
283+
the ``_truncated`` field on the sentinel object so buyers can
284+
surface a "details elided; see server logs" UX hint.
285+
286+
Pre-fix (PR #341 ship): truncation emitted a bare ``...`` suffix
287+
on the JSON tail, which made the buyer's ``json.loads`` throw
288+
``JSONDecodeError`` with no signal that the cause was
289+
server-side truncation. ad-tech-protocol-expert called this
290+
out as a follow-up before the wire shape became de-facto
291+
convention.
292+
"""
293+
try:
294+
details_json = json.dumps(details, separators=(",", ":"), default=str)
295+
except Exception:
296+
# Non-serializable details (rare — ``default=str`` catches
297+
# most). Emit a sentinel so the buyer's parse still
298+
# succeeds. ``str(details)`` may also raise on circular refs
299+
# — guarded with try/except + a fallback empty partial.
256300
try:
257-
details_json = json.dumps(details, separators=(",", ":"), default=str)
301+
partial = str(details)[: _MCP_DETAILS_MAX_BYTES // 2]
258302
except Exception:
259-
details_json = str(details)
260-
if len(details_json) > 8192:
261-
details_json = details_json[:8189] + "..."
262-
text += f"\nDetails: {details_json}"
263-
return ToolError(text)
303+
partial = ""
304+
return json.dumps(
305+
{
306+
"_truncated": True,
307+
"_reason": "non_serializable",
308+
"_partial": partial,
309+
},
310+
separators=(",", ":"),
311+
)
312+
if len(details_json) <= _MCP_DETAILS_MAX_BYTES:
313+
return details_json
314+
# Truncation: emit a sentinel object (always valid JSON).
315+
# ``_partial`` carries as much of the original payload as fits
316+
# inside the cap; buyers that want the full payload pull it
317+
# from server logs (still in ``logger.exception`` traces).
318+
#
319+
# Iterate to fit: JSON encoding of ``_partial`` adds backslash
320+
# escaping (each `"` becomes `\\"`, each `\\` becomes `\\\\`),
321+
# so a naive headroom calc undershoots when the partial contains
322+
# quotes or backslashes. Halve until the encoded sentinel fits.
323+
cut = _MCP_DETAILS_MAX_BYTES - 64 # rough headroom for sentinel keys
324+
while cut > 0:
325+
encoded = json.dumps(
326+
{
327+
"_truncated": True,
328+
"_reason": "size",
329+
"_partial": details_json[:cut],
330+
},
331+
separators=(",", ":"),
332+
)
333+
if len(encoded) <= _MCP_DETAILS_MAX_BYTES:
334+
return encoded
335+
cut = max(0, cut - max(64, (len(encoded) - _MCP_DETAILS_MAX_BYTES) * 2))
336+
# Cut hit zero — emit a no-partial sentinel so the buyer still
337+
# sees that something was truncated.
338+
return json.dumps(
339+
{"_truncated": True, "_reason": "size", "_partial": ""},
340+
separators=(",", ":"),
341+
)
264342

265343

266344
def _to_a2a(

tests/test_translate.py

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -458,15 +458,21 @@ def test_validation_errors_reach_mcp_buyer_via_details(self) -> None:
458458
loc_fields = {err["loc"][-1] for err in parsed["validation_errors"]}
459459
assert loc_fields == {"width", "height"}
460460

461-
def test_huge_details_truncated_at_8kb(self) -> None:
461+
def test_huge_details_truncated_with_parseable_sentinel(self) -> None:
462462
"""A pathological ``details`` payload (huge repr, DB query
463-
string) gets truncated to 8KB so the MCP error response stays
464-
bounded. Defense-in-depth against an adopter who fills
465-
``details`` with raw debug payloads."""
463+
string) gets truncated to 8 KB so the MCP error response
464+
stays bounded. The truncated payload is ALWAYS valid JSON —
465+
emits a ``{"_truncated": true, "_reason": "size", "_partial":
466+
"..."}`` sentinel so buyer agents can ``json.loads`` and
467+
branch on the marker (ad-tech-protocol-expert P1 follow-up
468+
from PR #341 — pre-fix the bare ``...`` suffix made buyer
469+
``json.loads`` throw ``JSONDecodeError`` with no signal of
470+
why)."""
471+
import json as _json
472+
466473
from adcp.decisioning.types import AdcpError as DecisioningAdcpError
467474
from adcp.server.translate import translate_error
468475

469-
# Build a details dict that JSON-encodes to >8 KB.
470476
big = {"blob": "X" * 10_000}
471477
exc = DecisioningAdcpError(
472478
"INTERNAL_ERROR",
@@ -478,4 +484,37 @@ def test_huge_details_truncated_at_8kb(self) -> None:
478484
text = str(tool_err.args[0]) if tool_err.args else str(tool_err)
479485
details_part = text.split("\nDetails: ", 1)[1]
480486
assert len(details_part) <= 8192
481-
assert details_part.endswith("...")
487+
# Critical: the truncated payload MUST be valid JSON.
488+
parsed = _json.loads(details_part)
489+
assert parsed["_truncated"] is True
490+
assert parsed["_reason"] == "size"
491+
assert "_partial" in parsed
492+
493+
def test_unserializable_details_emits_sentinel(self) -> None:
494+
"""When ``json.dumps`` itself raises (a circular reference or
495+
other unrepresentable shape that even ``default=str`` can't
496+
coerce), the function emits a parseable sentinel rather than
497+
letting the failure propagate or returning malformed JSON."""
498+
import json as _json
499+
500+
from adcp.decisioning.types import AdcpError as DecisioningAdcpError
501+
from adcp.server.translate import translate_error
502+
503+
# Circular reference defeats both ``json.dumps`` and the
504+
# ``default=str`` fallback (str() can't represent the cycle
505+
# either — it raises ``ValueError: Circular reference``).
506+
circular: dict[str, object] = {}
507+
circular["self"] = circular
508+
exc = DecisioningAdcpError(
509+
"INTERNAL_ERROR",
510+
message="circular",
511+
recovery="terminal",
512+
details=circular,
513+
)
514+
tool_err = translate_error(exc, protocol="mcp")
515+
text = str(tool_err.args[0]) if tool_err.args else str(tool_err)
516+
details_part = text.split("\nDetails: ", 1)[1]
517+
# Even on serialization failure, the output is parseable.
518+
parsed = _json.loads(details_part)
519+
assert parsed["_truncated"] is True
520+
assert parsed["_reason"] == "non_serializable"

0 commit comments

Comments
 (0)