fix(budgets): never write a member cap below their cycle baseline

[aivong-openhands]

HUMAN:

• A human has tested these changes.

AGENT:

---

Why

A negative per-user override locked a member out of the product for the rest of the cycle without their having spent anything. LiteLLM compares cumulative spend against an absolute member cap, so the sync writes baseline + allowance. Only the route's Pydantic model rejects a non-positive monthly_limit; upsert_user_override and the store take whatever they are handed, and the column has no CHECK constraint. With a negative allowance the cap landed below the member's cycle baseline — a cap already exceeded the moment it was written.

The cap is computed in two places from the same inputs: _sync_litellm_budgets writes it, and _budget_policy_comparison expects to read it back when detecting drift. Clamping only the write site would make the two disagree, so a negative override would leave the org permanently degraded — an HTTP 503 on the budgets routes with no self-service recovery. This clamps the allowance at zero in a single _member_cap helper that both sites call, so the two can never diverge. No allowance now means no further spend this cycle rather than retroactive debt. The same clamp covers a negative default_user_monthly_limit, which reaches the same arithmetic by a different route.

Summary

• Add _member_cap(baseline, effective_limit), which clamps the allowance at zero, and call it from both _sync_litellm_budgets and _budget_policy_comparison.
• Un-skip the reproduction test, which drives a -100 override, asserts every written cap is at or above the baseline, and asserts the org still reconciles healthy afterwards.

Issue Number

N/A

How to Test

.venv/bin/python -m pytest -q tests/unit/test_org_budget_service.py

Expect 41 passed, 6 skipped. Reverting the clamp at either call site makes test_override_cap_is_never_written_below_the_cycle_baseline fail: at the write site with a cap of baseline - 100 handed to LiteLLM, at the comparison site with the org stuck at reconciliation_state == 'degraded'.

Video/Screenshots

N/A — no UI change.

Type

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

Notes

Clamping is the narrow fix: it stops the bad cap reaching LiteLLM but still stores the nonsense override. Rejecting a non-positive monthly_limit in upsert_user_override and adding a CHECK constraint to the column would stop the row existing at all, which is the fuller fix and a separate change — it needs a migration and a backfill for any rows that already violate it, and the clamp is still wanted as defence-in-depth for those. The reproduction test deliberately asserts the cap rather than a rejection, so both remain open.

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

---

Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-54b1ce5

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
server/services
org_budget_service.py					340, 1397
Project Total

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

[aivong-openhands]

🔴 Taste Rating: Needs improvement

The diagnosis is right and the one-character-ish fix does stop the bad cap reaching LiteLLM. But clamping at the write site while leaving the verification site unclamped splits one truth into two, and the result is a permanently degraded org.

[CRITICAL ISSUES]

• [server/services/org_budget_service.py:1390] Data Structure — the clamp is applied at one of two sites that must agree. The expected member cap is computed in two places from the same inputs:

  • _sync_litellm_budgets line 1390 (this PR): baseline + max(effective_limit, 0) — what gets written.
  • _budget_policy_comparison line 329 (untouched): baseline + effective_limit — what drift detection expects to read back.

  With the -100 override the test drives, against baseline=100:

  written to LiteLLM : 100.0
  drift expects      : 0.0
  match (1e-6)       : False
  

  So _budget_sync_readback_errors emits member_budget_mismatch, policy_matches goes False, and _budget_policy_comparison returns reconciliation_state='degraded'. That state is not cosmetic — get_org_budget_settings and upsert_org_budget_override both turn degraded into an HTTP 503 (server/routes/orgs.py:1286, :1322), and the frontend surfaces the org as broken. The org cannot get out of it: every subsequent sync writes the clamped value and every subsequent readback expects the unclamped one, forever, until someone edits the row by hand.

  Note the readback loop inside _sync_litellm_budgets uses the local expected_member_budgets (which is clamped, line 1395), so the sync itself reports success. It is the separate comparison in _budget_policy_comparison that disagrees. That split is what makes this hard to notice and is the actual defect: two functions independently reimplement "the cap this member should have."

  The fix is to compute the cap once. Extract the clamp into the existing _effective_user_budget_limit (which already owns "what limit applies to this member") or into a small _member_cap(baseline, effective_limit) helper, and call it from both sites. Then the two can never drift apart, which is the property you want — not two max() calls that someone has to keep in sync.

