fix(proxy): list_files honors AsyncCursorPage from post-call hook (LIT-3386)#28957
Conversation
|
|
Greptile SummaryFixes a silent discard of the
Confidence Score: 5/5Change is minimal and surgical — one import and one isinstance tuple widening — with no behaviour change for existing hooks that mutate in place or return None. The fix touches exactly one conditional in one endpoint, is backward-compatible with every existing hook return shape, and all three branches of the broadened check are pinned by new regression tests. The import is correctly placed among other imports with no misleading annotations. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/openai_files_endpoints/files_endpoints.py | Broadens post-call hook type guard in list_files to accept AsyncCursorPage in addition to OpenAIFileObject; adds clean AsyncCursorPage import between existing import blocks. |
| tests/test_litellm/proxy/openai_files_endpoint/test_list_files_post_call_hook.py | New regression test file with three mock-only tests pinning all branches of the broadened isinstance check: AsyncCursorPage return, None return, and legacy OpenAIFileObject return. |
Reviews (2): Last reviewed commit: "test(proxy): add coverage for OpenAIFile..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Addressed both Greptile P2 comments in two commits:
Test suite is now 3 passing locally: @greptile-apps please re-review. |
|
Closing — bulk cleanup of PRs filed by this account. |
Summary
Closes the remaining secondary bug called out in GH #28294
The customer's analysis of #28339 and #27984 confirmed:
After
proxy_logging_obj.post_call_success_hookruns,list_filesreassigns the response only when the hook returns anOpenAIFileObject.UnifiedFileIdHook.async_post_call_success_hookactually returns anopenai.pagination.AsyncCursorPagefor the list-files response shape (seeenterprise/litellm_enterprise/proxy/hooks/managed_files.py:1209-1232), soisinstance(_response, OpenAIFileObject)is alwaysFalseand the hook's return value is silently dropped. In practice the bug is partially masked by the hook also mutatingresponse.datain place, but the type check itself is wrong and any future hook that returns a freshAsyncCursorPage(built from a different upstream payload, or rebuilt to drop a forbidden item, etc.) is silently discarded.The other site that uses the same pattern —
acreate_file(POST /v1/files) at line ~524 — does not need this change becauseacreate_fileonly returnsOpenAIFileObject.Fix 1 from the GH issue (
CheckBatchCostpoller writing output files withcreated_by = default_user_idinstead ofjob.created_by) is already in place — see the_minimal_authblock atenterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py:307-326, which was added in #27984 (5/15). That code path passesuser_id=job.created_byintostore_unified_file_id, which is what writesLiteLLM_ManagedFileTable.created_by. No change needed there in this PR.Change
Two new pytest cases pin the behavior at the FastAPI endpoint layer (
TestClientagainst the live/v1/filesroute, realapp.dependency_overrides, realproxy_logging_obj).Evidence
BEFORE the fix — the regression test fails because the hook's filtered
AsyncCursorPageis dropped and the endpoint returns the raw upstreamAsyncCursorPageto the caller:AFTER the fix — both tests pass; the endpoint reassigns
responseto the hook's filteredAsyncCursorPage, so the caller sees only their own (managed-id) files:The test drives the endpoint through
fastapi.testclient.TestClient(full FastAPI request/response stack), patcheslitellm.afile_listto act as the upstream provider, and patchesproxy_server.proxy_logging_obj.post_call_success_hookto return a freshAsyncCursorPageinstance with differentdata. The before/after difference is observable at the HTTP layer inr.json()["data"].Risk
list_files).acreate_file(POST /v1/files) unchanged.Nonestill pass through (covered bytest_list_files_passes_through_unchanged_when_hook_returns_none). Hooks that returnOpenAIFileObject(legacy / synthetic single-file responses) still honored.UnifiedFileIdHookbehavior: mutatesresponse.datain place AND returns the sameresponse, so the user-visible behavior is unchanged for the production hook. The fix corrects the type check so that any future hook (including a refactor ofUnifiedFileIdHookthat constructs a fresh page) is honored.Note on diff shape
Pushed via the GitHub Contents API (current
GITHUB_TOKENlacksrepo+workflowscopes). The net working-tree change is exactly the two files shown above.Closes LIT-3386.
Verification (ship-pr)
litellm_oss_agent_shin_daily_branch(per Shin policy)codecov/patch— Codecov bot informational only, not part of GitHub Actions, not blocking the merge gate.tests/test_litellm/proxy/openai_files_endpoint/test_list_files_post_call_hook.py, all passing locally. The first test fails on a reverted main, confirming it is a real regression guard./v1/files).