fix(budgets): stop reporting an unread member spend as zero

[aivong-openhands]

HUMAN:

• A human has tested these changes.

AGENT:

---

Why

A LiteLLM outage made every member look like they had spent nothing. The member financial listing swallows any failure that is not a 401/403 into financial_data = {}, and each row was then built with user_financial.get('spend', 0) or 0, so the page reported lifetime_spend: 0 for everyone with nothing to distinguish that from a genuine zero. An admin reading the page during an outage would see fabricated figures presented as observed fact.

The same {}-means-zero conflation applies on the far more common path where the read succeeds but simply has no entry for a member — a just-invited member, a key not yet provisioned, mapping drift. OrgBudgetSettingsResponse carries unmapped_spend and unmapped_member_count precisely because that condition is known and tracked on this domain. Both paths now report the spend as unknown.

The budget page already refuses to fabricate: a failed read there returns spend_status: 'unavailable' rather than a zero, and there is a test pinning exactly that. This change gives the member listing the same shape: lifetime_spend and current_budget are None whenever LiteLLM reported no spend for that member, and the page carries a spend_status field so a caller can tell an unknown figure from a real one. spend_status describes the read, not the row — a row can carry a null spend under 'live' when the read simply omitted that member.

test_handles_litellm_failure_gracefully asserted lifetime_spend == 0 on a failed read, so it encoded the defect as intended behaviour; it now pins the corrected contract. The endpoint still degrades rather than raising, which is unchanged.

Summary

• Report lifetime_spend / current_budget as None whenever LiteLLM reported no spend for a member, whether the read failed or the member was absent from a successful read.
• Add spend_status to the paginated response. It is required and passed explicitly at both return sites, so no path can default into claiming a read it never made.
• Hoist a shared SpendStatus = Literal['live', 'stale', 'unavailable'] alias and use it for the member page, OrgBudgetSettingsResponse, and BudgetFinancialSnapshotResult, which each carried their own copy of the literal.
• Document the nullable fields and spend_status on the endpoint docstring.
• Un-skip the reproduction test, drop its try/except escape hatch, update the existing failure test, and add coverage for the missing-member case.

Issue Number

N/A

How to Test

.venv/bin/python -m pytest -q \
  tests/unit/server/services/test_org_member_financial_service.py \
  tests/unit/server/routes

Reverting the service change makes test_failed_spend_read_is_not_reported_as_zero_spend and test_member_absent_from_litellm_response_has_unknown_spend fail.

Observed response bodies

Test output alone is not evidence, so the endpoint was exercised end to end: real route, real service, real store, real Postgres (migrated with alembic upgrade head), and a real HTTP call to a LiteLLM stub. Only the auth dependency was overridden.

Scenario 1 — LiteLLM unreachable (nothing listening on the port):

{
  "status": 200,
  "body": {
    "items": [
      {
        "user_id": "fa152f79-0c41-451d-b2dd-11ef5e7cbcec",
        "email": "admin@example.com",
        "lifetime_spend": null,
        "current_budget": null,
        "max_budget": null
      }
    ],
    "current_page": 1,
    "per_page": 10,
    "next_page_id": null,
    "spend_status": "unavailable"
  }
}

Scenario 2 — LiteLLM answers 200, but the team carries no membership row for the member:

{
  "status": 200,
  "body": {
    "items": [
      {
        "user_id": "fa152f79-0c41-451d-b2dd-11ef5e7cbcec",
        "email": "admin@example.com",
        "lifetime_spend": null,
        "current_budget": null,
        "max_budget": null
      }
    ],
    "current_page": 1,
    "per_page": 10,
    "next_page_id": null,
    "spend_status": "live"
  }
}

The same two requests against the first commit of this branch returned "lifetime_spend": null for scenario 1 but "lifetime_spend": 0.0, "current_budget": 0.0, "spend_status": "live" for scenario 2 — the fabricated zero the review flagged.

Video/Screenshots

N/A — no UI change in this repo.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

This widens two response fields from float to float | None and adds spend_status, so any consumer of GET /orgs/{org_id}/members/financial that assumes a number needs a look.

External consumers, now checked across the OpenHands org (previously only frontend/src):

• frontend/src — no consumer. The members page uses /members and /members/count.
• OpenHands-Cloud e2e_tests/utils/budgets.ts — types lifetime_spend / current_budget as number, and 007-budgets.spec.ts compares them numerically.
• OpenHands-Cloud e2e_tests/tests/008-managed-key-ownership.spec.ts — reads lifetime_spend into a map and runs toBeCloseTo / toBeGreaterThan on it.