• [tests/unit/test_org_budget_service.py] Testing gap that hid the above. test_override_cap_is_never_written_below_the_cycle_baseline asserts only on update_user.await_args_list — the caps handed to LiteLLM. It never inspects reconciliation_state, so the degraded-forever consequence is invisible to it. The test passes and the org is broken. Please add an assertion that the org reconciles to healthy (or at least not degraded) after a clamped override, which is the assertion that would have caught this.

[IMPROVEMENT OPPORTUNITIES]

• [server/services/org_budget_service.py:1383-1389] Unnecessary comments: five lines of comment for a max(x, 0). Lines 2-5 restate the PR description ("such a cap is already exceeded the moment it is written... no allowance means no further spend this cycle, not retroactive debt"). The original single line — "LiteLLM compares cumulative spend against an absolute member cap" — was the genuinely non-obvious fact and was sufficient; if the clamp moves into a named helper as suggested above, the name carries the rest.

• Pragmatism: the PR notes that the fuller fix is rejecting a non-positive monthly_limit in upsert_user_override plus a CHECK constraint, and defers it. I think that ordering is backwards for this particular defect. The clamp preserves a nonsense row in the database and then papers over it on every read forever; validation deletes the problem. The clamp is defensible as defence-in-depth alongside validation, but on its own it is the more complex of the two options and the one that just produced a second bug. Worth reconsidering which fix ships first.

[RISK ASSESSMENT]

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

Confined to org budget sync, no schema change, no API surface change. The realistic trigger — a negative override — requires bypassing the route's Pydantic gt=0, so the population at risk is small. But when it does trigger, the outcome is an org stuck at HTTP 503 on its budgets page with no self-service recovery, which is worse for that org than the original bug (one member over-restricted). CI is green, which is precisely the problem: the test suite does not check the state this change breaks.

Recommendation: Do not merge until the clamp is applied consistently at both computation sites. This is a small change, but the failure it introduces is silent and unrecoverable without manual intervention.

VERDICT:
❌ Needs rework: Clamp once, in one place, and assert the org still reconciles healthy afterwards.

KEY INSIGHT:
When the same quantity is computed in two places, fixing one is not a fix — it is a divergence, and here that divergence is exactly the drift detector those two sites exist to power.

---

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 @aivong-openhands.

[aivong-openhands]

Thanks — the critical issue is real and I reproduced it before changing anything. With baseline=100 and a -100 override, _budget_policy_comparison returned:

degraded | False | member_budget_mismatch: <uid>: expected=0.0 actual=100.0 uses_shared_budget=False

So the clamp at the write site alone did leave the org permanently degraded, which is a 503 on the budgets routes with no self-service recovery. Addressed in 54b1ce5.

Clamp once. Extracted _member_cap(baseline, effective_limit) and call it from both _sync_litellm_budgets and _budget_policy_comparison. I put it in its own helper rather than inside _effective_user_budget_limit, because that function returns the allowance (and is also used by the user-budget rows and the membership-repair path, where the absolute cap is not what is wanted); the cap is a different quantity and now has exactly one definition.

Test gap. test_override_cap_is_never_written_below_the_cycle_baseline now also drives get_budget_state after the override and asserts reconciliation_state == 'healthy'. I verified it fails on the divergence: reverting just the _budget_policy_comparison call site (leaving the write-site clamp in place) makes the test fail, so it pins the property rather than passing incidentally.

Comments. Dropped the five-line block. The helper name carries the intent and the remaining comment is just the non-obvious LiteLLM fact.

On validation vs clamping. Fair point, and I agree validation is the better primary fix — but I'd rather not fold it into this PR. The clamp is still needed regardless, because rows that predate validation already exist and a CHECK constraint would fail to apply until they are cleaned up; with the cap computed in one place it is now cheap defence-in-depth rather than a second source of truth. Rejecting a non-positive monthly_limit in upsert_user_override plus the CHECK constraint and a backfill is a separate change with a migration, and this PR's reproduction test deliberately asserts on the cap rather than a rejection, so it stays green either way.

tests/unit/test_org_budget_service.py is 41 passed, 6 skipped; the other four budget test files are 24 passed. Pre-commit is clean.

---

This comment was created by an AI agent (OpenHands) on behalf of @aivong-openhands.

[aivong-openhands]

Mutation review of the tests

I mutation-tested this PR's tests: break the behaviour on purpose and see whether the suite notices. Baseline on the branch is 41 passed, 6 skipped for tests/unit/test_org_budget_service.py (≈8s), plus 17 passed for tests/unit/test_org_budget_preflight.py when a mutant reached that module. 12 hand-written mutants; the interesting ones are below.

