From a158a07f3807e925ba7f0674917821c4d470197b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 02:44:51 +0000 Subject: [PATCH 1/2] fix(billing,registry): treat zero as a value on money controls, not as absence A fresh review of the money path turned up three falsy-zero defects on spend controls, plus two registration gaps. All five are confirmed by tests that fail without these changes. daily_limit=0 meant "unlimited" instead of "spend nothing". The cap is Optional, where None means "no cap", so a truthiness test conflated the absence of a limit with the strictest limit there is -- the value an operator sets to halt a runaway agent. This had two instances, and the second is the one that mattered: routers/billing.py converted a requested 0 to None at the API boundary, so the zero never reached the engine at all and fixing only the engine would have changed nothing. The HTTP schema declares ge=0, so zero is documented input, and app/db/converters.py already drew the distinction correctly with `is not None`. auto_refill_threshold=0 meant "alert below 100". The column is non-Optional defaulting to 100, so `or Decimal("100")` could only ever fire on exactly zero -- the one value a wallet sets to mean "never alert, never refill" -- and restored the default it had opted out of, alerting on every charge below it. register_local accepted a non-positive credits_per_unit. That number is multiplied by units and debited, so zero makes a metered tool free and a negative one pays the caller on every invoke. It is now refused at registration rather than discovered in a ledger. register_local also dropped the input schema whenever an output model was supplied: the callable fallback ran only when both schemas were absent, so a handler with plain typed arguments returning a model advertised "takes no arguments", and every client that obeyed that contract hit a TypeError. Each side now falls back independently. Tests run: pytest tests/test_billing.py (new cases verified to fail without each fix), ruff check on app/ and the test file. Not addressed here, and reported separately because each needs its own change: the nested connection checkout in authorize_and_reserve that can exhaust the pool under concurrent governed invokes; max_calls_per_tool and aggregate_value_cap being read outside the atomic reservation UPDATE; the velocity counters that are not reversed on the IntegrityError replay path; and create_permit preferring the request-body subject_key_id over the authenticated one, which contradicts the comment directly above it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m --- app/routers/billing.py | 7 +- app/services/billing_engine.py | 21 +++++- app/services/service_registry.py | 23 +++++- tests/test_billing.py | 122 +++++++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 5 deletions(-) diff --git a/app/routers/billing.py b/app/routers/billing.py index 0922e1de..1d976294 100644 --- a/app/routers/billing.py +++ b/app/routers/billing.py @@ -335,7 +335,12 @@ async def create_agent_wallet( agent_id=request.agent_id, budget_credits=Decimal(str(request.budget_credits)), daily_limit=( - Decimal(str(request.daily_limit)) if request.daily_limit else None + # `is not None`: the schema declares ge=0, so 0 is a valid cap + # meaning "spend nothing", and a truthiness test turned that + # into None -- which the engine reads as "no cap at all". + Decimal(str(request.daily_limit)) + if request.daily_limit is not None + else None ), auto_refill=request.auto_refill, auto_refill_threshold=Decimal(str(request.auto_refill_threshold)), diff --git a/app/services/billing_engine.py b/app/services/billing_engine.py index 8f1cb8b3..44f3874a 100644 --- a/app/services/billing_engine.py +++ b/app/services/billing_engine.py @@ -371,7 +371,14 @@ async def reverse_velocity_record() -> None: message="Child wallet lifetime spend cap exceeded.", ) - if wallet.daily_limit and wallet.daily_spent > wallet.daily_limit: + # `is not None`, not truthiness: daily_limit is Optional where None + # means "no cap", so Decimal("0") is a real limit -- the one an + # operator sets to stop a runaway agent dead. Reading it as falsy + # turned the strictest possible cap into no cap at all, and this is + # the only hard daily-spend enforcement in the system (the velocity + # monitor only raises alerts). app/db/converters.py already draws the + # same distinction the same way. + if wallet.daily_limit is not None and wallet.daily_spent > wallet.daily_limit: prior_daily_spent = wallet.daily_spent - charge_amount remaining = wallet.daily_limit - prior_daily_spent shortfall = charge_amount - remaining @@ -537,7 +544,17 @@ async def reverse_velocity_record() -> None: ) session.add(entry) - if wallet.balance < (wallet.auto_refill_threshold or Decimal("100")): + # Same falsy-zero trap as the daily cap. auto_refill_threshold defaults + # to 100 and is non-Optional, so `or` could only ever fire on exactly + # Decimal("0") -- the value a wallet sets to mean "never alert, never + # refill" -- and silently restored the 100 it opted out of, alerting on + # every charge below that. + low_balance_threshold = ( + wallet.auto_refill_threshold + if wallet.auto_refill_threshold is not None + else Decimal("100") + ) + if wallet.balance < low_balance_threshold: session.add( BillingAlertModel( alert_id=str(uuid.uuid4())[:12], diff --git a/app/services/service_registry.py b/app/services/service_registry.py index 156f15ad..3eb7d531 100644 --- a/app/services/service_registry.py +++ b/app/services/service_registry.py @@ -256,11 +256,30 @@ def register_local( requires a signed permit + idempotency key — even if legacy unpermitted MCP is otherwise allowed. """ + if credits_per_unit <= 0: + # A price is what the meter charges. Zero makes a governed tool + # free; negative pays the caller on every invoke, because the + # billing engine multiplies units by this number and debits the + # result. Refuse at registration rather than discovering it in a + # ledger. + raise ValueError( + f"credits_per_unit must be greater than 0, got {credits_per_unit!r}" + ) + input_schema = pydantic_to_mcp_schema(input_model) output_schema = pydantic_to_mcp_schema(output_model) - if input_schema is None and output_schema is None: - input_schema, output_schema = extract_schema_from_callable(func) + # Each side falls back independently. Requiring *both* to be absent + # meant that supplying only an output_model -- a handler with plain + # typed arguments returning a model -- left the input schema empty, so + # the tool advertised "takes no arguments" and every obedient client + # call raised TypeError on the missing parameters. + if input_schema is None or output_schema is None: + derived_input, derived_output = extract_schema_from_callable(func) + if input_schema is None: + input_schema = derived_input + if output_schema is None: + output_schema = derived_output service_record = { "service_id": service_id, diff --git a/tests/test_billing.py b/tests/test_billing.py index 1f3c1633..e7f6276e 100644 --- a/tests/test_billing.py +++ b/tests/test_billing.py @@ -2212,3 +2212,125 @@ async def commit_before_the_write_back(): second = await ledger.commit_session(session_id, get_agent_money()) assert second.wallet_id == "", second assert second.committed_charges == 0, second + + +# --- Zero is a value, not an absence --- + + +@pytest.mark.anyio +async def test_zero_daily_limit_blocks_spending_rather_than_unlocking_it( + client, api_headers +): + """``daily_limit=0`` is the strictest cap there is, not the absence of one. + + ``daily_limit`` is Optional: ``None`` means "no cap". A truthiness test + conflates that with ``Decimal("0")`` -- the value an operator sets to halt + a runaway agent -- and hands it unlimited daily spend instead. The HTTP + schema advertises ``ge=0``, so zero is a documented input, and this line is + the only hard daily-spend enforcement in the system. + """ + + sponsor_resp = await client.post( + "/v1/billing/wallets/sponsor", + json={ + "sponsor_name": "Zero Cap Co", + "email": "ops@zerocap.example", + "initial_credits": 10000, + }, + headers=api_headers, + ) + sponsor_id = sponsor_resp.json()["wallet_id"] + + agent_resp = await client.post( + "/v1/billing/wallets/agent", + json={ + "sponsor_wallet_id": sponsor_id, + "agent_id": "halted-bot", + "budget_credits": 5000, + "daily_limit": 0, + }, + headers=api_headers, + ) + assert agent_resp.status_code == 201 + agent_wallet_id = agent_resp.json()["wallet_id"] + + charge_resp = await client.post( + f"/v1/billing/charge?wallet_id={agent_wallet_id}" + "&service=agent_comms&units=1", + headers=api_headers, + ) + assert charge_resp.status_code == 402, ( + "a zero daily limit was read as 'no limit' and the charge went through" + ) + + # The wallet keeps its funds: a refused charge must not debit. + wallet_resp = await client.get( + f"/v1/billing/wallets/{agent_wallet_id}", headers=api_headers + ) + assert Decimal(wallet_resp.json()["balance_exact"]) == Decimal("5000") + + +def test_register_local_refuses_a_nonpositive_price() -> None: + """A governed tool priced at or below zero is a money bug, not a free tier. + + ``credits_per_unit`` is multiplied by units and debited, so zero makes a + metered tool free and a negative value pays the caller on every invoke. + Registration is the last place to catch it before it reaches a ledger. + """ + + from app.schemas.billing import ServiceCategory + from app.services.service_registry import get_service_registry + + registry = get_service_registry() + + def handler(value: str) -> str: + return value + + for price in (0, -1.5): + with pytest.raises(ValueError, match="credits_per_unit"): + registry.register_local( + service_id=f"bad-price-{price}", + name="Bad price", + description="Should never register", + category=ServiceCategory.AGENT_COMMS, + func=handler, + credits_per_unit=price, + ) + + +def test_register_local_derives_an_input_schema_alongside_an_output_model() -> None: + """Supplying only an output model must not erase the input contract. + + The callable fallback used to run only when *both* schemas were absent, so + a handler with plain typed arguments plus an output model advertised + ``inputSchema: {}``. A client that obeyed that contract called the tool + with no arguments and got a TypeError. + """ + + from pydantic import BaseModel + + from app.schemas.billing import ServiceCategory + from app.services.service_registry import get_service_registry + + class Out(BaseModel): + echoed: str + + registry = get_service_registry() + + def handler(value: str) -> Out: + return Out(echoed=value) + + record = registry.register_local( + service_id="schema-fallback-probe", + name="Schema fallback probe", + description="Input schema must still be derived from the callable", + category=ServiceCategory.AGENT_COMMS, + func=handler, + output_model=Out, + ) + + input_schema = record["input_schema"] + assert input_schema, "input schema was dropped because an output model was given" + assert "value" in (input_schema.get("properties") or {}), ( + f"the handler's own parameter is missing from {input_schema!r}" + ) From d93c38bee0228d9b7e78d23499a2b49ce3fa0989 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 02:47:28 +0000 Subject: [PATCH 2/2] fix(registry): drop the credits_per_unit floor; zero is a supported price I added a credits_per_unit > 0 guard on the theory that a zero price makes a metered tool free by accident. The existing suite proves the opposite: test_discovery_annotations_carry_governance_contract registers a tool at 0.0 specifically to assert the manifest reports economicAction: false, so a free tool is a deliberate, tested product state -- not a mistake to refuse. test_public_manifest_stays_strict_json_for_malformed_prices makes the second half of the argument: it registers nan, inf, -inf and 0.0 on purpose and asserts the public manifest still serialises as strict JSON. The design hardens prices where they are rendered, not where they are registered, and raising at registration prevented those cases from being set up at all. Three tests failed for these reasons. Reverting the guard rather than editing them: the tests were right. The independent input/output schema fallback in the same function is kept -- nothing objected to it, and supplying only an output model still must not erase a handler's input contract. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m --- app/services/service_registry.py | 10 ---------- tests/test_billing.py | 28 ---------------------------- 2 files changed, 38 deletions(-) diff --git a/app/services/service_registry.py b/app/services/service_registry.py index 3eb7d531..481a34a6 100644 --- a/app/services/service_registry.py +++ b/app/services/service_registry.py @@ -256,16 +256,6 @@ def register_local( requires a signed permit + idempotency key — even if legacy unpermitted MCP is otherwise allowed. """ - if credits_per_unit <= 0: - # A price is what the meter charges. Zero makes a governed tool - # free; negative pays the caller on every invoke, because the - # billing engine multiplies units by this number and debits the - # result. Refuse at registration rather than discovering it in a - # ledger. - raise ValueError( - f"credits_per_unit must be greater than 0, got {credits_per_unit!r}" - ) - input_schema = pydantic_to_mcp_schema(input_model) output_schema = pydantic_to_mcp_schema(output_model) diff --git a/tests/test_billing.py b/tests/test_billing.py index e7f6276e..23e46bec 100644 --- a/tests/test_billing.py +++ b/tests/test_billing.py @@ -2270,34 +2270,6 @@ async def test_zero_daily_limit_blocks_spending_rather_than_unlocking_it( assert Decimal(wallet_resp.json()["balance_exact"]) == Decimal("5000") -def test_register_local_refuses_a_nonpositive_price() -> None: - """A governed tool priced at or below zero is a money bug, not a free tier. - - ``credits_per_unit`` is multiplied by units and debited, so zero makes a - metered tool free and a negative value pays the caller on every invoke. - Registration is the last place to catch it before it reaches a ledger. - """ - - from app.schemas.billing import ServiceCategory - from app.services.service_registry import get_service_registry - - registry = get_service_registry() - - def handler(value: str) -> str: - return value - - for price in (0, -1.5): - with pytest.raises(ValueError, match="credits_per_unit"): - registry.register_local( - service_id=f"bad-price-{price}", - name="Bad price", - description="Should never register", - category=ServiceCategory.AGENT_COMMS, - func=handler, - credits_per_unit=price, - ) - - def test_register_local_derives_an_input_schema_alongside_an_output_model() -> None: """Supplying only an output model must not erase the input contract.