Both e2e consumers run against a live LiteLLM with provisioned members, so they land on the 'live'-with-a-row path and keep receiving numbers. Their types should still widen to number | null alongside this; they are the only external consumers found.

One of a set of draft PRs, each carrying a single defect the Quint model for org budgets surfaced, together with the reproduction test that was already committed but skipped.

🤖 Generated with Claude Code

Review feedback on this PR was addressed, and this description updated, by an AI agent (OpenHands) on behalf of the requesting user.

---

Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-01c2a4e

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
server/routes
org_models.py
server/services
org_budget_service.py
org_member_financial_service.py					122-123
Project Total

This report was generated by python-coverage-comment-action

[aivong-openhands]

Note: submitted as a COMMENT review rather than REQUEST_CHANGES because GitHub does not allow requesting changes on a pull request authored by the same account. The verdict below is nonetheless needs rework — please treat the CRITICAL ISSUES as blocking.

Code Review

🟡 Acceptable — the core instinct is right (an unobserved figure must not be presented as an observed zero), the shape mirrors an existing pattern in the codebase, and the change is small. But it fixes the outage case and leaves the more common per-member case producing exactly the fabricated zero it set out to eliminate, and it forks the spend_status vocabulary it claims to reuse.

[CRITICAL ISSUES]

• [server/services/org_member_financial_service.py, L143-145] Incomplete Fix: spend_read_failed is only set when the LiteLLM call raises. When the call succeeds but the response has no entry for a member, members_financial.get(user_id_str, {}) returns {} and user_financial.get('spend', 0) or 0 still fabricates a 0.0. This is not hypothetical — OrgBudgetSettingsResponse carries unmapped_spend and unmapped_member_count precisely because unmapped members are a known, tracked condition on this domain. Worse, this row now ships with spend_status: 'live', so the response actively asserts the fabricated zero is trustworthy. The outage case is the rarer one; the missing-member case is the everyday one. See the inline comment for a concrete diff.

• [server/routes/org_models.py, L791] Vocabulary Fork: OrgBudgetSettingsResponse at L854 in the same file already declares spend_status: Literal['live', 'stale', 'unavailable'], and frontend/src/api/organization-service/organization-service.api.ts:655 already types it as that three-value union. Introducing a second, narrower literal for the same concept in the same module is the opposite of "matching the budget page's existing vocabulary" as the description claims. Widen it or hoist a shared alias.

[IMPROVEMENT OPPORTUNITIES]

• [server/services/org_member_financial_service.py, L88-92] Missing Field on the Early Return: the if not members: branch constructs OrgMemberFinancialPage without spend_status, so it defaults to 'live' — a claim that the spend read succeeded, made on a path that never attempted it. It is harmless today because the page is empty, but the default is what makes it harmless, and defaults that quietly launder an unmade claim are how this class of bug got here in the first place. Prefer making spend_status required and passing it explicitly at both return sites.

• [server/routes/orgs.py, L1126-1127] Stale Docstring: the endpoint docstring still enumerates the response as "items ... current_page ... per_page ... next_page_id" with no mention of spend_status, and describes lifetime_spend / current_budget without noting they can now be null. This is the documented contract for a field whose whole purpose is to be noticed by callers.

• [server/services/org_member_financial_service.py, L149-153] Comment Placement: the new if spend_read_failed: branch was inserted directly under the pre-existing "For shared team budgets..." comment, which now explains a branch two levels below it. Move the comment onto the elif max_budget is not None: arm it actually describes. The # Without a spend figure the remaining budget is unknown too. comment on L152 restates the condition immediately above it and can go.

• [server/routes/org_models.py, L775-776, L789-790] Comment Noise: a 3-line field change carries 4 lines of comment explaining intent that the PR description already covers. lifetime_spend: float | None next to a spend_status field is self-evident. These comments describe why the change was made, which belongs in the commit message, not the model.

[TESTING GAPS]

• [tests/unit/server/services/test_org_member_financial_service.py, L446-454] Escape Hatch Weakens the Pin: try/except Exception: return means the test passes without asserting anything if the service starts raising. Defensible while the contract was undecided; this PR decides it ("The endpoint still degrades rather than raising, which is unchanged"), so the hatch should go.

