This repository was archived by the owner on Oct 24, 2026. It is now read-only.
Repository navigation
fix(billing,registry): treat zero as a value on money controls, not as absence - #321
Merged
PetrefiedThunder merged 2 commits intoAug 20, 2026
Merged
Conversation
…s 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe billing changes preserve explicit zero values for daily limits and low-balance thresholds. The service registry derives missing schemas independently. Regression tests cover both behaviors. ChangesBilling zero-value handling
Local service schema derivation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…rice 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m
This branch was successfully deployed
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
A fresh max-effort review of the money path (
billing_engine,refund_reconciliation,permits,service_registry) surfaced two falsy-zero defects on spend controls plus a schema-registration gap. Each is confirmed by a test that fails without the change.daily_limit = 0meant "unlimited" instead of "spend nothing." The cap isOptional, whereNonemeans "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:338Decimal(...) if request.daily_limit else None0becameNoneat the API boundarybilling_engine.py:374if wallet.daily_limit and ...0skipped the only hard daily-spend checkFixing only the engine would have changed nothing, because the zero never reached it — the new test is what proved that. The HTTP schema declares
ge=0, so zero is documented input, andapp/db/converters.pyalready drew the distinction correctly withis not None.auto_refill_threshold = 0meant "alert below 100." The column is non-Optional defaulting to100, soor Decimal("100")could only ever fire on exactly zero — the one value a wallet sets to mean "never alert, never refill" — and silently restored the default it had opted out of, inserting aLOW_BALANCEalert on every subsequent charge.register_localdropped the input schema when 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 advertisedinputSchema: {}— and every client that obeyed that contract called it with no arguments and hit aTypeError. Each side now falls back independently.Proposed and withdrawn: a
credits_per_unit > 0floorThe first commit added one, on the theory that a zero price makes a metered tool free by accident. The existing suite proved that wrong and it was reverted in d93c38b.
test_discovery_annotations_carry_governance_contractregisters a tool at0.0specifically to assert the manifest reportseconomicAction: false, so a free tool is a deliberate, tested product state.test_public_manifest_stays_strict_json_for_malformed_pricesthen registersnan,inf,-inf, and0.0on purpose: this codebase 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; the tests were right. Recorded here rather than silently dropped, since the reasoning is the useful part.Reported, not fixed here
Each needs its own change, so none are bundled:
authorize_and_reserve(permits.py:474) — holds a pooled connection and a permit row lock whileverify_signaturechecks out a second connection from the same 15-connection pool. Enough concurrent governed invokes and every request stalls forpool_timeoutand fails. Invisible under SQLite tests.max_calls_per_tool/aggregate_value_capread outside the atomic reservation (permits.py:685) — absent from the guarded UPDATE'sWHERE, so two simultaneous invokes both pass a one-call permit. Contradicts the module header, which states no cap decision may come from a number read in an earlier statement.IntegrityErrorreplay path (billing_engine.py:699) —reverse_velocity_recordlives inside the transaction that rolled back, so a raced retry leaves the wallet permanently over-counted, which eventually auto-freezes it.create_permitprefers the request-bodysubject_key_id(permits.py:324) over the authenticated one — the exact opposite of the comment above it. Either the code or the comment is wrong; which one depends on whether issuing a permit for another agent's key is a supported flow.Type of Change
Validation
pytest -q— two new cases intests/test_billing.py, each verified to fail without its fix (the zero-daily-limit case failed with200 != 402before the router fix; the schema case failed with the fix reverted).tests/test_tool_interface_authority.pyis 33/33 after the withdrawal above. A full-suite run is in progress and CI covers the same on this head; twotests/test_railway_preflight.pyfailures seen in an earlier local run were artifacts of the working tree changing mid-run — that file is 77/77 standalone.ruff check— clean onapp/and the test filepython scripts/export_openapi.py --check— n/a, no API surface change (behavior only, within the schema's existingge=0)python scripts/generate_sim_inventory.py --check— n/a, no simulator changePOST /v1/billing/wallets/agentwithdaily_limit=0, thenPOST /v1/billing/charge, asserting402and an undebited balanceSECURITY_LIMITATIONS.mdRisk
Notes on migration, rollout, or rollback concerns: no migration and no schema change. Behavior changes only for values that were already documented as valid and were being silently discarded — a wallet created with
daily_limit=0now actually blocks spending, andauto_refill_threshold=0now actually silences low-balance alerts. Existing wallets withNonelimits are unaffected.🤖 Generated with Claude Code
https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m