fix: skip prompt cache hints for Bedrock Nova and add per-model override - #2348
fix: skip prompt cache hints for Bedrock Nova and add per-model override#2348MingyuLiNCU wants to merge 5 commits into
Conversation
Bedrock Nova rejects the cachePoint field that litellm's
cache_control_injection_points translates into on Converse requests
("extraneous key [cachePoint] is not permitted"), breaking all Nova
usage. The Gemini-only exception added in HolmesGPT#2069 did not cover it.
- Add _supports_prompt_caching() as the single extension point for
routes that reject cache-control hints (Gemini + Bedrock Nova); new
incompatible models are one line in one place.
- Add ModelEntry.cache_control (Optional[bool]) so users can force
prompt caching on/off per model from model_list.yaml, covering
models the built-in defaults don't know about yet.
Unit tests extend tests/core/test_llm_completion_cache_control.py:
Nova routes skip the hint, Bedrock Claude still gets it, and the
cache_control override wins in both directions.
Signed-off-by: 掌心扑火 <li13031513035@163.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe LLM now detects prompt-cache support by route. Gemini and Bedrock Nova disable cache hints by default. A per-model ChangesPrompt-caching route control
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/core/test_llm_completion_cache_control.py`:
- Around line 123-126: Update the test docstring describing the per-model
cache_control override so it states that cache_control: true forces the cache
hint for models whose automatic defaults suppress it, including Bedrock Nova
models; remove the outdated “not covered by the default yet” wording.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15740e31-950c-4e22-8cba-79dc13486632
📒 Files selected for processing (2)
holmes/core/llm.pytests/core/test_llm_completion_cache_control.py
…vered by default) Signed-off-by: 掌心扑火 <li13031513035@163.com>
Summary
Fixes #2332 — Bedrock Nova models fail with
extraneous key [cachePoint] is not permittedon every completion call, because the prompt-cache hint thatLiteLLM translates into the Converse
cachePointfield was only skipped forGemini routes (#2069) and never for Nova.
Root cause
cache_control_injection_pointsis injected unconditionally for everynon-Gemini route in
holmes/core/llm.py:On Bedrock, LiteLLM sends this hint as the Converse API's
cachePointfield,which the Amazon Nova family rejects. Bedrock Claude (which supports
cachePoint) was never affected, which is why the exception list stayedGemini-only.
Changes
Two complementary parts, both in
holmes/core/llm.py:A single capability check instead of per-model exceptions —
new
_supports_prompt_caching(litellm_model_name)returns False for theGemini routes (unchanged behavior) and for Bedrock
amazon.nova*models.All route-specific decisions now live in one place, so the next
incompatible model is a one-line addition rather than another
ifat thecall site.
An explicit per-model override —
ModelEntrygains acache_control: Optional[bool]field. Users can force prompt cachingon/off from
model_list.yaml, covering models neither the defaults norLiteLLM know about yet (same shape as the temperature issue raised for
Bedrock Opus 4.7).
Tests
Extended
tests/core/test_llm_completion_cache_control.py:us.amazon.nova-pro/lite/micro-v1:0, plus a regionalvariant) get no
cache_control_injection_pointskwargcache_control: true|falseoverride wins in both directions, includingforcing the hint on for Nova
cache_controlnever leaks fromModelEntry/DefaultLLM.argsinto thelitellm call
All tests are offline (mocked
litellm.completion), following the existingpattern from the #2069 tests.
Verification
pytest tests/core/test_llm_completion_cache_control.pyand the LLM-relatedregression suite: all pass locally
Summary by CodeRabbit
New Features
Bug Fixes