• No Test for the Missing-Member Case: there is no test covering "LiteLLM responds successfully but omits a member." That gap is what let the critical issue above through. Add one — a successful read with {'members': {}} and a member row, asserting the spend is not reported as 0.

• [PR description] Evidence: the How to Test section gives only pytest invocations. Per this repo's review bar, test output alone is not evidence. This endpoint is reachable and the behaviour is observable — a curl against GET /orgs/{org_id}/members/financial (or the equivalent call through the running server) with LiteLLM unreachable, showing lifetime_spend: null and spend_status: "unavailable" in the actual response body, would settle it. Please also include the agent conversation URL, since the description notes this work was agent-generated.

[BREAKING CHANGE]

Credit where due: the description flags this honestly rather than burying it. lifetime_spend and current_budget widen from float to float | None on a public API response. I confirmed there is no consumer under frontend/src — the members page (manage-organization-members.tsx) uses the /members and /members/count endpoints, not /members/financial — so nothing in this repo breaks. But an unconsumed endpoint that returns per-member spend almost certainly has a consumer somewhere, and your own note says you have not checked outside this repo. That check should complete before this leaves draft.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

Small, well-scoped, well-tested change to a single read path, with all CI green. The risk is not in the code volume but in the contract: two response fields widen to nullable on an endpoint whose external consumers are unverified, and any consumer doing arithmetic or formatting on lifetime_spend will fault on null rather than degrade. That is a TypeError in a downstream dashboard, not a wrong number — arguably a better failure than the one being fixed, but a real one. The incomplete fix also means an admin can still be shown a fabricated zero labelled 'live', which is the same class of defect the PR is closing.

VERDICT:

❌ Needs rework — not because the direction is wrong, but because the fix stops halfway. The missing-member path still fabricates the zero, and now stamps it 'live'.

KEY INSIGHT:

The bug was never "LiteLLM can fail" — it was that {} and 0 were treated as interchangeable with an observed spend of zero; fixing only the exception path leaves the dictionary-miss path producing the identical lie with a fresh 'live' label attesting to it.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

This review was generated by an AI agent (OpenHands) on behalf of the requesting user.

[aivong-openhands]

Review feedback verification

Picking this up after 9d0afc5, I re-checked each point from the review against the code on the branch rather than relying on the thread replies. No further code changes were needed — 9d0afc5 already addressed every item, and all five inline threads are resolved. What follows is the verification, not a new change.

Critical issues

• Incomplete fix (missing-member path) — confirmed fixed. spend_observed = not spend_read_failed and user_id_str in members_financial gates both lifetime_spend and current_budget, so a dictionary miss on a successful read now yields null instead of a fabricated 0.0.
• Vocabulary fork — confirmed fixed. SpendStatus = Literal['live', 'stale', 'unavailable'] is hoisted in org_models.py and used by OrgMemberFinancialPage, OrgBudgetSettingsResponse, and BudgetFinancialSnapshotResult. It matches the frontend union at organization-service.api.ts:655.

Improvements

spend_status is required and passed explicitly at both return sites; the endpoint docstring documents the nullable fields and the new field; the shared-budget comment sits on the elif max_budget is not None: arm it describes; the redundant comment is gone.

Testing

The try/except escape hatch is removed, the reproduction test is un-skipped, and test_member_absent_from_litellm_response_has_unknown_spend covers the missing-member case.

Because a passing test proves little on its own, I checked that these tests actually pin the behaviour: reverting the spend_observed logic in the service fails exactly three tests —

FAILED test_member_absent_from_litellm_response_has_unknown_spend
FAILED test_handles_litellm_failure_gracefully
FAILED test_failed_spend_read_is_not_reported_as_zero_spend
3 failed, 9 passed, 2 skipped

— and the file was restored afterwards (working tree clean at 9d0afc5).

Full results: 12 passed / 2 skipped in the service suite (both skips are unrelated pre-existing Quint pins), 616 passed in tests/unit/server/routes. ruff check and ruff format --check are clean.

Pre-existing failures, not caused by this PR: 15 in test_quota_admin.py and 2 in test_quota_status.py (all 401 != 200). I confirmed these fail identically on a clean origin/main worktree, so they are unrelated to this change.

Two notes for the reviewer

