Fix pricing cache/shared-dict bugs and a mistyped terminal error handler - #232
Open
RasAlGhul96 wants to merge 1 commit into
Open
Fix pricing cache/shared-dict bugs and a mistyped terminal error handler#232RasAlGhul96 wants to merge 1 commit into
RasAlGhul96 wants to merge 1 commit into
Conversation
Found during an audit of core/pricing.py, error_handling.py, and terminal/manager.py: - calculate_cost's cache key omitted `strict`, so a strict=False lookup for an unknown model could cache a result that a later strict=True lookup for the same tokens would silently reuse instead of raising. - _get_pricing_for_model filled in missing cache_creation/cache_read fields by mutating the pricing dict in place. Several model aliases (e.g. claude-opus-4-5..4-8) share the same FALLBACK_PRICING dict object, so that mutation could leak across aliases (and into FALLBACK_PRICING itself) once a pricing dict without those fields was used. Replaced with a copy-on-write helper. - handle_error_and_exit declared Union[Exception, str] but always did `raise error`, which would raise TypeError if a str was ever passed. The only caller always passes an Exception, so the str branch was dead and misleading; narrowed the type. - report_error's fallback `except Exception: pass` swallowed logger failures with zero visibility; now writes a last-resort line to stderr first. - Uncommented the Windows PyPI classifier, which was inconsistent with the existing win32-specific tzdata/tzlocal deps and WSL path support. Verified with the existing test suite (no new failures) and mypy on the touched files.
📝 WalkthroughWalkthroughChangesPricing cache handling
Error handling contracts
Package metadata
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/claude_monitor/core/pricing.py`:
- Around line 205-222: Update _ensure_cache_pricing to return the copied pricing
dictionary after adding missing cache fields without assigning it back through
self.pricing[key]. Preserve the existing fast path for pricing entries that
already contain both cache fields, and avoid mutating the caller-provided
custom_pricing dictionary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 422bd3e4-8723-45a2-9edd-237df2879672
📒 Files selected for processing (4)
pyproject.tomlsrc/claude_monitor/core/pricing.pysrc/claude_monitor/error_handling.pysrc/claude_monitor/terminal/manager.py
Comment on lines
+205
to
+222
| def _ensure_cache_pricing(self, key: str) -> Dict[str, float]: | ||
| """Return self.pricing[key] guaranteed to have cache fields. | ||
|
|
||
| Several keys (e.g. the claude-opus-4-5..4-8 aliases) point at the | ||
| *same* FALLBACK_PRICING dict object. Filling in missing cache fields | ||
| in place would mutate that shared object for every alias (and, for | ||
| FALLBACK_PRICING itself, globally). Copy-on-write instead: only | ||
| allocate a new dict when a field is actually missing. | ||
| """ | ||
| pricing = self.pricing[key] | ||
| if "cache_creation" in pricing and "cache_read" in pricing: | ||
| return pricing | ||
| pricing = dict(pricing) | ||
| pricing.setdefault("cache_creation", pricing["input"] * 1.25) | ||
| pricing.setdefault("cache_read", pricing["input"] * 0.1) | ||
| self.pricing[key] = pricing | ||
| return pricing | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid mutating the outer self.pricing dictionary.
Assigning self.pricing[key] = pricing mutates the outer dictionary. If self.pricing was initialized with a user-provided custom_pricing dictionary in __init__ (via self.pricing = custom_pricing or ...), this will unexpectedly mutate the user's object, creating a side effect.
Since cost calculations are already cached at the calculate_cost level (via self._cost_cache), memoizing the patched pricing dictionary here is unnecessary and risks modifying user data. Consider returning the copied dictionary without writing it back.
💡 Proposed fix to avoid side effects
def _ensure_cache_pricing(self, key: str) -> Dict[str, float]:
"""Return self.pricing[key] guaranteed to have cache fields.
Several keys (e.g. the claude-opus-4-5..4-8 aliases) point at the
*same* FALLBACK_PRICING dict object. Filling in missing cache fields
in place would mutate that shared object for every alias (and, for
FALLBACK_PRICING itself, globally). Copy-on-write instead: only
allocate a new dict when a field is actually missing.
"""
pricing = self.pricing[key]
if "cache_creation" in pricing and "cache_read" in pricing:
return pricing
- pricing = dict(pricing)
- pricing.setdefault("cache_creation", pricing["input"] * 1.25)
- pricing.setdefault("cache_read", pricing["input"] * 0.1)
- self.pricing[key] = pricing
- return pricing
+ patched_pricing = dict(pricing)
+ patched_pricing.setdefault("cache_creation", patched_pricing["input"] * 1.25)
+ patched_pricing.setdefault("cache_read", patched_pricing["input"] * 0.1)
+ return patched_pricing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _ensure_cache_pricing(self, key: str) -> Dict[str, float]: | |
| """Return self.pricing[key] guaranteed to have cache fields. | |
| Several keys (e.g. the claude-opus-4-5..4-8 aliases) point at the | |
| *same* FALLBACK_PRICING dict object. Filling in missing cache fields | |
| in place would mutate that shared object for every alias (and, for | |
| FALLBACK_PRICING itself, globally). Copy-on-write instead: only | |
| allocate a new dict when a field is actually missing. | |
| """ | |
| pricing = self.pricing[key] | |
| if "cache_creation" in pricing and "cache_read" in pricing: | |
| return pricing | |
| pricing = dict(pricing) | |
| pricing.setdefault("cache_creation", pricing["input"] * 1.25) | |
| pricing.setdefault("cache_read", pricing["input"] * 0.1) | |
| self.pricing[key] = pricing | |
| return pricing | |
| def _ensure_cache_pricing(self, key: str) -> Dict[str, float]: | |
| """Return self.pricing[key] guaranteed to have cache fields. | |
| Several keys (e.g. the claude-opus-4-5..4-8 aliases) point at the | |
| *same* FALLBACK_PRICING dict object. Filling in missing cache fields | |
| in place would mutate that shared object for every alias (and, for | |
| FALLBACK_PRICING itself, globally). Copy-on-write instead: only | |
| allocate a new dict when a field is actually missing. | |
| """ | |
| pricing = self.pricing[key] | |
| if "cache_creation" in pricing and "cache_read" in pricing: | |
| return pricing | |
| patched_pricing = dict(pricing) | |
| patched_pricing.setdefault("cache_creation", patched_pricing["input"] * 1.25) | |
| patched_pricing.setdefault("cache_read", patched_pricing["input"] * 0.1) | |
| return patched_pricing |
🤖 Prompt for 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.
In `@src/claude_monitor/core/pricing.py` around lines 205 - 222, Update
_ensure_cache_pricing to return the copied pricing dictionary after adding
missing cache fields without assigning it back through self.pricing[key].
Preserve the existing fast path for pricing entries that already contain both
cache fields, and avoid mutating the caller-provided custom_pricing dictionary.
ClaimVerifier: blocking findingsThe following file claims in commit messages / PR body were not actually modified on this branch:
Please update the claim or add the missing changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Found while doing a source-level audit of
core/pricing.py,error_handling.py, andterminal/manager.py. No behavior change for the currently-shipped pricing tables, but these are latent correctness bugs:core/pricing.py— cache key ignoredstrict.calculate_cost's cache key wasf"{model}:{input}:{output}:{cache_creation}:{cache_read}", withoutstrict. Astrict=Falselookup for an unknown model caches a$0result; a laterstrict=Truelookup for the same token counts then silently returns that cached value instead of raisingKeyError. Fixed by includingstrictin the cache key.core/pricing.py— shared pricing dicts mutated in place. Several model aliases (claude-opus-4-5..claude-opus-4-8, etc.) are assigned the sameFALLBACK_PRICING["opus"]dict object inself.pricing._get_pricing_for_modelfilled in missingcache_creation/cache_readkeys by mutating that dict in place, so the fix could leak across every alias sharing the object (and intoFALLBACK_PRICINGitself) the first time a pricing dict without those fields was used — e.g. viacustom_pricing. Replaced with a small copy-on-write helper (_ensure_cache_pricing) that only allocates a new dict when a field is actually missing.terminal/manager.py—handle_error_and_exittypedUnion[Exception, str]but always doesraise error. Passing astrwould raiseTypeError: exceptions must derive from BaseException, masking the original error. The only caller (cli/main.py) always passes anException, so thestrbranch was dead and misleading; narrowed the type toException.error_handling.py— silentexcept Exception: passin the logging fallback. If the logger call itself fails, the original error vanished with zero trace. Now writes a last-resort line tostderrbefore falling back topass.pyproject.toml— uncommented theOperating System :: Microsoft :: Windowsclassifier, which was inconsistent with the existing win32-specifictzdata/tzlocaldependencies and the dedicated WSL path support/tests already in the codebase.Test plan
pytest src/tests/test_pricing.py src/tests/test_error_handling.py— all greenpytest src/tests) — same 15 pre-existing failures as on unmodifiedmain(Windows/Python 3.14 locale & signal environment issues, verified viagit stashcomparison), no new failures introducedmypyon the touched files — no new errors (pre-existingHAS_TERMIOSredefinition warnings inthemes.py/manager.pyare unrelated to this change)🤖 Generated with Claude Code
Summary by CodeRabbit