Controls — the PR's own claim is pinned

Mutant	Result
Revert the clamp at the write site (max_budget_in_team = baseline + effective_limit)	❌ caught
Revert the clamp at the drift-comparison site (expected_member_budgets[user_id] = baseline + effective_limit)	❌ caught
_member_cap uses abs(effective_limit) instead of clamping at zero, turning -100 into a +100 allowance	❌ caught
Anchor the comparison's member cap to the team cycle_start_spend instead of the member's own baseline	❌ caught
Write the clamped cap with clear_budget=True, so LiteLLM drops it	❌ caught
Clamp floor applied to the cap rather than the allowance: max(baseline + effective_limit, 0)	❌ caught
_member_cap floor raised from 0 to 1000 — consistent at both sites, just wrong	❌ caught
Drop the readback drift errors at the end of _sync_litellm_budgets	❌ caught

Both halves of the two-site claim are genuinely pinned, and what makes that work is driving the second snapshot rather than asserting on the first. Replacing the single return_value with a side_effect that serves before then after means the test exercises the real readback-and-compare path, so assert state['reconciliation_state'] == 'healthy' catches the comparison site independently of min(written_caps) >= baseline catching the write site. A single-snapshot mock would have left the second call site free. The clear_budget=False kwarg travelling on the same update_user_in_team call is what kills the "write the cap but tell LiteLLM to clear it" mutant — asserting on max_budget alone would have missed it.

Survivors

Mutant	Result
M10 — preflight keeps the unclamped baselines[user_id] + effective_limit	✅ 58 passed
M2 — clamp applies only when a per-user override row exists; a negative default_user_monthly_limit is written through	✅ 41 passed
M7 — a zero allowance is treated as falsy at both sites, so a clamped member falls back to the shared team budget	✅ 41 passed

---

M10 — org_budget_preflight still computes the unclamped cap, and it is blocking

This one is not really a mutant: it is the current state of server/services/org_budget_preflight.py:232. That module says in its own docstring that _sync_litellm_budgets "remains the source of truth for the cap formulas; this module mirrors them read-only". After this PR they no longer match — _sync_litellm_budgets clamps, the preflight does not — and the preflight feeds _budget_sync_readback_errors with the unclamped expectation:

desired:   {<user>: -92.0}
cap_drift: ['member_budget_mismatch: <user>: expected=-92.0 actual=8.0 uses_shared_budget=False']
blocking:  True

That is the preflight reporting cap_drift at blocking severity against the exact cap the clamped sync just wrote. cap_drift is SEVERITY_BLOCKING, so the post-upgrade gate in strict mode fails an upgrade of an org that holds a negative override — the same org this PR is meant to rescue. It is the mirror image of the reasoning in the PR description for clamping both service sites: clamping only some of the places that compute the cap makes them disagree.

The fix is the same one the PR applies to the other two sites:

from server.services.org_budget_service import (
    LiteLlmFinancialSnapshot,
    _budget_sync_readback_errors,
    _effective_user_budget_limit,
    _member_cap,
)

...
                if is_disabled or effective_limit is None:
                    desired_members[user_id] = None
                else:
                    desired_members[user_id] = _member_cap(
                        baselines[user_id], effective_limit
                    )

and a test in tests/unit/test_org_budget_preflight.py that pins the third site to the same formula:

def test_preflight_desired_member_cap_is_clamped_like_the_sync():
    # The preflight mirrors the sync's cap formula read-only; if it keeps the
    # unclamped one, the post-upgrade gate reports cap_drift as BLOCKING against
    # the very caps the sync just wrote, and the upgrade fails in strict mode.
    user_id = str(uuid4())
    settings = _settings(user_cycle_start_spend={user_id: 8.0})
    overrides = [
        OrgUserBudgetOverride(user_id=user_id, monthly_limit=-100.0, is_disabled=False)
    ]
    snapshot = _snapshot(members={user_id: (8.0, 8.0, False)})

    entry = _evaluate(settings, {user_id}, snapshot, overrides=overrides)

    assert entry['desired']['members'] == {user_id: 8.0}
    assert entry['cap_drift'] == []
    assert entry['blocking'] is False

Verified: this test fails on the branch as it stands (assert {<user>: -92.0} == {<user>: 8.0}), passes with the _member_cap call added, and fails again when the preflight is reverted to the unclamped formula.

