fix: Decimal JSON serialization for Usage.cost_usd — required for M2 cost stamping - #73
Merged
Merged
Conversation
Ken Chau (kenotron-ms)
force-pushed
the
fix/m2-decimal-json-serialization
branch
from
May 7, 2026 18:15
66bac8c to
0add90b
Compare
…us.cost_usd — revives PR #73 Revives the approach from amplifier-core PR #73 (closed without merge 2026-05-07T18:15:32Z) with one critical refinement: use when_used='always' instead of default mode='json'-only. PROBLEM: M2 cost-tracking introduced cost_usd: Decimal | None on Usage and SessionStatus. Without serializer help, plain model_dump() (called by every orchestrator: loop-streaming line 472,505, loop-basic 361, loop-events 307,337) emits Decimal values into events, which crash json.dumps() at every downstream JSONL write site (hooks-logging, hooks-backup, hook-shell, context-persistent, foundation/serialization.py). A "patch every consumer" approach doesn't scale to N orchestrators or future hook modules. FIX: Move conversion to the model itself using @field_serializer("cost_usd", when_used="always"): - model_dump() and model_dump(mode='json') both return JSON-safe strings for cost_usd - Direct attribute access still returns Decimal for in-memory math - None survives as None (preserves None ≠ Decimal("0") invariant for "unknown" vs "free") - Plus _json_default(obj) in models.py + default= in ToolResult.get_serialized_output() for non-Pydantic paths where tool outputs may carry Decimal WHY "when_used='always'" MATTERS: PR #73 used default field_serializer behavior, which fires only on mode='json'. But every orchestrator calls plain model_dump(). Verification today (Pydantic 2.13.4) confirmed that without when_used='always', Decimal values still leak into events on the plain-dump path. VERIFIED BEHAVIORS (10 tests): ✓ model_dump() returns string for cost_usd ✓ json.dumps(usage.model_dump()) doesn't crash ✓ Direct usage.cost_usd access still returns Decimal ✓ Existing field_validator rejecting float still fires ✓ extra="allow" still works for provider-specific fields ✓ None survives as None (not "None") ✓ Round-trip model_validate(model_dump()) rebuilds with Decimal ✓ Precision preserved across 0.0000123, 12345.6789, 0, 1E-9 ✓ Plain model_dump() and mode='json' produce identical output ✓ int coerces to Decimal then serializes to string NEW TESTS: - tests/test_cost_usd_serialization.py: 25 tests (all pass) - 129 adjacent tests still pass (test_message_models, test_session, test_tool_result_autopop, test_interfaces, test_generated_equivalence) VERSION BUMP: 1.5.0 → 1.5.1 (PATCH, per release-mandate) - pyproject.toml - crates/amplifier-core/Cargo.toml - bindings/python/Cargo.toml - python/amplifier_core/__init__.py E2E SMOKE TEST: PASSED (run by user before push, per release-mandate Pre-Merge Gate) FILES CHANGED: - python/amplifier_core/message_models.py: +field_serializer on Usage.cost_usd - python/amplifier_core/models.py: +field_serializer on SessionStatus.cost_usd + _json_default + default= in ToolResult - tests/test_cost_usd_serialization.py: new, 234 lines - Version files: all bumped to 1.5.1 - Cargo.lock: regenerated by Rust toolchain UNBLOCKS M2/M3 milestone (microsoft-amplifier/amplifier-support#225): Once landed, these 11 PRs can drop per-consumer Decimal sanitizer code: - foundation #190: drop Decimal→str branch from serialization.py - hooks-logging #5: close as superseded (model_dump(mode='json') becomes redundant) - provider PRs: anthropic #52, openai #32, azure-openai #24, gemini #28, vllm #22, ollama #14, chat-completions #5 - consumer PRs: app-cli #171, hooks-streaming-ui #11 (unchanged) Plus 3 modules no longer need follow-up PRs: - amplifier-module-hooks-backup (automatic safety) - amplifier-module-hook-shell (automatic safety) - amplifier-module-context-persistent (automatic safety) Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Contributor
Author
|
Reopened with refinement: switched from default |
Ken Chau (kenotron-ms)
added a commit
to microsoft/amplifier-foundation
that referenced
this pull request
May 8, 2026
…core #73 The Decimal handling was added defensively when Usage.cost_usd emitted Decimal into events. With amplifier-core #73 adding @field_serializer (when_used='always') on Usage.cost_usd and SessionStatus.cost_usd directly, plain model_dump() produces JSON-safe strings before reaching this sanitizer. The branch is dead code in the M2/M3 milestone path. Other Decimal-bearing types added in the future should follow the same pattern (model-level @field_serializer), keeping the sanitizer focused on its original purpose: handling untyped LLM API response objects. Tests still pass: 19/19 (TestSanitizeForJson, TestSanitizeMessage, plus 3 cost_bridge_foundation tests). Refs: microsoft/amplifier-core#73, microsoft-amplifier/amplifier-support#225 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…tract The test was written for the OLD contract (model_dump() returns Decimal). With @field_serializer(when_used='always') on Usage.cost_usd, plain model_dump() now returns string — same as mode='json'. Renamed and updated assertion to match. Added a "direct attribute access still returns Decimal" assertion to make the bidirectional contract explicit. Self-correction: I missed sweeping bindings/python/tests/ in the original PR's verification pass; only ran tests/. The CI catch is exactly what the release-mandate Pre-Merge Gate is for. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Test docstrings shouldn't read like a changelog. A reader landing on
these tests cares about what cost_usd actually is, not which milestone
introduced it or which PR-number-of-the-week it was last fixed in.
- Module docstring opens with "cost_usd is Decimal in memory ... JSON-safe
string when serialized" and the None ≠ Decimal("0") invariant
- Class docstrings: "Decimal in memory, string on the wire"
- Per-test docstrings describe what the test verifies, not why history
forced it (no @field_serializer, no _json_default, no PR #73, no M2)
- Also: regenerate uv.lock for the 1.5.0 → 1.5.1 version bump that was
missed when bumping the other version files manually
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Salil Das (sadlilas)
pushed a commit
to microsoft/amplifier-foundation
that referenced
this pull request
May 8, 2026
* feat(m3a): bridge child session costs in PreparedBundle.spawn() * refactor: improve _sum_cost_usd comment to describe purpose rather than implementation note * refactor: consolidate error handling inside _bridge_child_cost; harden _sum_cost_usd against malformed values * refactor: promote _bridge_child_cost and _sum_cost_usd to public API; add to __init__ and API_REFERENCE.md 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * fix: NameError in PreparedBundle.spawn() — _bridge_child_cost → bridge_child_cost The call site at bundle/_prepared.py:722 used the underscore-prefixed name _bridge_child_cost, which does not exist. The function is defined (and exported) as bridge_child_cost. This caused a runtime NameError every time PreparedBundle.spawn() was called with a non-None parent_session, silently breaking cost bridging for all library-level agent spawns. Also adds two integration tests that drive PreparedBundle.spawn() directly (not just bridge_child_cost in isolation) to catch this class of call-site typo going forward: - test_spawn_calls_bridge_child_cost_with_parent: verifies bridge is called with the child coordinator, parent coordinator, and correct session_id. - test_spawn_does_not_call_bridge_without_parent: verifies rootless spawns skip the bridge entirely. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * refactor: drop Decimal branch from sanitize_for_json — superseded by core #73 The Decimal handling was added defensively when Usage.cost_usd emitted Decimal into events. With amplifier-core #73 adding @field_serializer (when_used='always') on Usage.cost_usd and SessionStatus.cost_usd directly, plain model_dump() produces JSON-safe strings before reaching this sanitizer. The branch is dead code in the M2/M3 milestone path. Other Decimal-bearing types added in the future should follow the same pattern (model-level @field_serializer), keeping the sanitizer focused on its original purpose: handling untyped LLM API response objects. Tests still pass: 19/19 (TestSanitizeForJson, TestSanitizeMessage, plus 3 cost_bridge_foundation tests). Refs: microsoft/amplifier-core#73, microsoft-amplifier/amplifier-support#225 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --------- Co-authored-by: Ken Chau <kchau@microsoft.com> Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What broke
M2 cost-tracking added
cost_usd: Decimal | NonetoUsageandSessionStatus. Every orchestrator (loop-streaming, loop-basic, loop-events) callsmodel_dump()to serialize these models into events. With no field-level serializer,model_dump()emits rawDecimalvalues, which then crashjson.dumps()at downstream JSONL write sites.The milestone-wide sweep identified at least 5 such write sites across 4 modules:
amplifier-module-hooks-logging(fix(core): Handle encoding errors in exception logging for Windows #5 attempted a per-module fix viamode='json')amplifier-module-hooks-backup(unpatched)amplifier-module-hook-shell(unpatched)amplifier-module-context-persistent(unpatched)amplifier-foundation/serialization.py(#190 attempted a module-level Decimal branch)A "patch every consumer" approach doesn't scale to N orchestrators or future hook modules.
Root cause
The Decimal field had no
@field_serializer, somodel_dump()returned the rawDecimalobject — JSON-incompatible. Adding a field-level serializer that convertsDecimal -> stron every dump path is the correct boundary defense.Solution
Define
@field_serializer("cost_usd", when_used="always")on bothUsage.cost_usdandSessionStatus.cost_usd.when_used="always"is explicit (it matches Pydantic 2.x's default for@field_serializer, but stating it makes the contract unambiguous and survives any future Pydantic default change without surprise).Usage.cost_usd— serializer returnsstr(v) if v is not None else NoneSessionStatus.cost_usd— same shapeToolResult.get_serialized_output()— adds_json_default(obj)handler +default=argument tojson.dumps()for the non-Pydantic path (tool outputs that may carryDecimalvalues e.g.{"price": Decimal("9.99")})Invariants preserved:
usage.cost_usdaccess still returnsDecimalfor in-memory mathNonestays asNone(preserves theNone != Decimal("0")invariant for "unknown" vs "free")"0.0000123", not rounded or truncated)field_validatorrejectingfloatstill firesmodel_dump()andmodel_dump(mode='json')produce identical outputVerified behaviors (10 test cases, all PASS)
model_dump()returns string forcost_usdjson.dumps(usage.model_dump())doesn't crashusage.cost_usdaccess still returnsDecimalfield_validatorrejectingfloatstill firesextra="allow"still works for provider-specific fieldsNonesurvives asNone(not"None")model_validate(model_dump())rebuilds withDecimal0.0000123,12345.6789,0,1E-9model_dump()andmode='json'produce identical outputintcoerces toDecimalthen serializes to stringTest coverage
tests/test_cost_usd_serialization.py— 25 tests, all passingVersion bump
1.5.0 -> 1.5.1 (PATCH — bug fix)
pyproject.tomlcrates/amplifier-core/Cargo.tomlbindings/python/Cargo.tomlpython/amplifier_core/__init__.pyFiles changed
python/amplifier_core/message_models.py@field_serializeronUsage.cost_usdpython/amplifier_core/models.py@field_serializeronSessionStatus.cost_usd+_json_default(obj)+default=inToolResult.get_serialized_output()tests/test_cost_usd_serialization.pyCargo.lockTotal: 286 insertions, 7 deletions across 8 files.
Unblocks M2/M3 milestone (#225)
This PR is a prerequisite for the M2/M3 cost-tracking milestone. Once merged, the following 11 PRs can drop per-consumer Decimal sanitizer code:
Foundation & framework:
amplifier-foundation#190 — can drop theDecimal -> strbranch fromserialization.py(will be cleanup commit on existing PR after this merges)Hooks:
amplifier-module-hooks-loggingfix(core): Handle encoding errors in exception logging for Windows #5 — close as superseded (mode='json'switch becomes redundant)Provider PRs (M2):
amplifier-module-provider-anthropicfeat: add --local-source to E2E smoke test + fix input_schema test #52amplifier-module-provider-openaifeat: polyglot contracts — gRPC bridges, native transport, WASM, proto conversions #32amplifier-module-provider-azure-openaifeat: add retry_after and delay_multiplier to LLMError for generalized backoff #24amplifier-module-provider-geminifix: resolve fast class by cost_tier=low #28amplifier-module-provider-vllmfeat: add input_schema to Tool protocol to match ecosystem convention #22amplifier-module-provider-ollamadocs: add metadata field to context compaction event contracts #14amplifier-module-provider-chat-completionsfix(core): Handle encoding errors in exception logging for Windows #5App/consumer PRs (M3a/M3b):
amplifier-app-cli#171 (unchanged)amplifier-module-hooks-streaming-uifix: catch BaseException in cleanup loops to survive asyncio.CancelledError #11 (unchanged)Modules no longer needing PRs (automatic safety from this fix):
amplifier-module-hooks-backup— no follow-up PR neededamplifier-module-hook-shell— no follow-up PR neededamplifier-module-context-persistent— no follow-up PR neededTracked in: microsoft-amplifier/amplifier-support#225
Side finding (future work)
The version bump script
scripts/bump_version.pyhas a regex bug: the pattern^((?:__)?version\s*=\s*\")treats__as an optional prefix but doesn't account for the suffix in__version__ = "...". The fix is one-line (^((?:__)?version(?:__)?\s*=\s*\")), tracked for a follow-up PR.Relates to: microsoft-amplifier/amplifier-support#225