fix: guard against cached context_length=0 poisoning resolution chain - #231
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The context window display in Hermes can show a broken `? (est)` or `ctx --` value when the persistent context-length cache contains a 0 entry. This happens because `get_model_context_length()` has a guard `if cached is not None` — but `0 is not None` evaluates to `True`, so the function returns 0 instead of falling through to the hardcoded defaults (e.g. 1M for `deepseek-v4-pro`).
The zero propagates to `ContextCompressor.context_length`, which then causes every downstream display code path to fail:
While `save_context_length()` is never intentionally called with 0, a corrupted cache file (manual edit, YAML parse glitch, or a future buggy probe) can introduce a 0 entry and permanently poison the resolution chain.
What changed
Two-layer defense in `agent/model_metadata.py`:
`get_model_context_length()`: After retrieving a cached value, check `cached <= 0` before checking the provider-specific invalidation chain. Non-positive values are logged as a warning, invalidated from the cache, and the function falls through to normal resolution (which ends at the hardcoded 256K fallback).
`save_context_length()`: Added a guard that refuses to persist values `<= 0`, logging a warning. This prevents new poison entries from ever being written.
How to review
Evidence
Manual testing:
```
Test 1: save rejects 0
save_context_length('test-model', 'https://api.test.com', 0)
→ "Refusing to cache non-positive context length ..."
→ get_cached_context_length('test-model', 'https://api.test.com') → None ✓
Test 2: cached 0 handling
Manually wrote 0 to cache YAML
get_model_context_length('test-model', base_url='https://api.test-zero.com')
→ "Dropping non-positive cache entry ..."
→ Returns 256,000 (DEFAULT_FALLBACK_CONTEXT) ✓
→ Cache entry auto-invalidated ✓
Test 3: normal resolution still works
get_model_context_length('deepseek-v4-pro', base_url='https://api.deepseek.com', provider='deepseek')
→ 1,000,000 ✓
```
Test suite:
Verification
Risks / gaps
Fork counterpart of upstream PR NousResearch#25812. Merging this fork PR so the packaged app ships; upstream PR remains awaiting upstream review.