Repository navigation
feat(decisions): backport the rest of the decisions and System One launch to rc/1.106.0 - #45891
Conversation
The picks above each touched schema.d.ts on main against a different OpenAPI surface, so the file is regenerated on this line with npm run gen:api instead of hand-merged.
GitHub Actions
Error
|
|
|
bugbot run |
|
@litereview greptile code-review |
✅ greptileReview incomplete. Merge readiness: 3/5Fix the new System One test's logging-queue isolation before merging. I reproduced its failure with the CI worker configuration; the same test passes alone. The focused backend run finished with 152 passed and 1 failed
Couldn’t complete
Checks and execution details
View review · Reviewed |
✅ code-reviewCompleted · Some checks not run. StandardsI read the HEAD versions of the listed Python modules, the OpenRouter diff hunks, the endpoint and OpenAPI helper, and skimmed the System One UI. I checked them against root Hard violations (documented standard)
Judgement calls (Fowler baseline)
UI: I found no Totals: 5 hard-violation groups and 6 judgement-call smells. The worst is that the new decisions core is built around
Checks and execution details
View review · Reviewed |
![litereview[bot]](https://avatars.githubusercontent.com/in/5255007?s=60&v=4){kind=small}
|
|
||
| @pytest.mark.asyncio | ||
| async def test_router_asystemone_logs_the_systemone_call_type( | ||
| respx_mock: respx.MockRouter, monkeypatch: pytest.MonkeyPatch |
There was a problem hiding this comment.
Medium · Drain pending logging callbacks before installing the recorder
Running this file with pytest tests/unit/systemone/test_main.py -n 4 --dist=loadscope fails at line 137. Earlier tests leave callbacks in GLOBAL_LOGGING_WORKER; after this test installs its recorder, flush() executes those callbacks against it, adding two unrelated call types. The test passes alone, so the assertion depends on test order. The new core-utils CI shard uses this worker configuration with no reruns. Request the existing drained_logging_worker fixture before installing the recorder, or drain callbacks in the preceding tests
File run: 6 passed, 1 failed; worker reported carrying 2 pending logging tasks onto the new loop
Isolated test: 1 passed
tests/unit/conftest.py:355 defines drained_logging_worker
Review by greptile · Reviewed acf745591166
There was a problem hiding this comment.
Fixed in cd141c6, picked the per-test logging-worker flush hook main carries in tests/unit/conftest.py, the file now passes with -n 4 --dist=loadscope
| ) | ||
| provider_config: Final = _provider_config(upstream_model, provider) | ||
| if not upstream_model: | ||
| raise litellm.BadRequestError( |
There was a problem hiding this comment.
Medium · Decisions core throws instead of modelling failures as values
Root AGENTS.md says: "Don't throw; model failures as values." prepare_call has seven raise sites. _provider_config and reject_other_format each raise as well. _UnsupportedSafetyIdentifier is turned into a value and then raised straight away.
call.py L103, 171, 179, 184, 192, 199, 212, 225, 343
databricks/decisions/transformation.py L14, L50
strands_decider/decisions/transformation.py L157
Review by code-review · Reviewed acf745591166
There was a problem hiding this comment.
Faithful pick, litellm/decisions/call.py is byte-identical to main, so the failures-as-values refactor belongs on main first, then a follow-up pick
| ) | ||
|
|
||
| router: Final = APIRouter() | ||
| _REQUEST_DATA_ADAPTER: Final[TypeAdapter[dict[str, object]]] = TypeAdapter(dict[str, object]) |
There was a problem hiding this comment.
Low · Coarse dict types and an untyped endpoint return
Root AGENTS.md says code must be fully typed, with no dict[str, Any] or bare dict. The request data and general settings are validated into dict[str, object], and the systemone/decisions handlers have no return annotation.
_REQUEST_DATA_ADAPTER: Final[TypeAdapter[dict[str, object]]]
async def systemone(...): # L191, no return type
Review by code-review · Reviewed acf745591166
There was a problem hiding this comment.
Faithful pick, endpoints.py matches main byte for byte. Typing changes land on main first and get picked afterwards, not on a release line
| self._set_provider_reported_cost(terminal_response) | ||
| return response_event | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
Low · OpenRouter cost helper mutates the response in place
Root AGENTS.md says: "No mutation." _set_provider_reported_cost returns None and calls set_hidden_params on the response it was passed. It also builds TypeAdapter(dict[str, object]) on every call.
def _set_provider_reported_cost(response) -> None: ... set_hidden_params(response, {...})
Review by code-review · Reviewed acf745591166
There was a problem hiding this comment.
Faithful pick, this transformation.py is identical to main. A no-mutation rewrite lands on main first, this backport only carries merged code
| } | ||
|
|
||
|
|
||
| class StrandsDeciderDecisionsConfig(BaseDecisionsConfig): |
There was a problem hiding this comment.
Low · Provider configs depend on inheritance rather than composition
Root AGENTS.md says: "Composition over inheritance." Each new decisions provider subclasses BaseDecisionsConfig and changes its behaviour through attribute and method overrides plus super() calls.
class StrandsDeciderDecisionsConfig(BaseDecisionsConfig)
return super().transform_decisions_response(...)
Review by code-review · Reviewed acf745591166
There was a problem hiding this comment.
Faithful pick, strands_decider transformation.py is identical to main. Changing the config class design is a main refactor, out of scope for a backport
| ) | ||
|
|
||
|
|
||
| async def asend(call: DecisionsCall) -> DecisionsResponse | OpenAIDecisionResponse: |
There was a problem hiding this comment.
Low · Duplicated sync/async send and entry-point bodies (judgement call)
Possible Duplicated Code: asend and send differ only by await. The public sync/async wrappers in decisions/main.py and systemone/main.py repeat the same 11-argument forwarding.
asend L300-317 vs send L320-337
decisions/main.py L75-128; systemone/main.py L51-104
Review by code-review · Reviewed acf745591166
There was a problem hiding this comment.
Judgement call on code identical to main. Deduplicating send and asend is a main refactor, this backport ships merged code only
| state: DecisionsJSON | None | ||
| questions: SystemOneQuestions | None | ||
|
|
||
| def validate(self, safety_identifier: str | None) -> DecisionsRequestBody: |
There was a problem hiding this comment.
Low · SystemOneRequestFields.validate ignores its parameter (judgement call)
Possible Refused Bequest: this implementation of the shared validate(safety_identifier) signature drops the argument, and _ir_request puts it back later with replace.
def validate(self, safety_identifier: str | None) -> DecisionsRequestBody: return ...({"state": ..., "questions": ...})
Review by code-review · Reviewed acf745591166
There was a problem hiding this comment.
Same shape as main byte for byte. Any change to SystemOneRequestFields.validate lands on main first, then gets picked here
…allback checks stay isolated Ports the pytest_runtest_call hook from tests/unit/conftest.py on main (#44146). Without it the asystemone call type test picks up adecisions callbacks queued by earlier tests in the same worker Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> (cherry picked from commit 23fec69)
|
@litereview greptile code-review |
|
bugbot run |
✅ greptileScore: 4/5The backport passed 582 targeted backend and dashboard tests. I found one low-severity SDK regression: malformed question inputs bypass LiteLLM’s
Checks and execution details
View review · Reviewed |
✅ code-reviewCompleted · Some checks not run. I've finished the Spec-axis review, including a read of the head source to check each finding. I found no missing PR and no regression against what the description promises. There are four findings: one partial requirement, two scope-creep items and one leftover from conflict resolution. I couldn't run tests: Spec(a) Missing or partial
(b) Scope creep
(c) Looks implemented but wrong
Checked and matching the spec
Summary: 4 Spec findings. The worst is that
Checks and execution details
View review · Reviewed |
| questions: OpenAIDecisionQuestions | SystemOneQuestions | None, | ||
| kwargs: Mapping[str, object], | ||
| ) -> RequestFields: | ||
| if "state" not in kwargs: |
There was a problem hiding this comment.
Medium · litellm.decisions still accepts the System One state shape
The spec says "litellm.decisions only takes the OpenAI shape while the System One shape moves to litellm.systemone".
At head, _request_fields still routes state= calls to SystemOneRequestFields and only raises a UserWarning. The return types are still OpenAIDecisionResponse | DecisionsResponse.
The other direction is strict: systemone rejects input with reject_other_format. Either the PR text should say this is a soft deprecation, or decisions should reject state the same way.
litellm/decisions/main.py:34-46 deprecation-warn path
litellm/decisions/main.py:87,115 union return types
litellm/systemone/main.py:32 reject_other_format for input
Review by code-review · Reviewed cd141c6d3656
There was a problem hiding this comment.
Fair, main's #45846 is a soft deprecation too, main.py is byte-identical. Reworded the PR body to say state= warns rather than rejects
| prisma_client=prisma_client, proxy_logging_obj=proxy_server.proxy_logging_obj | ||
| ) | ||
| return proxy_server.llm_router is not None | ||
| if _holds_an_auto_router(rows): |
There was a problem hiding this comment.
Low · Auto-router registry miss now reloads every deployment; the spec doesn't mention it
_holds_an_auto_router triggers a full add_deployment reconcile, which changes proxy-wide DB load.
None of the 15 Included PR titles or the User Flow mentions this; only the Cursor summary does. If it comes with #45200, the PR description should say so.
registry_read_through.py: if _holds_an_auto_router(rows): await proxy_config.add_deployment(...)
Review by code-review · Reviewed cd141c6d3656
There was a problem hiding this comment.
It comes with #45200, which is in the Included PRs list, and the file matches main. Added a line to the body naming the auto-router reconcile
| from litellm._logging import ALL_LOGGERS # noqa: E402 # same import-time dependency | ||
| from litellm.litellm_core_utils.logging_worker import GLOBAL_LOGGING_WORKER # noqa: E402 # same import-time dependency | ||
| from litellm.anthropic_beta_headers_manager import reload_beta_headers_config # noqa: E402 # same import-time dependency | ||
| from litellm.litellm_core_utils.logging_worker import GLOBAL_LOGGING_WORKER # noqa: E402 # same import-time dependency |
There was a problem hiding this comment.
Low · Duplicate GLOBAL_LOGGING_WORKER import plus an unrelated conftest hook
Line 50 repeats the import on line 48, which looks like a cherry-pick merge mistake.
The same file also adds a global pytest_runtest_call hook that flushes the logging worker. That isn't part of any listed decisions PR.
tests/unit/conftest.py:48 and :50 identical imports
tests/unit/conftest.py:163-187 new hook
Review by code-review · Reviewed cd141c6d3656
There was a problem hiding this comment.
Dropped the duplicated import in d5b0ad0. The flush hook is main's, picked so the systemone tests pass under xdist, now called out in the body
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| _run_coroutine_if_needed(_complete_test_logging()) | ||
| return | ||
| if worker_loop.is_running(): | ||
| asyncio.run_coroutine_threadsafe(_complete_test_logging(), worker_loop).result() |
There was a problem hiding this comment.
Test flush hook can deadlock
Medium Severity
When the logging worker loop is still running, _flush_completed_test_logging blocks on run_coroutine_threadsafe(...).result() with no timeout. If that loop belongs to the current thread, the thread waits for a flush/stop that cannot run, and the unit suite hangs after a test that left work in GLOBAL_LOGGING_WORKER.
Reviewed by Cursor Bugbot for commit cd141c6. Configure here.
There was a problem hiding this comment.
Hook is byte-identical to main's. pytest_runtest_call yields after the test coroutine returns, so the current thread never owns a running loop here
| ) -> RequestFields: | ||
| if "state" not in kwargs: | ||
| return OpenAIDecisionRequestFields( | ||
| input=decision_input, questions=_OPENAI_QUESTIONS_ADAPTER.validate_python(questions) |
There was a problem hiding this comment.
Low · Preserve BadRequestError for malformed SDK questions
Calling litellm.decisions(model="openai/test", input="test", questions=[5], api_key="test") now raises a Pydantic ValidationError without a status code. The base version raises litellm.BadRequestError with status 400. SDK callers that catch LiteLLM errors therefore miss this failure. _request_fields() runs before the validation exception handler in prepare_call(). Move these adapter validations, including the legacy state adapters, inside the same error-conversion boundary
Reproduced on both decisions and adecisions at the supplied head
Reconstructed the base decisions/main.py from the supplied diff and confirmed BadRequestError (400) for the same input
Review by greptile · Reviewed cd141c6d3656
There was a problem hiding this comment.
Faithful pick of #45846, litellm/decisions/main.py is byte-identical to main, so this ships there today and the fix lands on main first
…onftest pick Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@litereview greptile code-review |
|
bugbot run |
✅ greptileCompleted · Some checks not run. Score: 4/5The backport looks close to merge-ready. I found one SDK error-handling regression: malformed questions now escape as Pydantic errors instead of Verification: 391 tests passed across the selected files. Four OCR health-check cases failed because the local Rust extension was unavailable
Checks and execution details
View review · Reviewed |
✅ code-reviewReview incomplete. The review could not complete. Couldn’t complete
View review · Reviewed |
| ) -> RequestFields: | ||
| if "state" not in kwargs: | ||
| return OpenAIDecisionRequestFields( | ||
| input=decision_input, questions=_OPENAI_QUESTIONS_ADAPTER.validate_python(questions) |
There was a problem hiding this comment.
Low · Preserve BadRequestError for malformed SDK questions
Calling litellm.decisions(model="openai/test", input="hello", questions={"q": {"type": "predicate", "instructions": "yes?"}}, api_key="dummy") now raises a raw Pydantic ValidationError. The base returned litellm.BadRequestError with status 400, so applications catching the documented LiteLLM exception no longer handle this invalid request. The same regression affects adecisions. Move these adapter validations inside the ValidationError conversion boundary in prepare_call, or wrap them here
A local comparison against the base implementation reconstructed from the supplied diff returned base: BadRequestError 400 and head: ValidationError None
_prepare_decisions_call evaluates _request_fields(...) before entering prepare_call, so its validation-error handler cannot catch these exceptions
Review by greptile · Reviewed d5b0ad087ce5
There was a problem hiding this comment.
Same as the thread above, main.py is byte-identical to main's #45846. Moving the adapters inside prepare_call lands on main first, then gets picked
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d5b0ad0. Configure here.
|
@litereview code-review |
Review not startedLiteReview only reviews open pull requests. |
This branch is waiting to be deployed
3 participants
TLDR
Problem this solves:
ai_decideand AgentCore Strands Decider are missinglitellm.systemoneHow it solves it:
schema.d.tson this line in its own commitIntentional product change: #44955 and #45846 are carried over as merged on main.
/v1/decisionsnow refusessafety_identifieron providers that cannot take it unlessdrop_paramsis on, andlitellm.decisionsis trimmed to the OpenAI shape while the System One shape moves tolitellm.systemone, withstate=onlitellm.decisionskept as a soft deprecation that warns and still routes, exactly as on main. #45200 also makes a registry miss on an auto-router row run the fulladd_deploymentreconcile inregistry_read_through.py, as merged on main.litellm.systemoneIncluded PRs
The line already had #45639, #45673, #45654, #45717 (through #45735), #45721 (through #45739) and #45663 (through #45888). With this PR the decisions backend, SDK and playground on rc/1.106.0 match main
Conflict notes. #45846 landed on main before #45663 but reaches this line after it, so
litellm/decisions/call.pyand its two importers are resolved to main's final state (supported_decisions_providers).schema.d.tsis generated, so it is regenerated withnpm run gen:apiin the last commit and never hand-merged. No version bump, since rc lines keeppyproject.tomlat1.106.0Test isolation. cd141c6 picks main's per-test logging-worker flush hook into
tests/unit/conftest.pysotests/unit/systemone/test_main.pypasses with-n 4 --dist=loadscope, which LiteReview reproduced failing at acf7455. d5b0ad0 then drops the import line the pick duplicated. Both change test setup onlyUser Flow
Before
openrouter/typesafe/jev-1.13, POSTs/v1/decisionsand gets 200 with nox-litellm-response-costheader, so the spend log stays at 0/v1/decisionswithmodel: databricks/...and gets 400 Unknown Decisions provider 'databricks'safety_identifierto a TypeSafe model and it is silently dropped (200)After
/v1/decisionson OpenRouter returnsx-litellm-response-cost: 1.4616e-05, the provider-reported cost, and/v1/responsesbills the same way/v1/decisionsondatabricks/...returns 200 with answers from the Databricks serving endpointsafety_identifierto TypeSafe returns 400 UnsupportedParamsError unless the deployment setsdrop_params: true, which returns 200/v1/decisionsand/v1/systemoneselectable, an Example preset dropdown (Triage a bug report, Route a support ticket, Moderate a forum comment, Score a sales lead) plus a Clear button, anoulquestion type with hover help, and the models page banner links to the supported providers docsScreenshots / Proof of Fix
Live /qa leg at acf7455 (cd141c6 on top changes test setup only) against the merge base (1bdfb67): two proxies from clean worktrees with
--num_workers 2, their own Postgres and Redis, plus the dashboard dev server per leg, real TypeSafe, OpenRouter and Databricks keys from the shared vault, real spendTwo picks could not be exercised live and are blocked, not mocked: hosted_vllm (no vLLM server; the route now resolves the provider and fails on connect instead of 400 unknown provider) and the Strands Decider AgentCore call (no AgentCore runtime ARN credential in the shared vault)
Pre-Submission checklist
uv run pytest tests/unit/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Type
New Feature
Caveats (if any)
Medium
/v1/decisionscall withsafety_identifiernow gets a 400 on providers without itdrop_paramsdrops the field and restores the old behaviorLow
tests/integration/providers/test_decisions_wire.pylacks one skipped test from test: fix stale and flaky tests across CircleCI, GHA and Buildkite #45530, which is test-only and not pickedFinal Attestation
Note
Medium Risk
Includes a breaking change for
safety_identifieron unsupported providers and touches proxy routing, billing headers, and AWS-signed outbound requests for AgentCore; scope is large but mostly additive behind new providers and routes.Overview
This backports the rest of the decisions / System One launch:
litellm.systemone/asystemoneown the System One request shape (state+ namedquestions), whilelitellm.decisionsis narrowed to the OpenAI Decisions shape (input+ orderedquestions). Shared prepare/send logic moves tolitellm/decisions/call.py, with a deprecation warning ifstate=is still passed todecisions.Provider and transport work adds Databricks and hosted_vllm decision configs, expands Strands Decider for Bedrock AgentCore runtime ARNs (AWS signing, session headers, runtime error mapping), and threads
litellm_paramsthrough the decisions HTTP path.safety_identifieris honored only when the provider supports it; otherwise callers getUnsupportedParamsErrorunlessdrop_paramsdrops it. OpenRouter always advertises reasoning params, surfaces provider-reported cost on decisions/responses/pass-through, and bills fromusage.costwhen present.Proxy/router routes
/systemoneviaasystemone(distinct call type from/decisions), documents both endpoints in OpenAPI with inlined schemas, uses provider-specific health-check questions, blocks calleragentcore_runtime_session_id, and fully reconciles deployments when an auto-router model is missing from the in-memory registry. Complexity router can use Databricks as an OSS classifier backend.Reviewed by Cursor Bugbot for commit d5b0ad0. Bugbot is set up for automated code reviews on this repo. Configure here.