Skip to content

fix(files): use "invalid_request_error" and null instead of string "None" in /v1/files error handlers - #40235

Closed
okxint wants to merge 4 commits into
BerriAI:litellm_internal_stagingfrom
okxint:fix/files-endpoints-type-param-none
Closed

okxint wants to merge 4 commits into
BerriAI:litellm_internal_stagingfrom
okxint:fix/files-endpoints-type-param-none

Conversation

@okxint

@okxint okxint commented Sep 8, 2026

Copy link
Copy Markdown

Summary

Fixes #40135.

All five /v1/files route handlers (create_file, get_file_content, get_file, delete_file, list_files) used getattr(e, "type", "None") and getattr(e, "param", "None") as fallbacks in their except blocks. fastapi.HTTPException defines neither .type nor .param, so the fallback was hit on every HTTPException — meaning every error response from these routes shipped "type": "None" and "param": "None" as literal strings.

Before (every error on these routes):

{"error": {"message": "File not found", "type": "None", "param": "None", "code": "404"}}

After:

{"error": {"message": "File not found", "type": "invalid_request_error", "param": null, "code": "404"}}

Two defects fixed:

  1. type="None"type="invalid_request_error" — the correct OpenAI error category for these endpoints; clients branching on error.type for retry/alert routing can now classify errors
  2. param="None"param=None (JSON null) — OpenAI types param as a nullable string; the string "None" looked like a real param name

10 occurrences of each replaced across the five route handlers (20 total).

Test plan

  • Manual: trigger a 404 on /v1/files/{id} — response now has "type": "invalid_request_error" and "param": null
  • No existing tests changed

yuneng-berri and others added 4 commits September 1, 2026 12:55
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
Four tests fail on Windows when run as a non-elevated user:

* test_rejects_symlink_pointing_to_non_image
* test_accepts_symlink_pointing_to_image
  (test_static_asset_utils.py)

* test_writes_through_a_symlinked_settings_file
* test_written_file_is_owner_only
  (test_claude_settings.py)

The first three call os.symlink / Path.symlink_to without a
privilege guard. Windows permits symlink creation only for an elevated
process or with Developer Mode enabled, so both fail with
OSError: [WinError 1314].

The fourth asserts stat.S_IMODE(...) == 0o600; POSIX mode bits are
not enforced on Windows (chmod is a no-op), so the mode comes back as
0o666 and the assertion fails.

Add @pytest.mark.skipif(sys.platform == "win32", ...) to all four
tests, matching the pattern already used in
tests/test_litellm/proxy/db/test_query_engine_reaper.py.

Fixes BerriAI#40046

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…/v1/files error handlers

All five /v1/files route handlers (create_file, get_file_content,
get_file, delete_file, list_files) used getattr fallbacks of "None"
(a string) for both the type and param fields of ProxyException.
fastapi.HTTPException has neither attribute, so the fallback was hit
on every request that raised an HTTPException — meaning every error
response from these routes shipped:

  {"error": {"type": "None", "param": "None", ...}}

Two separate defects:
- type="None" is not a recognised error category; clients branching on
  error.type for retry policy could not classify the error
- param="None" is the string "None", not JSON null; OpenAI's schema
  types param as nullable string, so "None" looks like a real param name

Fix: replace the 10 type fallbacks with "invalid_request_error" (the
correct OpenAI category for these errors) and the 10 param fallbacks
with None (JSON null), matching OpenAI's own error contract.

Fixes BerriAI#40135

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@okxint
okxint requested a review from a team September 8, 2026 10:34
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ yuneng-berri
❌ okxint
You have signed the CLA already but the status is still pending? Let us recheck it.

@codspeed

codspeed Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing okxint:fix/files-endpoints-type-param-none (90d78f6) with litellm_internal_staging (1af7a40)

Open in CodSpeed

@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates error metadata returned by all five OpenAI-compatible file handlers and adjusts platform-specific tests

  • Replaces literal "None" parameter values with JSON null
  • Defaults file errors to invalid_request_error
  • Skips POSIX mode and symlink tests on Windows
  • The generic 500 path now receives a client-error classification
  • No focused regression test covers the corrected file response contract

Confidence Score: 4/5

This PR should not merge until internal 500 errors retain an appropriate classification and the explicit regression-test requirements are satisfied

The nullable parameter change is safe, but generic provider and storage failures are now mislabeled as invalid requests, and the changed contract has no regression test

Files Needing Attention: litellm/proxy/openai_files_endpoints/files_endpoints.py, tests/test_litellm/proxy/common_utils/test_static_asset_utils.py, tests/test_litellm/proxy/client/cli/test_claude_settings.py

Important Files Changed

Filename Overview
litellm/proxy/openai_files_endpoints/files_endpoints.py Corrects null serialization but also labels untyped internal 500 failures as invalid requests and lacks regression coverage
tests/test_litellm/proxy/client/cli/test_claude_settings.py Adds Windows skips for mode and symlink tests, with the symlink skip broader than the unsupported capability
tests/test_litellm/proxy/common_utils/test_static_asset_utils.py Unconditionally removes direct symlink-validation coverage from Windows environments, including those where symlinks work

Reviews (1): Last reviewed commit: "fix(files): replace string \"None\" with..." | Re-trigger Greptile

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

message=getattr(e, "message", error_msg),
type=getattr(e, "type", "None"),
param=getattr(e, "param", "None"),
type=getattr(e, "type", "invalid_request_error"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Internal failures mislabeled

A storage or provider exception can return HTTP 500 with invalid_request_error, causing type-based clients to treat a server failure as invalid input

Knowledge Base Used: Proxy endpoints and request processing

Comment on lines +688 to +689
type=getattr(e, "type", "invalid_request_error"),
param=getattr(e, "param", None),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Regression test missing

This public error-contract fix lacks a focused regression test. Repository rules require one for bug fixes before merging

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

result = resolve_validated_local_image_path("/proc/self/environ")
assert result is None

@pytest.mark.skipif(sys.platform == "win32", reason="os.symlink needs elevation or Developer Mode on Windows")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Windows symlink coverage removed

If Windows Developer Mode enables symlinks, these platform-wide skips still remove validation coverage, violating the repository's test-integrity requirement before merging

Rule Used: What: Flag any modifications to existing tests and... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@mateo-berri

Copy link
Copy Markdown
Contributor

Closing as superseded: #39536 landed the same /v1/files fix plus the rerank, images, realtime, and pass-through tails, with regression tests

@mateo-berri mateo-berri closed this Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: All five /v1/files routes ship type and param as the literal string "None", so every error body is unclassifiable

4 participants