fix(frontend): accept empty image URLs with UUIDs

[Chokoyo]

Overview:

Allow Dynamo to accept UUID-only image references emitted by AIPerf #869:

{"image_url":{"url":""},"uuid":"image-42"}

Dynamo currently rejects the empty URL String (only accept null) during typed deserialization before UUID handling is reached.

Details:

• Preserve the normal parsing path for valid requests.
• Normalize an empty image URL with a non-empty UUID to image_url: null.
• Route the normalized input through the existing UUID-only path.
• Continue rejecting empty image URLs without a UUID.
• Add tests for both accepted and rejected cases.

Where should the reviewer start?

lib/llm/src/http/service/openai.rs, specifically parse_chat_completion_request and normalize_empty_uuid_image_urls.

Related Issues

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of multimodal chat requests containing cache-only images.
  • Requests with an empty image URL and valid cache identifier are now accepted correctly.
  • Invalid image references without the required cache identifier continue to be rejected with consistent error responses.

[coderabbitai]

Walkthrough

The chat-completions handler now uses a dedicated parser that normalizes cache-only image parts with empty URLs and UUIDs. Tests cover acceptance with a UUID and rejection without one.

Changes

Chat completion normalization

Layer / File(s)	Summary
Request parsing normalization lib/llm/src/http/service/openai.rs	Adds chat-completion-specific parsing that retries failed deserialization after converting eligible empty image_url.url values to null.
Handler integration and validation lib/llm/src/http/service/openai.rs	Routes the handler through the new parser and tests UUID-backed acceptance alongside missing-UUID rejection.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	Overview, details, and reviewer start are present, but the required Related Issues section is incomplete.	Add a linked issue or explicitly confirm there is no related issue in the required Related Issues section.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change: accepting empty image URLs when a UUID is present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

lib/llm/src/http/service/openai.rs (1)

3831-3841: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the normalization contract in the acceptance test.

The test verifies that the UUID survives serialization but does not assert that image_url was rewritten to null, which is the core behavior under test.

Suggested assertion

-        assert_eq!(
-            serde_json::to_value(request).expect("request should serialize")["messages"][0]["content"]
-                [0]["uuid"],
-            "image-42"
-        );
+        let request = serde_json::to_value(request).expect("request should serialize");
+        assert_eq!(request["messages"][0]["content"][0]["uuid"], "image-42");
+        assert_eq!(
+            request["messages"][0]["content"][0]["image_url"],
+            serde_json::Value::Null
+        );

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/llm/src/http/service/openai.rs` around lines 3831 - 3841, Update
test_parse_chat_completion_request_accepts_empty_image_url_with_uuid to also
assert that the serialized content item's image_url field is null, while
preserving the existing UUID assertion.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/llm/src/http/service/openai.rs`:
- Around line 1453-1454: Update normalize_empty_uuid_image_urls to parse the
request body via parse_json_request::<serde_json::Value> instead of
serde_json::from_slice, preserving control-character escaping and lossy UTF-8
handling during normalization retries.

---

Nitpick comments:
In `@lib/llm/src/http/service/openai.rs`:
- Around line 3831-3841: Update
test_parse_chat_completion_request_accepts_empty_image_url_with_uuid to also
assert that the serialized content item's image_url field is null, while
preserving the existing UUID assertion.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 73c4ad33-46b4-48bc-88ed-5662fdd7579e

📥 Commits

Reviewing files that changed from the base of the PR and between 56cd43e and 941b072.

📒 Files selected for processing (1)

• lib/llm/src/http/service/openai.rs

[datadog-official]

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 43.91% (+3.44%)

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: cab9f61 | Docs | Datadog PR Page | Give us feedback!}

[rmccorm4]

I think this belongs one layer down, in dynamo-protocols, rather than in the HTTP handler.

The root cause is type-level: ImageUrl.url is url::Url, so "" is unrepresentable and fails in Deserialize before any semantic layer sees it. The layer that should own this decision already exists and already handles every (url, uuid) combination for all three modalities — lib/llm/src/preprocessor.rs:1473-1499. It just never receives the value.

Proposed fix

One helper in protocols/src/types/chat.rs (ai-dynamo/frontend-crates), alongside the existing deserialize_arguments_opt:

