diff --git a/.github/assets/roi-calculator/21-empty-repository-preserved-report.png b/.github/assets/roi-calculator/21-empty-repository-preserved-report.png new file mode 100644 index 00000000000..4c6add87f95 Binary files /dev/null and b/.github/assets/roi-calculator/21-empty-repository-preserved-report.png differ diff --git a/.github/assets/roi-calculator/22-partial-calculation-explanation.png b/.github/assets/roi-calculator/22-partial-calculation-explanation.png new file mode 100644 index 00000000000..5415956b3fa Binary files /dev/null and b/.github/assets/roi-calculator/22-partial-calculation-explanation.png differ diff --git a/.github/assets/roi-calculator/23-estimator-outage-preserved-report.png b/.github/assets/roi-calculator/23-estimator-outage-preserved-report.png new file mode 100644 index 00000000000..346cc2acab7 Binary files /dev/null and b/.github/assets/roi-calculator/23-estimator-outage-preserved-report.png differ diff --git a/litellm/proxy/roi_calculator/github.py b/litellm/proxy/roi_calculator/github.py index 6fa2ce24d63..997be03cdd4 100644 --- a/litellm/proxy/roi_calculator/github.py +++ b/litellm/proxy/roi_calculator/github.py @@ -315,7 +315,7 @@ class GitHub: ) -> None: if client is not None and transport is not None: raise ValueError("Pass either an injected GitHub client or a transport.") - self._profiles: Mapping[str, str] = MappingProxyType({}) + self._profiles: Mapping[str, str | None] = MappingProxyType({}) token: Final = settings.github_token.get_secret_value() self._headers: Final[Mapping[str, str]] = ( MappingProxyType( @@ -494,25 +494,26 @@ class GitHub: } return evidence - async def profile_email(self, login: str) -> str: + async def profile_email(self, login: str, *, fallback: str = "") -> str: if login.casefold() in self._profiles: - return self._profiles[login.casefold()] + cached: Final = self._profiles[login.casefold()] + return cached if cached is not None else fallback address: Final = await self._load_profile_email(login) self._profiles = MappingProxyType({**self._profiles, login.casefold(): address}) - return address + return address if address is not None else fallback - async def _load_profile_email(self, login: str) -> str: + async def _load_profile_email(self, login: str) -> str | None: try: response: Final = await self.client.get( self._url(f"users/{quote(login, safe='')}"), headers=self._headers, ) if response.status_code != 200: - return "" + return None profile: Final = _GitHubUserProfile.model_validate(response.json()) return normalize_email(profile.email) except (httpx.HTTPError, ValueError): - return "" + return None async def _commit_metadata( self, repo: str, number: int, detail: _PullDetail diff --git a/litellm/proxy/roi_calculator/sync.py b/litellm/proxy/roi_calculator/sync.py index 93614c33da0..7bb47ec8e88 100644 --- a/litellm/proxy/roi_calculator/sync.py +++ b/litellm/proxy/roi_calculator/sync.py @@ -274,6 +274,11 @@ async def _read_repositories(github: GitHub, repos: tuple[str, ...], start: date "check repository access or try analysis again later." ) queue: Final = tuple(chain.from_iterable(((group.repo, pull) for pull in group.pulls) for group in groups)) + if unavailable and not queue: + raise SourceError( + f"GitHub could not read {', '.join(unavailable)}, and the accessible repositories returned no pull requests. " + "No new report was published; check repository access or try analysis again later." + ) warnings: Final = ( ( ( @@ -298,6 +303,13 @@ def _processed_records(processed: tuple[_ProcessedPull, ...]) -> Mapping[int, RO raise SourceError( "GitHub could not provide PR metadata. No new report was published; try analysis again later." ) + if any(item.record["estimate"]["status"] == "error" for item in processed) and not any( + item.record["estimate"]["status"] == "estimated" for item in processed + ): + raise SourceError( + "The estimator could not score any pull requests. No new report was published; " + "check the estimator connection or try analysis again later." + ) return MappingProxyType({item.position: item.record for item in processed}) @@ -469,7 +481,9 @@ class SyncManager: and cached_pull["estimate"]["status"] == "estimated" and "commit_emails" in cached_pull ): - profile: Final = await github.profile_email(cached_pull["login"]) + profile: Final = await github.profile_email( + cached_pull["login"], fallback=cached_pull.get("profile_email", "") + ) cached_record: Final = TypeAdapter(ROIPullRecord).validate_python( MappingProxyType( { diff --git a/litellm/types/roi_calculator.py b/litellm/types/roi_calculator.py index 2e3aa8cf49a..a15bcbdac9b 100644 --- a/litellm/types/roi_calculator.py +++ b/litellm/types/roi_calculator.py @@ -11,6 +11,15 @@ DEFAULT_PROMPT: Final = ( ) +def _normalize_login(value: str) -> str: + import re + + login: Final = value.strip().casefold() + if re.fullmatch(r"[A-Za-z0-9_\[\]-]+", login) is None: + raise ValueError("Enter a valid GitHub username.") + return login + + class ROISettings(BaseModel): model_config = ConfigDict(frozen=True) @@ -81,15 +90,13 @@ class ROISettings(BaseModel): @field_validator("identity_map") @classmethod def normalize_identity_map(cls, values: Mapping[str, str]) -> Mapping[str, str]: - import re - from litellm.proxy.roi_calculator.analytics import normalize_email normalized: Final[Mapping[str, str]] = MappingProxyType( { - login.strip().casefold(): normalize_email(address) + _normalize_login(login): normalize_email(address) for login, address in values.items() - if re.fullmatch(r"[A-Za-z0-9_\[\]-]+", login.strip()) is not None and normalize_email(address) + if normalize_email(address) } ) if len(normalized) != len(values): @@ -421,6 +428,11 @@ class ROIIdentityMapUpdate(BaseModel): github_login: str email: str | None + @field_validator("github_login") + @classmethod + def normalize_login(cls, value: str) -> str: + return _normalize_login(value) + class ROIIdentityMapResponse(BaseModel): report: ROISummaryResponse | None diff --git a/tests/unit/proxy/management_endpoints/test_roi_calculator_endpoints.py b/tests/unit/proxy/management_endpoints/test_roi_calculator_endpoints.py index df3a6256965..66b9df69996 100644 --- a/tests/unit/proxy/management_endpoints/test_roi_calculator_endpoints.py +++ b/tests/unit/proxy/management_endpoints/test_roi_calculator_endpoints.py @@ -177,6 +177,16 @@ def test_all_writes_require_full_admin(role: LitellmUserRoles, method: str, path assert client.request(method, path, json=body).status_code == 403 +@pytest.mark.parametrize("login", ("invalid.name", " ", "user/name")) +@pytest.mark.parametrize("email", ("alice@example.com", None)) +def test_invalid_identity_login_returns_validation_error(login: str, email: str | None) -> None: + repository: Final = _ConfigRepository() + client: Final = _client(LitellmUserRoles.PROXY_ADMIN, repository) + response: Final = client.put("/roi-calculator/identity-map", json={"github_login": login, "email": email}) + assert response.status_code == 422 + assert not repository.values + + def test_schedule_and_estimator_key_persist_without_exposing_secrets(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("LITELLM_SALT_KEY", "roi-calculator-test-salt-key-0123456789") repository: Final = _ConfigRepository() diff --git a/tests/unit/proxy/roi_calculator/test_sync.py b/tests/unit/proxy/roi_calculator/test_sync.py index a1f98caf0c5..3547b503484 100644 --- a/tests/unit/proxy/roi_calculator/test_sync.py +++ b/tests/unit/proxy/roi_calculator/test_sync.py @@ -460,7 +460,9 @@ async def test_one_unreadable_pr_preserves_other_estimates_in_report() -> None: assert manager.status.needs_attention == 1 -def _repository_outage_transport(status: int, *, all_unavailable: bool = False) -> httpx.MockTransport: +def _repository_outage_transport( + status: int, *, all_unavailable: bool = False, healthy_empty: bool = False +) -> httpx.MockTransport: baseline: Final = _transport() def respond(request: httpx.Request) -> httpx.Response: @@ -468,6 +470,8 @@ def _repository_outage_transport(status: int, *, all_unavailable: bool = False) return httpx.Response(status, json=[] if status == 200 else {"message": "Repository unavailable"}) if all_unavailable and request.url.path.endswith("/pulls"): return httpx.Response(status) + if healthy_empty and request.url.path == "/repos/org/repo/pulls": + return httpx.Response(200, json=[]) return baseline.handle_request(request) return httpx.MockTransport(respond) @@ -512,7 +516,8 @@ async def test_unavailable_repository_publishes_flagged_partial_report_and_recov @pytest.mark.asyncio -async def test_all_repository_outage_preserves_previous_report() -> None: +@pytest.mark.parametrize("all_unavailable", (True, False)) +async def test_repository_outage_without_usable_pulls_preserves_previous_report(all_unavailable: bool) -> None: repository: Final = _ReportRepository() manager: Final = SyncManager(clock=_fixed_now) settings: Final = _settings().model_copy(update=MappingProxyType({"repos": ("org/repo", "org/unavailable")})) @@ -521,9 +526,73 @@ async def test_all_repository_outage_preserves_previous_report() -> None: previous: Final = repository.values["roi_calculator_report"] assert await manager.start( - settings, repository, _spend_reader(), _completion(), _repository_outage_transport(403, all_unavailable=True) + settings, + repository, + _spend_reader(), + _completion(), + _repository_outage_transport(403, all_unavailable=all_unavailable, healthy_empty=not all_unavailable), ) await _wait_until_finished(manager) assert manager.status.phase == "error" assert manager.status.error is not None and "No new report was published" in manager.status.error assert repository.values["roi_calculator_report"] == previous + + +@pytest.mark.asyncio +@pytest.mark.parametrize("profile_status", (200, 403, 429, 503)) +async def test_reused_profile_preserves_email_only_when_lookup_fails(profile_status: int) -> None: + repository: Final = _ReportRepository() + manager: Final = SyncManager(clock=_fixed_now) + baseline: Final = _transport() + + def respond(request: httpx.Request) -> httpx.Response: + if request.url.path.endswith("/commits"): + return httpx.Response(200, content=_COMMITS_JSON.replace("alice@example.com", "")) + return baseline.handle_request(request) + + assert await manager.start(_settings(), repository, _spend_reader(), _completion(), httpx.MockTransport(respond)) + await _wait_until_finished(manager) + + def refreshed(request: httpx.Request) -> httpx.Response: + if request.url.path == "/users/alice": + return httpx.Response(profile_status, json={"email": None}) + return baseline.handle_request(request) + + async def unexpected_completion(request: ROICompletionRequest) -> object: + raise AssertionError("A reused estimate must not call the estimator") + + assert await manager.start( + _settings(), repository, _spend_reader(), unexpected_completion, httpx.MockTransport(refreshed) + ) + await _wait_until_finished(manager) + report: Final = TypeAdapter(ROIReport).validate_python(repository.values["roi_calculator_report"]) + expected: Final = "" if profile_status == 200 else "alice@example.com" + assert manager.status.phase == "complete" + assert manager.status.reused == 1 + assert report["pulls"][0]["profile_email"] == expected + assert report["pulls"][0]["emails"] == ((expected,) if expected else ()) + assert summarize(report, MappingProxyType({}))["metrics"]["cost_per_hour"] == (None if profile_status == 200 else 3) + + +@pytest.mark.asyncio +async def test_complete_estimator_outage_preserves_report_and_recovers() -> None: + repository: Final = _ReportRepository() + manager: Final = SyncManager(clock=_fixed_now) + assert await manager.start(_settings(), repository, _spend_reader(), _completion(), _transport()) + await _wait_until_finished(manager) + previous: Final = repository.values["roi_calculator_report"] + changed: Final = _settings(estimator_prompt="Updated estimation instructions") + + async def failed_completion(request: ROICompletionRequest) -> object: + raise httpx.ConnectError("Estimator unavailable") + + assert await manager.start(changed, repository, _spend_reader(), failed_completion, _transport()) + await _wait_until_finished(manager) + assert manager.status.phase == "error" + assert manager.status.error is not None and "No new report was published" in manager.status.error + assert repository.values["roi_calculator_report"] == previous + assert await manager.start(changed, repository, _spend_reader(), _completion(), _transport()) + await _wait_until_finished(manager) + assert manager.status.phase == "complete" + recovered: Final = TypeAdapter(ROIReport).validate_python(repository.values["roi_calculator_report"]) + assert recovered["pulls"][0]["estimate"]["hours"] == 4 diff --git a/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorView.integration.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorView.integration.test.tsx index 49d0d03df85..3e71e9fb860 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorView.integration.test.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorView.integration.test.tsx @@ -179,6 +179,10 @@ describe("ROICalculatorView", () => { expect(await screen.findByRole("alert")).toHaveTextContent(warning); expect(screen.getByRole("button", { name: "Open estimate for org/repo pull request 42" })).toBeInTheDocument(); expect(screen.queryByText("$3.00")).not.toBeInTheDocument(); + fireEvent.click(screen.getByText("Calculation details")); + expect( + screen.getByText("Spend per estimated hour is unavailable until all selected repositories can be read."), + ).toBeVisible(); }); it("lets a view-only admin read the report without write controls", async () => { diff --git a/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorViews.tsx b/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorViews.tsx index 75df18752de..fbda0fdc434 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorViews.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/roi-calculator/_components/ROICalculatorViews.tsx @@ -41,6 +41,10 @@ export function ROIOverview({ const [pagination, setPagination] = React.useState({ query, visibleCount: 10 }); const visibleCount = pagination.query === query ? pagination.visibleCount : 10; const metrics = summary.metrics; + const unavailableRate = + metrics.output_hours > 0 + ? "Spend per estimated hour is unavailable until all selected repositories can be read." + : "A rate requires matched estimated hours greater than zero and access to all selected repositories."; return (
@@ -59,7 +63,7 @@ export function ROIOverview({

{metrics.cost_per_hour != null ? `${formatMoney(metrics.matched_spend)} gateway spend รท ${formatNumber(metrics.output_hours)} estimated engineering hours = ${formatMoney(metrics.cost_per_hour)} per estimated hour.` - : "A rate is available when matched estimated hours are greater than zero."} + : unavailableRate}

The comparison includes {metrics.cohort_people} matched {metrics.cohort_people === 1 ? "person" : "people"}{" "}