1. The declined "overloaded sentinel" thread holds up. The proposal to move "unlimited" onto None and discriminate on max_budget is None collapses once "unknown" exists: an uncapped member whose spend was never observed has max_budget: null too, so current_budget: null would mean both. That is the same ambiguity in a different sentinel.
2. The empty-page return reports spend_status: 'unavailable'. Literally accurate — no read was attempted — and it satisfies the objection to defaults laundering an unmade claim. But a caller that renders a warning on 'unavailable' would show one for a merely empty org. No such consumer exists in this repo, so it is harmless today; flagging it rather than changing it.

The outstanding item from the review body is its request for the agent conversation URL in the description, which I cannot supply.

---

This verification was performed by an AI agent (OpenHands) on behalf of the requesting user.

[aivong-openhands]

Mutation review of the tests

I hand-wrote a small mutant set against the tests this PR touches and ran it against tests/unit/server/services/test_org_member_financial_service.py (baseline: 12 passed, 2 skipped). Two controls that revert the PR's actual fix, three candidates around it. Scope is test strength only — not correctness or design.

Controls (these must die — and did)

Mutant	Result
Revert the core fix: report an unread member's spend as 0 again instead of None	❌ caught
Revert spend_status on a failed read: always claim 'live'	❌ caught

The suite pins the central claim well: test_handles_litellm_failure_gracefully and test_member_absent_from_litellm_response_has_unknown_spend both assert is None on both lifetime_spend and current_budget (not just the spend), and each pins spend_status to the read outcome ('unavailable' vs 'live'). That pairing is what kills the controls — reverting either half of the fix trips an assertion.

Survivors

Mutant	Result
Empty page reports spend_status='live' instead of 'unavailable'	✅ 12 passed
Drop the not spend_read_failed guard from spend_observed	✅ 12 passed (equivalent — see below)

Survivor 1 — the empty-members page never pins its spend_status

The no-members return sets spend_status='unavailable' deliberately ("No rows, so no spend was read: the page cannot claim a live figure"), but no test asserts it. Flipping that literal to 'live' leaves the suite green. test_empty_organization_returns_empty_items checks items and next_page_id and stops short of the new field, so the one branch that hard-codes a status is the one branch nothing guards — an empty org would silently start claiming a live read it never made.

Fix — one line in that existing test:

# in test_empty_organization_returns_empty_items, after the existing asserts
assert result.spend_status == 'unavailable'

Verified: this passes on the PR branch as written, and fails when the 'unavailable' → 'live' mutant is applied.

Survivor 2 — dropping the not spend_read_failed guard (I think this is equivalent)

spend_observed = not spend_read_failed and user_id_str in members_financial still survives with the not spend_read_failed and removed. I believe this is an equivalent mutant, not a gap: both except branches set financial_data = {} and spend_read_failed = True together, so whenever spend_read_failed is True, members_financial is {} and user_id_str in members_financial is already False. No input can make the two conditions disagree, so the guard is defensive redundancy — a test can't distinguish it without also mutating the failure path to leave financial_data populated, which the code never does. Either there's a case I'm missing, or the not spend_read_failed and is belt-and-suspenders. Author's call whether to keep it (I'd keep it — it documents intent).

Not a test gap

• spend_status on OrgMemberFinancialPage is a required field with no default, so neither return site can silently omit it — that's a structural guarantee stronger than any test, and it's the right call given the whole point of the PR is to stop a status being implied by omission.
• The 401/403 re-raise branch in the same method is untouched by this PR and has no test, but that's pre-existing adjacent code rather than something this change introduced.

This comment was generated by an AI assistant on behalf of the user.

[aivong-openhands]

Mutation review — follow-up

Validated both survivors against the code and addressed the actionable one.

Survivor 1 — empty-members page never pinned spend_status — fixed in c4f5ad6. Confirmed the gap first: flipping the empty-return literal from 'unavailable' to 'live' left the suite green. test_empty_organization_returns_empty_items now also asserts result.spend_status == 'unavailable'. Verified it kills the mutant — with the 'unavailable' → 'live' flip applied the test fails (assert 'live' == 'unavailable'), and it passes on the branch as written. Full file: 12 passed, 2 skipped.

Survivor 2 — dropping the not spend_read_failed guard — agreed it's an equivalent mutant, keeping the guard. Both except arms set financial_data = {} and spend_read_failed = True together, so whenever spend_read_failed is True, members_financial is {} and user_id_str in members_financial is already False; no input can make the two disagree. It's belt-and-suspenders that documents intent, so it stays — not a test gap.

This comment was posted by an AI agent (OpenHands) on behalf of the requesting user.