Repository navigation
feat: configure assistant reasoning history for compatible providers - #41050
jibanez-staticduo wants to merge 38 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30bee8adc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptileai Please re-review 8a82ea7: cache isolation is fixed, documentation moved to litellm-docs, and real-provider evidence now matches the current tip |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
The auth failure reproduces on unchanged upstream; #41055 fixes it and passes proxy-behavior. OSV flags unchanged mlflow. Details and evidence are documented above |
jibanez-staticduo
changed the title
|
@greptileai Please review the new per-model reasoning field selector, forwarding-policy interaction, cache isolation, and outgoing HTTP tests in this revision |
|
@greptileai Please recheck this revision: omitted model-update fields preserve stored reasoning settings, and the generated snapshot now matches the CI Python version |
|
@greptileai Please review the final revision, including immutable hosted filtering, encrypted model-setting roundtrips, and preservation of omitted options during partial updates |
|
@greptileai Please recheck selector validation before Chat dispatch and typed hosted overrides. Both findings have regression coverage, including encrypted storage and native Responses |
|
@greptileai Please review the current tip The extra commit touches only CI on this tip is 94 successful and 1 skipped with no failures, and the reasoning-scope files plus the owned-inventory file are 1866 passed locally with 8 skipped. |
Upstream moved tests/test_litellm core utils, routing, responses and caching into tests/unit (BerriAI#43199), which collided with the reasoning-history tests. The merge keeps both sides: reasoning normalization and forwarding still run ahead of the configured system-message ordering in the synchronous and asynchronous paths, and the coverage now lives at the upstream locations. Verified against upstream/main 797fddf: 1539 passed in the relocated hosted_vllm, openai chat and litellm_params files, 32 passed and 8 skipped in the router forward_reasoning_content suite, 31 in tests/unit/caching, 308 in the Responses-to-Chat bridge, 337 in model management endpoints. ruff_strict_gate, type_discipline_gate and test_quality_gate pass with --base upstream/main.
|
@greptileai Please review 04ea8f4, which merges current upstream main. The upstream test relocation in #43199 moved the affected suites into |
Upstream added 185 commits since the previous sync, including the owned `Client` pool refactor (BerriAI#43245), the Responses lifecycle-event hold (BerriAI#43238) and the driver preflight split (BerriAI#43259). One conflict, in the `get_litellm_params` import block in `litellm/main.py`: upstream added `InvalidControlOption`, `parse_control_options` and `with_control_options` where this branch added `REASONING_TRANSPORT_KWARGS_KEYS`. Both stay, in the order the import sorter wants. Reasoning normalization still runs ahead of the configured system-message ordering, and the net diff against upstream is the same 16 paths and 1005 added lines as before the sync. Verified on this merge: 1574 passed across the relocated hosted_vllm, openai chat and litellm_params files, 32 passed and 8 skipped in the router suite, 32 in caching, 308 in the Responses-to-Chat bridge, 337 in model management endpoints. ruff_strict_gate, type_discipline_gate and test_quality_gate pass with --base upstream/main.
…m models that use them Declaring `forward_reasoning_content` and `reasoning_content_field` on `GenericLiteLLMParams` reached every caller that builds those params from an unannotated `**kwargs`, because basedpyright reports one unknown-argument diagnostic per declared parameter at every such spread site: 113 sites, so the two fields added 226 diagnostics and pushed `reportUnknownArgumentType` past its ceiling without any of those files changing. `LiteLLM_Params` (deployment and creation) and `updateLiteLLMParams` (model update) are the models that carry the options, and neither is built by spreading unknown kwargs, so declaring them there keeps the API, the generated dashboard types and the partial-update behaviour identical while the shared signature stays as it was.
|
@greptileai Please re-review 1e7d04a: upstream is merged through the client-pool refactor, and the two reasoning-transport options are now declared on |
|
@greptileai Please review 82ef0b9: removed the flagged comments and fixed the shared catalog and Interactions contract tests without skipping validation |
|
@greptileai Please review the updated upstream merge, default forwarding alignment, explicit exclusion, cache identity, and normalized string validation regressions |
| ) -> dict[str, object]: # mutable-ok: provider request contract | ||
| request_messages: Final = normalize_reasoning_content( | ||
| messages, | ||
| forward=litellm_params.get("forward_reasoning_content") is not False, |
There was a problem hiding this comment.
When forward_reasoning_content is omitted, this condition now sends supplied assistant reasoning to Hosted vLLM. Existing configurations can therefore forward history without opting in.
How this was verified: An omitted option enables forwarding, and the hosted request transform retains string-valued assistant reasoning_content.
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!
| if forward_reasoning_content is False: | ||
| cache_key += "forward_reasoning_content: False" |
There was a problem hiding this comment.
During a rolling upgrade with a shared cache, an unexpired response created when omitted forwarding sent no history can be returned for a request that now forwards history. Both use the same legacy cache key.
- batches _raw_batches_request now provides the ASGI scope real starlette requests carry, so the OTLP-trace branch of _read_request_body no longer collapses the body and the metadata 400 names the offending field again - test_outer_deadline_delivers_session_termination gives the initialize/ tools-call handshake its own 2s budget and keeps the original 0.2s fail_after as the real outer cancellation deadline; the in-flight task is cancelled and awaited in finally so session-termination DELETE is delivered before the caller resumes Validated: batches 155 passed; test_mcp_client 409 passed; 14 cancel/teardown variants green under 8-way CPU load.
… native binding stubtest reports the stub parameter "sql" inconsistent with the pyo3 runtime parameter "query" (python-bridge routes/traces.rs), failing the rust-wheel job. Python callers pass the argument positionally, so only the stub name changes; upstream main still carries the stale name.
This reverts commit d8f1ca1.
Resolve the OpenAPI compliance conflict by keeping the branch helpers (_model_request_schema/_interaction_operation/_resolve_local_ref) with their stronger required/readOnly/path-param validations, dropping the now-unused upstream helpers _model_create_request_schema and _interaction_resource_path, and preserving upstream's auto-merged _declared_type_value fix. Inherit canonical GitPython/Tornado bumps from upstream; uv.lock matches the snapshot exactly.
Whitespace-only normalization (blank-line collapse and line wrapping) required by the ruff format gate after merging upstream 424bfd8; no semantic or runtime change. Verified: 8 passed/13 skipped identical to pre-format, ruff-tests and py_compile clean.
This branch has not been deployed
2 participants
TLDR
Problem this solves:
How it solves it:
reasoningexplicitly with per-modelreasoning_content_fieldUser Flow
Before: compatible requests retain the default field name
reasoning_content_field: reasoningPOST http://127.0.0.1:4000/v1/chat/completions, or/v1/responseswith Chat Completions routing, containing assistant reasoning and two completed toolsreasoning_contentAfter: configured requests carry a single canonical reasoning field
reasoning_content_field: reasoningreasoning, with tool IDs, arguments, results, and ordering preservedreasoning_content_fielddefaults toreasoning_content, meaning no additional normalization. Withreasoningselected, a non-null target wins, including an empty string. Otherwise the source value is used when non-null. Only outgoing assistant copies losereasoning_content. Omitting either transport option from a model update preserves its stored value; an explicit false value still disables forwarding. The model representation also accepts encrypted stored values, so subsequent management updates can read them. Invalid selectors fail with HTTP 400 at the Chat dispatch boundary before reaching the backendHosted forwarding follows current upstream: omitting the option or setting it true forwards supplied strings. Explicit false excludes assistant reasoning history. When normalization is selected with false, both field names are removed. Hosted drops non-string values even after normalization. Default and explicit true share a cache key; explicit false is isolated. The OpenAI adapter retains its existing forwarding behavior. Template
preserve_thinking, client responses, native Responses, and signed thinking-block handling remain unchangedThe field selector improves compatibility and endpoint consistency. In vLLM revision
2a02f6efe319c885e3ccbcecde402e0028f9ec1e, Chat Completions already normalizesreasoning_content, while/tokenizedoes not. This extension does not claim demonstrated Chat Completions history loss at that backend layer or a performance improvementRelevant issues
Product configuration and compatibility documentation: BerriAI/litellm-docs#1456
Current upstream restores default hosted string forwarding. This PR retains that behavior and adds a field selector and explicit exclusion
Affected release
Linear ticket
Pre-Submission checklist
At
9f6bfb26ac5a3375cc2f1fe9a7ab60d70ed3bdb1, the branch includes upstream0980f756bd031993329eb0b8b2caa193047e6465and is mergeable.make checkpasses against that upstream snapshot, including Python lint and type budgets, test quality, E2E types, dashboard lint, and API-type synchronization. Coverage thresholds, lint budgets, and signature verification remain unchangedLocal validation passes 515 reasoning tests, 122 cache/OpenAI tests, and 32 router tests with eight existing skips. Model construction now uses
model_validatewith preprocessing preserved, including reserved-key filtering and retry conversion. A mutation that bypasses preprocessing fails the regression test; restoring it passes all 12 API-base tests. Two health-test failures also reproduce on clean upstream in the same environmentCurrent-tip
codecov/patchand Veria pass. The remoteproxy-behaviortests pass all 929 cases, then the Lens coverage upload fails because Codecov CLI 11.3.1 cannot import its GPG key and reportsNo public key, matching codecov-action#1876. An attempted workflow rerun was rejected because it requires repository-admin rights. Therust-testcheck is cancelled rather than passed. Required CI remains incompleteGreptile's visible 2/5 belongs to
2a7abe8ca5, not this tip. Current-tip Greptile confirmation is unavailable, and Bugbot has no visible result. Maintainer review has not been requestedScreenshots / Proof of Fix
Fresh before/after captures against the merge base and this exact public tip are unavailable. Local regression checks and live deployment smoke checks do not replace that feature-specific evidence
Type
Bug Fix, New Feature
Caveats
Medium
Final Attestation