/// Deserialises an optional media object, treating `{"url": ""}` as absent.
///
/// vLLM's OpenAI-compatible schema requires the media object to be present, so
/// UUID-cache clients emit an empty URL where Dynamo's canonical form is `null`.
/// Normalising at the type boundary keeps the empty-vs-null distinction out of
/// every call site and leaves the `(url, uuid)` policy to one downstream check.
fn deserialize_optional_media<'de, D, T>(deserializer: D) -> Result<Option<T>, D::Error>
where
    D: serde::Deserializer<'de>,
    T: serde::de::DeserializeOwned,
{
    use serde::de::Error;
    match Option::<serde_json::Value>::deserialize(deserializer)? {
        None | Some(serde_json::Value::Null) => Ok(None),
        Some(value) if value.get("url").and_then(serde_json::Value::as_str) == Some("") => Ok(None),
        Some(value) => serde_json::from_value(value).map(Some).map_err(D::Error::custom),
    }
}

Plus one attribute change on each of ChatCompletionRequestMessageContentPartImage, ...PartVideo, and ...PartAudioUrl:

     #[builder(default)]
-    #[serde(default)]
+    #[serde(default, deserialize_with = "deserialize_optional_media")]
     pub image_url: Option<ImageUrl>,

That crate's own comment says this is exactly why these types are defined locally instead of re-exported from upstream (protocols/src/types/chat.rs:236): "We define these locally so we can attach #[serde(deserialize_with)]".

I prototyped this locally against dynamo-protocols 4.0.0. cargo test -p dynamo-protocols: 77 passed, 0 failed, no regressions. Behavior:

image_url  {"url":""} + uuid -> {"image_url":null,"uuid":"cache-key"}
video_url  {"url":""} + uuid -> {"video_url":null,"uuid":"cache-key"}
audio_url  {"url":""} + uuid -> {"audio_url":null,"uuid":"cache-key"}
real URL                     -> unchanged
{"url":"not a url"}          -> Err("relative URL without a base") (still rejected)

Why not the handler

It's a cost on the steady-state path, not an error path. For a client using UUID caching, every cache-hit request takes the fallback. Microbenchmarked parse_chat_completion_request on a 190KB multi-turn body, release build:

canonical (image_url: null)  158 us
fallback  (url: "")          947 us   -> 5.97x

The failing request runs the whole ladder: typed parse, control-char escape scan, from_utf8_lossy scan, from_slice::<Value>, then from_value re-cloning every string. The deserializer approach is a single parse — Option::<Value>::deserialize materializes only the small media object, not the request body.

Coverage. The handler fix misses lib/bindings/c/src/lib.rs:1285, which does a bare serde_json::from_value::<NvCreateChatCompletionRequest>. It also doesn't cover video_url/audio_url, which have the identical Option<T> + url: Url + sibling uuid shape. That's currently moot since preprocessor.rs:1426,1438 reject UUIDs on video/audio outright, but it's a gap the type-level fix closes for free.

Composition. The new fallback wraps parse_json_request, which already has two repair rungs (openai.rs:1416-1439), and it re-parses the original body. So a body needing both control-char escaping and URL normalization still 400s. Fixing at the type means the rungs don't have to multiply.

Schema. The utoipa-derived OpenAPI schema (openapi_docs.rs:247) is generated from these types, so it currently advertises url as a required string while the handler accepts otherwise.

Errors. With the type-level fix, url: "" without a UUID falls through to preprocessor.rs:1497 — "image_url part has neither url nor uuid; at least one is required" — instead of "data did not match any variant of untagged enum ChatCompletionRequestUserMessageContent".

Smaller notes on the current patch

• No tracing::warn! on the repair path, unlike both sibling rungs (openai.rs:1424, openai.rs:1454). Without it there's no signal that clients are sending non-canonical bodies.
• or_else returns the second error, and from_value errors carry no position. A body with url: "" plus an unrelated type error reports invalid type: string "hot", expected f32 with no at line N column M, where the normal path includes it.
• detail is silently dropped: {"url":"","detail":"high"} + uuid becomes image_url: null. Same in both approaches, so not a blocker.
• docs/features/multimodal/multimodal-vllm.md:174-188 documents image_url: null as the form and says UUIDs "must use the top-level field shown above". Worth updating either way, since this widens the accepted wire format.