M2 and M7 — the default-limit route, and the clamp's own output value

The PR description says "the same clamp covers a negative default_user_monthly_limit, which reaches the same arithmetic by a different route", but no test drives that route: every assertion goes through upsert_user_override. A clamp made conditional on override is not None at both sites keeps the whole suite green (M2), so nothing stops the org-wide default from regressing separately from the per-user override.

M7 is the more subtle one. The clamp's output for a negative allowance is effective_limit == 0, and zero is falsy. Change either site's effective_limit is not None to a truthiness check and the member silently returns to the shared team budget instead of being capped — no cap written, no drift reported, suite green. That is the opposite of the PR's intent: "no allowance now means no further spend this cycle" becomes "no allowance means spend against the org's pool". It is a one-character edit away and nothing pins it.

One parametrized test covers both, driving _sync_litellm_budgets directly with the default limit rather than an override:

@pytest.mark.asyncio
@pytest.mark.parametrize('limit', [-100.0, 0.0])
async def test_negative_org_default_limit_caps_members_at_their_baseline(
    async_session_maker, budget_org, limit
):
    # A non-positive default_user_monthly_limit reaches the same arithmetic by a
    # different route than an override, and zero is the clamp's own output: a
    # falsy-vs-None check anywhere on that path silently returns the member to the
    # shared team budget.
    user_id = uuid4()
    baseline = 100.0
    async with async_session_maker() as session:
        session.add_all(
            [
                Role(id=1, name='member', rank=1),
                User(id=user_id, current_org_id=budget_org.id),
                OrgMember(
                    org_id=budget_org.id,
                    user_id=user_id,
                    role_id=1,
                    llm_api_key='test-api-key',
                    status='active',
                ),
                OrgBudgetSettings(
                    org_id=budget_org.id,
                    enabled=True,
                    reset_day=1,
                    monthly_limit=250.0,
                    default_user_monthly_limit=limit,
                    cycle_start_at=datetime.now(UTC),
                    cycle_start_spend=baseline,
                    user_cycle_start_spend={str(user_id): baseline},
                    litellm_known_member_ids=[str(user_id)],
                ),
            ]
        )
        await session.commit()

        service = OrgBudgetService(session)
        settings = await service._get_or_create_settings(budget_org.id)
        overrides = await service._get_overrides(budget_org.id)
        before = _snapshot(
            team_spend=baseline, members={str(user_id): (baseline, None, True)}
        )
        after = _snapshot(
            team_spend=baseline,
            team_max_budget=baseline + 250.0,
            members={str(user_id): (baseline, baseline, False)},
        )
        snapshots = [before, after]
        with (
            patch.object(
                service,
                '_get_financial_snapshot',
                AsyncMock(
                    side_effect=lambda *args, **kwargs: BudgetFinancialSnapshotResult(
                        snapshot=snapshots.pop(0) if snapshots else after,
                        status='live',
                    )
                ),
            ),
            patch(
                'server.services.org_budget_service.LiteLlmManager.update_team',
                AsyncMock(),
            ),
            patch(
                'server.services.org_budget_service.LiteLlmManager.update_user_in_team',
                AsyncMock(),
            ) as update_user,
        ):
            await service._sync_litellm_budgets(budget_org.id, settings, overrides)
            state = await service.get_budget_state(budget_org.id)

    call = update_user.await_args_list[-1]
    # No allowance means no further spend this cycle: a private cap at the
    # baseline, not a fall-through to the shared team budget.
    assert call.kwargs['max_budget'] == baseline
    assert call.kwargs['clear_budget'] is False
    assert state['reconciliation_state'] == 'healthy'

Verified: passes on the branch unmodified, and fails with M2 and with M7 applied. The clear_budget is False assertion is what does the work for M7 — asserting only on max_budget would not distinguish "capped at the baseline" from "no cap written at all".

Not a test gap

• A mutant that makes is_disabled fall through to the clamped cap survives, but it is an equivalent mutant: _effective_user_budget_limit returns (None, True, True) for a disabled override, so effective_limit is always None when is_disabled is true and both branches land in the same else. Not worth a test.
• The PR's own note already flags that the clamp leaves the nonsense override stored. Agreed that the column CHECK constraint plus rejection in upsert_user_override is a separate change; worth noting that with all three cap sites clamped, the constraint is the only thing that would make the stored value and the enforced value agree.

---

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