Skip to content

fix(bindings): sanitize non-JSON-native types at emit() FFI boundary - #76

Merged
Brian Krabach (bkrabach) merged 4 commits into
mainfrom
fix/emit-json-sanitize-228
May 12, 2026
Merged

fix(bindings): sanitize non-JSON-native types at emit() FFI boundary#76
Brian Krabach (bkrabach) merged 4 commits into
mainfrom
fix/emit-json-sanitize-228

Conversation

@kenotron-ms

Copy link
Copy Markdown
Contributor

Summary

  • Fixes #228: Non-JSON-native Python types (Decimal, datetime, custom objects) no longer crash emit() and related registry methods when sent as event payloads
  • Added json_dumps_safe() DRY helper in helpers.rs that threads default=str through the FFI boundary — all non-JSON types now serialize via their str() representation
  • Incidental fix: resolves provider-anthropic cost_usd Decimal serialization issue (related to feat: add --local-source to E2E smoke test + fix input_schema test #52 fix)

Sites Fixed

  • bindings/python/src/helpers.rs — new DRY helper
  • bindings/python/src/hooks.rsemit(), set_default_fields(), emit_and_collect()
  • bindings/python/src/bridges.rsPyHookHandlerBridge::handle() result path
  • python/amplifier_core/loader_grpc.pyGrpcToolBridge._serialize_input()

Test Coverage

  • 5 new tests in bindings/python/tests/test_emit_json_sanitize.py
  • 1 new test in bindings/python/tests/test_loader_grpc.py
  • Full suite: 1016 passed, 1 skipped, 0 failed

Design doc: docs/plans/2026-05-08-emit-json-sanitize-design.md


Generated with Amplifier

All four unguarded json.dumps() call sites at the Python/Rust FFI
boundary now pass default=str so Decimal, datetime, and any other
non-JSON-native type becomes its str() representation instead of
raising TypeError and crashing the caller.

Sites fixed:
- helpers.rs: new json_dumps_safe() DRY helper used at all Rust sites
- hooks.rs: emit(), set_default_fields(), emit_and_collect()
- bridges.rs: PyHookHandlerBridge::handle() result-serialization path
- loader_grpc.py: GrpcToolBridge._serialize_input()

Incidental fix: provider-anthropic cost_usd crash (PR #52) is resolved
automatically. str(Decimal("1.23")) == "1.23", identical to the existing
@field_serializer output on the Pydantic model path.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@kenotron-ms
Ken Chau (kenotron-ms) force-pushed the fix/emit-json-sanitize-228 branch from d988a22 to 8d714fa Compare May 9, 2026 05:37
Ken Chau (kenotron-ms) and others added 3 commits May 8, 2026 23:44
- pyproject.toml: 1.5.1 → 1.5.2
- crates/amplifier-core/Cargo.toml: 1.5.1 → 1.5.2
- bindings/python/Cargo.toml: 1.5.1 → 1.5.2
- python/amplifier_core/__init__.py: 1.5.1 → 1.5.2
- scripts/bump_version.py: regex fix to handle __version__ (trailing __)
- Cargo.lock: updated

Verified: E2E smoke test PASSED — manylinux wheel built, pristine-import OK, Docker container test passed.

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Fix rustfmt CI check failure by applying standard formatting to:
- json_dumps_safe function signature in helpers.rs
- test binding in tests/mod.rs

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…ructural guard test

- Apply json_dumps_safe() to all remaining unguarded json.dumps() calls:
  * session.rs: session config serialization
  * coordinator/mod.rs: coordinator config serialization
  * wasm.rs: tool input, provider request/response, hook data, context message, set_messages, approval request (7 sites)
- Add no_raw_json_dumps_outside_helpers structural test that scans .rs files and fails
  if raw json.dumps() calls appear outside the safe helper, preventing future regressions
- Remove private repo reference from test docstring

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@kenotron-ms
Ken Chau (kenotron-ms) force-pushed the fix/emit-json-sanitize-228 branch from f482316 to 75fccb7 Compare May 9, 2026 06:44
@kenotron-ms

Copy link
Copy Markdown
Contributor Author

DTU Validation — Pre-fix provider-anthropic + fixed amplifier-core

To validate that the default=str safety net at the FFI boundary is sufficient on its own (independent of any provider-side fix), a Digital Twin Universe was run with the following deliberately adversarial stack:

Component Version / State
amplifier-core fix/emit-json-sanitize-228 — this PR (1.5.2 wheel, built inside DTU)
provider-anthropic c07131df6a123809d3806b72599732ab3fa71197 — base of PR #53, before the provider-side str() conversion landed
amplifier CLI + foundation upstream main

The provider at that commit passes cost_usd (a Decimal) directly into the llm:response event dict without converting it — this is the exact code path that caused the original crash. The question being tested: does the Core fix catch it even when the provider doesn't?

Results

[PASS] amplifier, version 2026.05.08-a0e1627 (core 1.5.2) — branch wheel confirmed loaded
[PASS] provider-anthropic @ c07131df confirmed installed (settings.yaml source pin)
[PASS] ANTHROPIC_API_KEY passed through
[PASS] amplifier run "Say exactly: amplifier-tester-ok"
         Tokens:  57,377 in / 61 out / 57,438 total
         Cost:    $0.22
         Reply:   amplifier-tester-ok
         Exit:    0
[PASS] No TypeError, no JSON serialization crash anywhere in the run

Why the cost stamp is the proof

The $0.22 line is rendered by the cost-tracking hook that consumes the llm:response event. For that to print, the event payload had to pass through emit() at the Python→Rust FFI boundary — the exact call site that this PR hardens with json_dumps_safe(). The session exited 0, confirming the boundary now tolerates non-JSON-native types regardless of what the provider passes.

Also confirmed

  • Downstream repo check: amplifier, amplifier-app-cli, amplifier-foundation — no [tool.uv.sources] git overrides for amplifier-core
  • E2E smoke test (from scripts/e2e-smoke-test.sh): PASSED — manylinux wheel built, pristine-import preflight passed, core 1.5.2 confirmed, no crashes ✅

@bkrabach
Brian Krabach (bkrabach) merged commit 47fd2ad into main May 12, 2026
6 checks passed
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.

2 participants