test(proxy): phase-4 payload behavior pinning for tier-2/3 key + team management endpoints#28681
Merged
yuneng-berri merged 3 commits intoMay 23, 2026
Conversation
… management endpoints Extends the Phase 1–3 behavior-pin suite at tests/proxy_behavior/management/ with a second axis: payload-shape pinning. Phase 1–3 held payload minimal and pinned (actor, target) → status across 37 routes; Phase 4 holds the caller fixed at an authorized actor, varies the payload shape, and asserts the observable DB effect (on accept) or the named guard / row-unchanged (on reject). Faithfulness contract from Phase 1–3 is unchanged. Six families + one gap-closer (59 new scenarios, 620 → 679 total): * F1 — key budget / rate-limit (test_key_budget_limits.py, 18) * F2 — key↔team reassignment (test_key_team_change.py, 6) * F3 — team budget / rate-limit (test_team_budget_limits.py, 15) * F4 — member-info validation (test_team_member_info_validation.py, 5) * F5 — permission batching (test_team_permissions_bulk_update.py, 6) * F6 — org-scoped team access (+2 detail-string pins in existing files) * F7 — coverage gap-closer (test_f7_coverage_closeout.py, 7) Harness extensions in conftest.py (additive only): * create_scratch_org() seeder with its own scratch-prefixed budget row * budget / limit fields on create_scratch_team() * scratch teardown also sweeps litellm_organizationtable Coverage telemetry (behavior-suite-only): * key_management_endpoints.py 60 % → 65 % (+82 lines) * team_endpoints.py 62 % → 72 % (+137 lines, crosses 70 % stretch) Key lands under 70 % per plan §7 escape hatch — the gap is dominated by routes outside F1–F6 scope (key list/info v2 internals) and structurally dead org-budget guards (call sites at lines 889 + 2310 + 985 + 1751 load the org without include_budget_table=True, so org.litellm_budget_table is None at guard time and the aggregate guard no-ops). Pinned as observed no-op behavior so a future fix that flips the flag turns these into reds. Zero source-code changes; pyproject.toml diff is empty; test_route_coverage.py stays green untouched; G3 grep guards still green; local wall-time 14 s for the full suite (no coverage), 22 s with coverage. G4 regression-replay protocol executed against three representative fix-PR parents (410ce76, 0bd49ec, 8bbc61e): all Phase 4 tests PASS at pre-fix SHAs — confirming the F1–F7 layer is a helper-body pin, not a regression-replay layer for those specific historical bypass shapes. Targeted RED-bait scenarios for each fix are left for a follow-up PR.
…7-extension) Adds 24 more payload-pin scenarios in test_f7_key_coverage_push.py following the same accepted-effect / rejected-guard pattern. Each scenario cites the file:line range it pins; same anti-snapshot rules apply. Target ranges (all reachable via HTTP-boundary payload variation): * 5942-6063 /key/health with metadata.logging → test_key_logging body * 4565-4692 /key/reset_spend happy + 404 + non-admin gate + value validation * 4421-4533 /key/regenerate ghost-404 + happy + new_key + grace_period * 4168-4202 _insert_deprecated_key body via grace_period * 6118-6133 _enforce_unique_key_alias duplicate-alias rejection * 6148-6169 validate_model_max_budget malformed-payload rejection * 4708-4789 validate_key_list_check user/team/org/key_hash branches * 2622-2733 /key/bulk_update mixed success/failure + admin gate + size limits * 2797-2950 /team/key/bulk_update all-keys path + explicit-keys dedupe + 404 * 5108-5207 /key/aliases admin + scoped + search-filter branches * 3253-3303 /key/info ghost + explicit-key + no-key-uses-auth-header * 3427-3436 generate_key_helper_fn budget_limits initialization * 1794-1815 prepare_key_update_data duration + budget_duration paths * 5280-5388 _build_filter_conditions across include_created_by_keys/team/sort/alias Coverage telemetry — full PR4 dataset: key_management_endpoints.py: 60 % → 71 % (+11 pts, +194 lines) team_endpoints.py: 62 % → 72 % (+10 pts, +137 lines) Both files now over the plan §7 PR4.M4 70 % stretch as a side effect of pinning real payload behavior. 721 tests pass in 19 s local (full suite, no coverage); 27 s with coverage. Zero source-code changes; pyproject.toml diff still empty; test_route_coverage.py + G3 grep guards still green. Honest finding (kept from the prior commit's body): four structurally-dead org-budget guards remain pinned as observed no-op behavior — they fire only when get_org_object is called with include_budget_table=True, which none of the four management-endpoint call sites currently do. Pinned so a future change that flips the flag turns these into reds. Two helper guards are honest-ceiling: _validate_reset_spend_value's isinstance check at line 4568 is unreachable from HTTP because Pydantic 422s non-float before the helper runs; same shape for /team/key/bulk_update's missing team_id / no-selector pre-handler guards.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Greptile SummaryThis PR adds 101 new behavior-pinning test scenarios (620 \u2192 721 total) across 8 new and 2 extended files in
Confidence Score: 4/5Test-only PR; no production code is touched. The budget-row leakage on assertion failure in two closeout tests is the main concern — orphaned rows accumulate across CI re-runs but do not affect production. All changes are confined to the test suite and harness helpers. The budget row cleanup pattern in test_f7_coverage_closeout.py places teardown after assertions without a try/finally, leaving non-scratch-prefixed rows in the DB whenever an assertion fails mid-test. The two 500-pinning tests could suppress detection of error-handling improvements in the future. tests/proxy_behavior/management/test_f7_coverage_closeout.py — the two team-member-budget tests need cleanup wrapped in try/finally. tests/proxy_behavior/management/test_f7_key_coverage_push.py — the two 500-asserting tests and the missing Optional annotations.
|
| Filename | Overview |
|---|---|
| tests/proxy_behavior/management/conftest.py | Additive harness extensions: create_scratch_team gains budget/limit kwargs, new create_scratch_org seeds org + linked budget row, scratch teardown sweeps orgs before budgets (correct FK order). Minor comment typo. |
| tests/proxy_behavior/management/test_f7_coverage_closeout.py | Seven gap-closing scenarios. Two tests perform manual cleanup of non-scratch-prefixed budget rows after assertions; a failing assertion leaves those rows permanently leaked. |
| tests/proxy_behavior/management/test_f7_key_coverage_push.py | 42 scenarios targeting key management endpoints. Two tests pin 500 as expected status, which would flag legitimate error-handling improvements as regressions. _seed_token has missing Optional annotations. |
| tests/proxy_behavior/management/test_key_budget_limits.py | 18 scenarios for key budget/rate-limit helpers; documents org-level aggregate checks as structurally unenforced and pins the no-op as an explicit tripwire. |
| tests/proxy_behavior/management/test_team_budget_limits.py | 15 scenarios for team budget enforcement via /team/new and /team/update; covers all four _check_user_team_limits caps with accepted and rejected cases. |
| tests/proxy_behavior/management/test_key_team_change.py | Six scenarios pinning validate_key_team_change; rejected cases re-read DB row to assert team_id unchanged — correct IDOR regression shape. |
| tests/proxy_behavior/management/test_team_member_info_validation.py | Five scenarios for _validate_and_populate_member_user_info; accepted cases re-read members_with_roles to confirm resolved user_id. |
| tests/proxy_behavior/management/test_team_permissions_bulk_update.py | Six scenarios for /team/permissions_bulk_update; apply_to_all_teams path snapshots and restores world teams in try/finally, correctly preventing state leakage. |
| tests/proxy_behavior/management/test_team_info.py | Additive: one new test pins the exact _verify_team_access 403 detail string for cross-org rejection. |
| tests/proxy_behavior/management/test_team_update.py | Additive: one new test pins the _verify_team_access 403 detail string for org-admin cross-org relocation rejection. |
Reviews (1): Last reviewed commit: "test(proxy): push key_management_endpoin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+199
to
+253
| ) -> str: | ||
| cleartext = "sk-" + uuid.uuid4().hex | ||
| await prisma.db.litellm_verificationtoken.create( | ||
| data={ | ||
| "token": hash_token(cleartext), | ||
| "key_alias": f"{scratch_prefix}-key", | ||
| "key_name": f"{scratch_prefix}-key", | ||
| "user_id": user_id, | ||
| "team_id": team_id, | ||
| "models": [], | ||
| } | ||
| ) | ||
| return cleartext | ||
|
|
||
|
|
||
| async def test_team_new_with_team_member_budget_creates_budget_row( | ||
| proxy_client, prisma, scratch, world | ||
| ): | ||
| """/team/new with `team_member_budget` routes through | ||
| TeamMemberBudgetHandler.create_team_member_budget_table (lines 196–248). | ||
| Observable end-state: a litellm_budgettable row is created and the | ||
| team's metadata.team_member_budget_id points at it.""" | ||
| seeder = world.keys[Actor.PROXY_ADMIN].cleartext | ||
| team_id = scratch.tag("team-with-mbudget") | ||
| resp = await proxy_client.post( | ||
| "/team/new", | ||
| headers={"Authorization": f"Bearer {seeder}"}, | ||
| json={ | ||
| "team_id": team_id, | ||
| "team_alias": scratch.tag("alias"), | ||
| "team_member_budget": 10.0, | ||
| "team_member_rpm_limit": 100, | ||
| "team_member_tpm_limit": 1000, | ||
| }, | ||
| ) | ||
| assert resp.status_code == 200, resp.text | ||
| team_row = await prisma.db.litellm_teamtable.find_unique(where={"team_id": team_id}) | ||
| assert team_row is not None | ||
| budget_id = (team_row.metadata or {}).get("team_member_budget_id") | ||
| assert ( | ||
| budget_id is not None | ||
| ), f"team_member_budget_id not stamped on team metadata: {team_row.metadata!r}" | ||
| budget_row = await prisma.db.litellm_budgettable.find_unique( | ||
| where={"budget_id": budget_id} | ||
| ) | ||
| assert budget_row is not None, "team_member_budget row was not created" | ||
| assert budget_row.max_budget == 10.0 | ||
| assert budget_row.rpm_limit == 100 | ||
| assert budget_row.tpm_limit == 1000 | ||
| # Manual cleanup: this budget row was created by the handler under a | ||
| # non-scratch budget_id pattern (`team-<alias>-budget-<uuid>`), so the | ||
| # scratch teardown's prefix sweep won't reclaim it. | ||
| await prisma.db.litellm_teamtable.update( | ||
| where={"team_id": team_id}, | ||
| data={"metadata": Json({})}, |
Contributor
There was a problem hiding this comment.
Both test_team_new_with_team_member_budget_creates_budget_row and test_team_update_team_member_budget_upserts perform manual cleanup of budget rows whose IDs don't share the scratch prefix (format team-<alias>-budget-<uuid>). The cleanup calls are placed after the assertions with no try/finally guard. If any assertion fires before the cleanup executes, the orphaned budget row persists across the test run, accumulating with every CI re-run. The scratch teardown sweeps by prefix so these rows are never reclaimed. Wrapping the assertion + cleanup block in try/finally ensures the delete always runs.
Comment on lines
+113
to
+115
| # The outer handler wraps the inner ValueError as a 500. | ||
| assert resp.status_code == 500, resp.text | ||
| assert "callback_name" in resp.text |
Contributor
There was a problem hiding this comment.
500 makes any fix that turns this into a proper 400 look like a regression. The outer try/except wrapping an inner ValueError is an implementation detail that can change. Consider in (400, 500) to avoid a false red when the error handling improves.
Suggested change
| # The outer handler wraps the inner ValueError as a 500. | |
| assert resp.status_code == 500, resp.text | |
| assert "callback_name" in resp.text | |
| # The outer handler currently wraps the inner ValueError as a 500. | |
| assert resp.status_code in (400, 500), resp.text | |
| assert "callback_name" in resp.text |
Comment on lines
+374
to
+378
| # validate_model_max_budget raises ValueError, which the outer handler | ||
| # wraps as a 500. Pin the observed shape — the helper's exception path | ||
| # is the regression-risk surface, not the status code envelope. | ||
| assert resp.status_code == 500, resp.text | ||
| assert "Invalid model_max_budget" in resp.text, resp.text |
Contributor
There was a problem hiding this comment.
500 as the expected status code locks in server-error behavior as a hard contract. Any future fix that properly converts the ValueError into a 400/422 will trip this test as a regression even though the behavior improved. Consider relaxing to in (400, 422, 500) and keeping the string assertion.
Suggested change
| # validate_model_max_budget raises ValueError, which the outer handler | |
| # wraps as a 500. Pin the observed shape — the helper's exception path | |
| # is the regression-risk surface, not the status code envelope. | |
| assert resp.status_code == 500, resp.text | |
| assert "Invalid model_max_budget" in resp.text, resp.text | |
| # validate_model_max_budget raises ValueError, which the outer handler | |
| # wraps as a 500. Pin the observed shape — the helper's exception path | |
| # is the regression-risk surface, not the status code envelope. | |
| assert resp.status_code in (400, 422, 500), resp.text | |
| assert "Invalid model_max_budget" in resp.text, resp.text |
Comment on lines
+40
to
+43
| spend: float = 0.0, | ||
| max_budget: float = None, | ||
| metadata: dict = None, | ||
| team_id: str = None, |
Contributor
There was a problem hiding this comment.
max_budget parameter is annotated as float but defaults to None, which is only valid as Optional[float]. Mypy and strict type checkers will flag this as a type error.
Suggested change
| spend: float = 0.0, | |
| max_budget: float = None, | |
| metadata: dict = None, | |
| team_id: str = None, | |
| spend: float = 0.0, | |
| max_budget: Optional[float] = None, | |
| metadata: Optional[dict] = None, | |
| team_id: Optional[str] = None, |
Comment on lines
+337
to
+340
| # F1+F3 seed scratch orgs via create_scratch_org; the world seeder is | ||
| # the only other writer of LiteLLM_OrganizationTable and uses the | ||
| # behavior-pin- prefix, so a scratch-prefixed sweep here cannot | ||
| # collide with the read-world. Org must be reclaimed BEFORE its |
Contributor
There was a problem hiding this comment.
Suggested change
| # F1+F3 seed scratch orgs via create_scratch_org; the world seeder is | |
| # the only other writer of LiteLLM_OrganizationTable and uses the | |
| # behavior-pin- prefix, so a scratch-prefixed sweep here cannot | |
| # collide with the read-world. Org must be reclaimed BEFORE its | |
| # F1+F3 seed scratch orgs via create_scratch_org; the world seeder is | |
| # the only other writer of LiteLLM_OrganizationTable and uses the | |
| # behavior-pin- prefix, so a scratch-prefixed sweep here cannot | |
| # collide with the real-world. Org must be reclaimed BEFORE its |
…elope pins + Optional annotations Greptile review feedback on PR #28681: 1. Wrap manual budget-row cleanup in try/finally so an assertion failure doesn't leave non-scratch-prefixed budget rows orphaned across CI re-runs (test_team_new_with_team_member_budget_creates_budget_row and test_team_update_team_member_budget_upserts). 2. Loosen the two 500-status pins to in (400, 422, 500) — the named-guard substring is the real pin; the outer ValueError-wrap envelope is an implementation detail that a future improvement should be free to fix to a proper 400/422 without flipping these tests red. 3. Add missing Optional annotations on _seed_token's max_budget / metadata / team_id keyword args (they default to None). Greptile's typo flag on 'read-world' in the conftest comment is declined — 'read-world' is the project's established term for the immutable seeded world fixture (see other usages in conftest.py and actors.py). 721 tests still pass in 17 s.
ryan-crabbe-berri
approved these changes
May 23, 2026
yuneng-berri
merged commit 5e16f20
into
litellm_internal_staging
108 of 116 checks passed
fzowl
pushed a commit
to fzowl/litellm
that referenced
this pull request
Jun 24, 2026
… management endpoints (BerriAI#28681) * test(proxy): phase-4 payload behavior pinning for tier-2/3 key + team management endpoints Extends the Phase 1–3 behavior-pin suite at tests/proxy_behavior/management/ with a second axis: payload-shape pinning. Phase 1–3 held payload minimal and pinned (actor, target) → status across 37 routes; Phase 4 holds the caller fixed at an authorized actor, varies the payload shape, and asserts the observable DB effect (on accept) or the named guard / row-unchanged (on reject). Faithfulness contract from Phase 1–3 is unchanged. Six families + one gap-closer (59 new scenarios, 620 → 679 total): * F1 — key budget / rate-limit (test_key_budget_limits.py, 18) * F2 — key↔team reassignment (test_key_team_change.py, 6) * F3 — team budget / rate-limit (test_team_budget_limits.py, 15) * F4 — member-info validation (test_team_member_info_validation.py, 5) * F5 — permission batching (test_team_permissions_bulk_update.py, 6) * F6 — org-scoped team access (+2 detail-string pins in existing files) * F7 — coverage gap-closer (test_f7_coverage_closeout.py, 7) Harness extensions in conftest.py (additive only): * create_scratch_org() seeder with its own scratch-prefixed budget row * budget / limit fields on create_scratch_team() * scratch teardown also sweeps litellm_organizationtable Coverage telemetry (behavior-suite-only): * key_management_endpoints.py 60 % → 65 % (+82 lines) * team_endpoints.py 62 % → 72 % (+137 lines, crosses 70 % stretch) Key lands under 70 % per plan §7 escape hatch — the gap is dominated by routes outside F1–F6 scope (key list/info v2 internals) and structurally dead org-budget guards (call sites at lines 889 + 2310 + 985 + 1751 load the org without include_budget_table=True, so org.litellm_budget_table is None at guard time and the aggregate guard no-ops). Pinned as observed no-op behavior so a future fix that flips the flag turns these into reds. Zero source-code changes; pyproject.toml diff is empty; test_route_coverage.py stays green untouched; G3 grep guards still green; local wall-time 14 s for the full suite (no coverage), 22 s with coverage. G4 regression-replay protocol executed against three representative fix-PR parents (410ce76, 0bd49ec, 8bbc61e): all Phase 4 tests PASS at pre-fix SHAs — confirming the F1–F7 layer is a helper-body pin, not a regression-replay layer for those specific historical bypass shapes. Targeted RED-bait scenarios for each fix are left for a follow-up PR. * test(proxy): push key_management_endpoints.py past the 70% stretch (F7-extension) Adds 24 more payload-pin scenarios in test_f7_key_coverage_push.py following the same accepted-effect / rejected-guard pattern. Each scenario cites the file:line range it pins; same anti-snapshot rules apply. Target ranges (all reachable via HTTP-boundary payload variation): * 5942-6063 /key/health with metadata.logging → test_key_logging body * 4565-4692 /key/reset_spend happy + 404 + non-admin gate + value validation * 4421-4533 /key/regenerate ghost-404 + happy + new_key + grace_period * 4168-4202 _insert_deprecated_key body via grace_period * 6118-6133 _enforce_unique_key_alias duplicate-alias rejection * 6148-6169 validate_model_max_budget malformed-payload rejection * 4708-4789 validate_key_list_check user/team/org/key_hash branches * 2622-2733 /key/bulk_update mixed success/failure + admin gate + size limits * 2797-2950 /team/key/bulk_update all-keys path + explicit-keys dedupe + 404 * 5108-5207 /key/aliases admin + scoped + search-filter branches * 3253-3303 /key/info ghost + explicit-key + no-key-uses-auth-header * 3427-3436 generate_key_helper_fn budget_limits initialization * 1794-1815 prepare_key_update_data duration + budget_duration paths * 5280-5388 _build_filter_conditions across include_created_by_keys/team/sort/alias Coverage telemetry — full PR4 dataset: key_management_endpoints.py: 60 % → 71 % (+11 pts, +194 lines) team_endpoints.py: 62 % → 72 % (+10 pts, +137 lines) Both files now over the plan §7 PR4.M4 70 % stretch as a side effect of pinning real payload behavior. 721 tests pass in 19 s local (full suite, no coverage); 27 s with coverage. Zero source-code changes; pyproject.toml diff still empty; test_route_coverage.py + G3 grep guards still green. Honest finding (kept from the prior commit's body): four structurally-dead org-budget guards remain pinned as observed no-op behavior — they fire only when get_org_object is called with include_budget_table=True, which none of the four management-endpoint call sites currently do. Pinned so a future change that flips the flag turns these into reds. Two helper guards are honest-ceiling: _validate_reset_spend_value's isinstance check at line 4568 is unreachable from HTTP because Pydantic 422s non-float before the helper runs; same shape for /team/key/bulk_update's missing team_id / no-selector pre-handler guards. * test(proxy): address PR review — try/finally cleanup + loosen 500 envelope pins + Optional annotations Greptile review feedback on PR BerriAI#28681: 1. Wrap manual budget-row cleanup in try/finally so an assertion failure doesn't leave non-scratch-prefixed budget rows orphaned across CI re-runs (test_team_new_with_team_member_budget_creates_budget_row and test_team_update_team_member_budget_upserts). 2. Loosen the two 500-status pins to in (400, 422, 500) — the named-guard substring is the real pin; the outer ValueError-wrap envelope is an implementation detail that a future improvement should be free to fix to a proper 400/422 without flipping these tests red. 3. Add missing Optional annotations on _seed_token's max_budget / metadata / team_id keyword args (they default to None). Greptile's typo flag on 'read-world' in the conftest comment is declined — 'read-world' is the project's established term for the immutable seeded world fixture (see other usages in conftest.py and actors.py). 721 tests still pass in 17 s.
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
2 participants
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.
Adds 101 new behavior-pin tests in
tests/proxy_behavior/management/(620 → 721). Zero source changes.New test files
test_key_budget_limits.py_check_key_model_specific_limits,_check_key_rpm_tpm_limits,_check_team_key_limits,_check_org_key_limits(18 scenarios)test_key_team_change.pyvalidate_key_team_change— model / tpm / rpm / membership / initiator branches (6)test_team_budget_limits.py_check_org_team_limits+_check_user_team_limits(15)test_team_member_info_validation.py_validate_and_populate_member_user_info(5)test_team_permissions_bulk_update.py/team/permissions_bulk_update— specific / all-teams / unknown-id (6)test_f7_coverage_closeout.pytest_f7_key_coverage_push.py/key/health,/key/regenerate,/key/reset_spend,/key/bulk_update,/team/key/bulk_update,/key/aliases,/key/info,/key/listfilter branches (42)Modified files
conftest.py— newcreate_scratch_orgseeder;create_scratch_teamgainsmax_budget/tpm_limit/rpm_limit/metadatakwargs;scratchteardown also sweepslitellm_organizationtable(before its budget, per FK).test_team_info.py,test_team_update.py— +1 each:_verify_team_accessrejection-detail pin.Coverage (behavior-suite-only)
key_management_endpoints.pyteam_endpoints.pyTest plan
Local wall-time: 19 s without coverage, 27 s with.
test_route_coverage.pygreen untouched;pyproject.tomldiff empty.