Repository navigation
fix(frontend): accept safety_identifier, prompt_cache_retention, verbosity in chat - #17508
mananpatelll wants to merge 1 commit into
Conversation
…osity in chat These are valid OpenAI Chat Completions fields that Dynamo doesn't act on, so clients sending them got a 400. Check their values and drop them, the way /v1/responses already handles them. Legacy completions still rejects them. Signed-off-by: Manan Patel <144215944+mananpatelll@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
👋 Hi mananpatelll! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Define and validate accepted fields lib/llm/src/protocols/openai/validate.rs |
The chat-specific validator allows the three fields and checks their values. Other unsupported fields remain subject to the configured policy. Unit tests cover accepted, null, and invalid values. |
Integrate chat validation and document behavior lib/llm/src/protocols/openai/chat_completions.rs, lib/llm/tests/frontend_protocol_validation_http.rs, docs/fern/pages/developer-guide/knowledge-base/modular-components/frontend/protocol-field-handling.md |
Chat Completions uses the chat-specific validator. HTTP tests verify unary and streaming requests omit the fields from backend requests, and legacy Completions rejects them. The documentation describes the accepted-and-dropped fields. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Severity of issue fixed: Medium
Merge Risk: 
e89d2
-
Chat accepts and drops the three fields while rejecting malformed values. No merge-blocking issue was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the main change: chat completions now accept three OpenAI fields (safety_identifier, prompt_cache_retention, verbosity). It uses the conventional fix(frontend) prefix and matches the o… Description check The description follows the template. It has Overview, Key Changes, Validation, and Issues and Ongoing Work sections. The Overview states the problem and the fix. Key Changes lists each touched file a… Linked Issues check Directly linked issue #17506has two coding parts. (1) Chat completions must accept safety_identifier, prompt_cache_retention, and verbosity. (2) function_call and functions must return 400 instead of…Out of Scope Changes check The changed files are validate.rs, chat_completions.rs, frontend_protocol_validation_http.rs, and protocol-field-handling.md. Each supports the objective of accepting and validating the three chat fie… Full details: Title check
-
Explanation
The title names the main change: chat completions now accept three OpenAI fields (safety_identifier, prompt_cache_retention, verbosity). It uses the conventional fix(frontend) prefix and matches the objectives, which limit the change to chat completions. It is 82 characters, which is slightly long but readable.
Full details: Description check
-
Explanation
The description follows the template. It has Overview, Key Changes, Validation, and Issues and Ongoing Work sections. The Overview states the problem and the fix. Key Changes lists each touched file and the design choice to keep the fields out of PASSTHROUGH_EXTRA_FIELDS. Validation lists the test names and the behavior checks, and it notes the one behavior change under DYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDS. Issues links
#17506with 'Fixes'. The template's 'Closes#XXXX' line is replaced by 'Fixes#17506', which the template's issue-link check should accept. The checklist item is ticked. Minor gap: the template's issue-link comment says the PR Issue Link check is advisory, so no action is required.
Full details: Linked Issues check
-
Explanation
Directly linked issue
#17506has two coding parts. (1) Chat completions must accept safety_identifier, prompt_cache_retention, and verbosity. (2) function_call and functions must return 400 instead of 501. This PR covers part (1). Source at head (lib/llm/src/protocols/openai/validate.rs) adds CHAT_IGNORED_FIELDS with exactly these three fields and a new validate_no_unsupported_chat_fields. That function allows the fields through the unknown-field check and then validates them: safety_identifier is null or a string of at most 128 characters; prompt_cache_retention is null, in_memory, or 24h; verbosity is null, low, medium, or high. The general validate_no_unsupported_fields keeps its previous behavior, so /v1/completions still rejects the fields. The PR summary states chat_completions.rs (around line 712) calls the new chat validator. The issue's Additional Context assigns the function_call/functions change to#17507and this PR's scope to#17508(chat only), so part (2) is not required here. Residual uncertainty: I did not read chat_completions.rs or the test file. The claim that dropped fields are not serialized to the backend rests on the PR description and summary, not on a source read. The test names cited in the PR (for example chat_completions_accept_and_drop_openai_ignored_fields) were not located in the readable portion of the test file.
Full details: Out of Scope Changes check
-
Explanation
The changed files are validate.rs, chat_completions.rs, frontend_protocol_validation_http.rs, and protocol-field-handling.md. Each supports the objective of accepting and validating the three chat fields. validate.rs adds CHAT_IGNORED_FIELDS, the chat-specific validator, and value checks. It also changes validate_no_unsupported_fields_with_ignore to take an accepted-fields list; the general validator passes an empty list, so its behavior is unchanged. chat_completions.rs switches the chat path to the new validator. The HTTP test covers chat acceptance and the /v1/completions rejection. The docs sentence describes the new behavior. The PR also states that malformed values now return 400 when DYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDS is set. This follows from applying the value checks to chat fields and is a documented behavior change. I found no unrelated changes in the inspected source. The function_call/functions 501-to-400 change from
#17506is not in this PR's diff, so it is not an out-of-scope change. Residual uncertainty: the full diff of chat_completions.rs and the test file was not read.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| &["prompt_cache_retention", "safety_identifier", "verbosity"]; | ||
|
|
||
| /// Maximum `safety_identifier` length in the OpenAI request contract. | ||
| const MAX_SAFETY_IDENTIFIER_LENGTH: usize = 128; |
There was a problem hiding this comment.
The Chat Completions contract limits safety_identifier to 64 characters, while 128 is the limit used by the Responses API. With this constant, Dynamo accepts and drops 65–128-character identifiers that OpenAI Chat Completions rejects, so the new compatibility validation does not match the endpoint contract. Use a 64-character limit for chat requests and cover the 64/65 boundary.
🤖 AI Fix
Change the Chat Completions safety identifier limit to 64 characters and add boundary coverage for lengths 64 and 65.
| .await; | ||
| } | ||
|
|
||
| /// OpenAI's `safety_identifier`, `prompt_cache_retention`, and `verbosity` reach the |
There was a problem hiding this comment.
This comment only narrates what the test name and assertions already show: chat requests succeed without forwarding these fields, while legacy completions reject them. It preserves no additional constraint and should be removed.
🤖 AI Fix
Remove this redundant test doc comment.
| let script = load_agent_fixture("text.sse").await.unwrap(); | ||
| let svc = HarnessService::start(vec![script; 2]).await; | ||
|
|
||
| for stream in [false, true] { |
There was a problem hiding this comment.
The stream: true iteration is redundant with the retained stream: false case for this test's acceptance-and-drop contract. Both deserialize the same NvCreateChatCompletionRequest, run the same ValidateEngine before dispatch, and assert the identical serialized backend request; streaming diverges only while formatting the response, which this test does not inspect. One unary request still protects chat acceptance, omission from the backend request, and the separate legacy-completions rejection while avoiding a second scripted HTTP inference.
🤖 AI Fix
Replace the stream loop with one stream: false chat request and reduce the harness fixture to one backend script.
Overview
Chat completions rejects
safety_identifier,prompt_cache_retention, andverbositywith a 400Unsupported parameter(s), even though they're part of OpenAI's Chat Completions request. Right now the only workaround isDYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDS, which lets every unknown field through. This accepts these three on chat completions.Key Changes
About 45 lines of core logic.
validate.rs: a chat-only list,CHAT_IGNORED_FIELDS, andvalidate_no_unsupported_chat_fields. Values are checked against OpenAI's contract (safety_identifierup to 128 chars,prompt_cache_retentionisin_memoryor24h,verbosityislow,medium, orhigh,nullis fine) and then dropped. They stay inunsupported_fields, which is never serialized, so nothing reaches the backend.chat_completions.rs: chat validation calls the new function./v1/completionsis unchanged and still rejects these.protocol-field-handling.md: one sentence about the new set.I kept this out of
PASSTHROUGH_EXTRA_FIELDS, since that list is for backend-handled fields and also covers/v1/completions. Adding typed fields todynamo-protocolswould need a crate release first; once it has them, this list can go.Validation
verbosity: "loud",prompt_cache_retention: "1h", or a 129-charsafety_identifier: 400 naming the field./v1/completionswith these fields: still 400.chat_completions_accept_and_drop_openai_ignored_fieldscovers the first and last, streaming and not, and fails onmain.chat_ignored_fields_reject_malformed_valuesandchat_ignored_fields_are_accepted_only_by_chatcover the value checks and the chat-only scope. Ran on macOS with--no-default-features.One behavior change: with
DYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDSon, a malformed value for one of these is now a 400 instead of being dropped, same as the named passthrough fields.Issues and Ongoing Work
function_call/functions).PASSTHROUGH_EXTRA_FIELDS, which skipped value checks and also opened/v1/completions. feat(frontend): drop named unknown OpenAI request fields via DYN_OPENAI_FE_IGNORED_FIELDS #16967 adds an operator-named ignore list; this doesn't need any config and only covers fields in OpenAI's chat contract.Pull Request Checklist
/pr-checklistskill.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
safety_identifier,prompt_cache_retention, andverbosity. These fields are not forwarded to the backend, so their associated behaviors are not applied.Documentation