Add readonly mutation protection to models #747
Conversation
scosman
changed the title
WalkthroughKilnBaseModel gains read-only protection via a Changes
Sequence DiagramsequenceDiagram
participant User
participant KilnBaseModel
participant ModelCache
participant Storage
rect rgb(200,220,240)
Note over User,KilnBaseModel: Load & Cache as Read-Only
User->>KilnBaseModel: load_from_file(path, readonly=True)
KilnBaseModel->>Storage: read file
KilnBaseModel->>KilnBaseModel: mark_as_readonly()
KilnBaseModel->>ModelCache: set_model(path, model)
ModelCache->>ModelCache: assert model._readonly
ModelCache-->>ModelCache: store readonly instance
end
rect rgb(240,220,200)
Note over User,ModelCache: Retrieve Mutable Copy
User->>ModelCache: get_model(path, readonly=False)
ModelCache->>ModelCache: fetch readonly instance
ModelCache->>KilnBaseModel: mutable_copy()
KilnBaseModel->>KilnBaseModel: deep copy + _readonly=False
KilnBaseModel-->>ModelCache: mutable instance
ModelCache-->>User: return mutable instance
end
rect rgb(220,240,200)
Note over User,KilnBaseModel: Prevent Mutation on Readonly
User->>KilnBaseModel: setattr(...)
KilnBaseModel->>KilnBaseModel: _ensure_not_readonly()
KilnBaseModel-->>User: raise ReadOnlyMutationError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Path-based instructions (4)libs/{core,server}/**/*.py📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Files:
{libs,app/desktop}/**/*.py📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Files:
libs/core/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
{libs/core,libs/server,app/desktop}/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)libs/core/kiln_ai/datamodel/basemodel.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
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 |
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEAD
Summary
Line-by-lineView line-by-line diff coveragelibs/core/kiln_ai/datamodel/model_cache.pyLines 18-28 18 from pathlib import Path
19 from typing import TYPE_CHECKING, Dict, Optional, Tuple, Type, TypeVar
20
21 if TYPE_CHECKING:
! 22 from kiln_ai.datamodel.basemodel import KilnBaseModel
23
! 24 T = TypeVar("T", bound="KilnBaseModel")
25 else:
26 T = TypeVar("T")
27 Lines 93-101 93 if not self._enabled:
94 return
95
96 if not model._readonly:
! 97 raise RuntimeError(
98 "Mutable models are not allowed to be cached. Model should be readonly."
99 )
100 self.model_cache[path] = (model, mtime_ns)
|
…ed model that doesn't match disk gets into cache. Smaller tweaks on params
scosman
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (6)
libs/core/kiln_ai/datamodel/basemodel.py (3)
304-314: Avoid whitelisting ‘path’ for readonly mutation.Allowing path changes on cached readonly instances can lead to inconsistent cache semantics and surprising behavior.
Recommendation: only whitelist 'parent'. If you still need to set path during load/save flows, do it before mark_as_readonly() or via object.setattr internally.
- readonly_safe_attrs = {"parent", "path"} + readonly_safe_attrs = {"parent"}
350-399: Safer model_type check to avoid KeyError and clearer error message.parsed_json["model_type"] will KeyError if the field is missing. Prefer get() and handle None.
- if parsed_json["model_type"] != cls.type_name(): + if parsed_json.get("model_type") != cls.type_name(): raise ValueError( - f"Cannot load from file because the model type is incorrect. Expected {cls.type_name()}, got {parsed_json['model_type']}. " + f"Cannot load from file because the model type is incorrect or missing. Expected {cls.type_name()}, " + f"got {parsed_json.get('model_type')!r}. " f"Class: {m.__class__.__name__}, id: {getattr(m, 'id', None)}, path: {path}, " f"version: {m.v}, max version: {m.max_schema_version()}" )
420-451: Block save_to_file on readonly instances.Saving from a readonly (cached) instance violates the “don’t mutate cached objects” guarantee and can cause subtle bugs.
def save_to_file(self) -> None: """Save the model instance to a file. Raises: ValueError: If the path is not set """ + if getattr(self, "_readonly", False): + raise ReadOnlyMutationError( + "Cannot save a readonly model. Call mutable_copy() first." + )libs/core/kiln_ai/datamodel/test_basemodel.py (2)
417-430: Avoid asserting private _readonly; prefer a public accessor.Accessing internals makes tests brittle. Consider adding is_readonly property/method and assert via that.
Example:
+ assert getattr(model, "is_readonly", lambda: model._readonly)() is TrueOr, add to KilnBaseModel:
+ @property + def is_readonly(self) -> bool: + return self._readonly
1028-1121: Add coverage for in-place container mutations on readonly models.Currently not tested; without freezing, list/dict/set fields can still be mutated. Please add a test that attempts list.append, dict assignment, and set.add on a readonly instance and expects ReadOnlyMutationError (or other failure) once freeze is implemented.
libs/core/kiln_ai/datamodel/test_model_cache.py (1)
59-62: Use st_mtime_ns consistently to avoid unintended invalidation in tests.Several tests pass st_mtime (seconds) to set_model, but the cache compares st_mtime_ns, causing spurious invalidations and potentially measuring the wrong path in benchmarks.
- Replace st_mtime with st_mtime_ns in:
- Line 59 (test_invalidate_model), 71 (test_clear_cache), 135 (test_benchmark_get_model), and similar occurrences.
- Keep st_mtime only in tests explicitly verifying invalidation due to mtime changes.
Also applies to: 71-76, 135-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/core/kiln_ai/datamodel/basemodel.py(3 hunks)libs/core/kiln_ai/datamodel/model_cache.py(2 hunks)libs/core/kiln_ai/datamodel/test_basemodel.py(5 hunks)libs/core/kiln_ai/datamodel/test_model_cache.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/{core,server}/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Python 3.10+ for library and server code under libs/core and libs/server
Files:
libs/core/kiln_ai/datamodel/test_basemodel.pylibs/core/kiln_ai/datamodel/basemodel.pylibs/core/kiln_ai/datamodel/test_model_cache.pylibs/core/kiln_ai/datamodel/model_cache.py
{libs,app/desktop}/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
{libs,app/desktop}/**/*.py: Use Pydantic v2 (not v1) in Python code that models/validates data
Use explicit type hints for functions, classes, and public APIs in Python code
Files:
libs/core/kiln_ai/datamodel/test_basemodel.pylibs/core/kiln_ai/datamodel/basemodel.pylibs/core/kiln_ai/datamodel/test_model_cache.pylibs/core/kiln_ai/datamodel/model_cache.py
libs/core/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code in libs/core must be compatible with Python 3.10+
Files:
libs/core/kiln_ai/datamodel/test_basemodel.pylibs/core/kiln_ai/datamodel/basemodel.pylibs/core/kiln_ai/datamodel/test_model_cache.pylibs/core/kiln_ai/datamodel/model_cache.py
{libs/core,libs/server,app/desktop}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{libs/core,libs/server,app/desktop}/**/*.py: Use Pydantic v2 APIs (e.g., BaseModel v2, model_validate/model_dump) and avoid v1-only patterns
Maintain strong typing in Python code (add type hints and run type checking)
Files:
libs/core/kiln_ai/datamodel/test_basemodel.pylibs/core/kiln_ai/datamodel/basemodel.pylibs/core/kiln_ai/datamodel/test_model_cache.pylibs/core/kiln_ai/datamodel/model_cache.py
🧬 Code graph analysis (4)
libs/core/kiln_ai/datamodel/test_basemodel.py (2)
libs/core/kiln_ai/datamodel/basemodel.py (6)
ReadOnlyMutationError(44-47)KilnBaseModel(265-469)load_from_file(351-399)mark_as_readonly(291-293)mutable_copy(320-325)save_to_file(420-450)libs/core/kiln_ai/datamodel/model_cache.py (2)
get_model(68-79)set_model(91-100)
libs/core/kiln_ai/datamodel/basemodel.py (1)
libs/core/kiln_ai/datamodel/model_cache.py (3)
ModelCache(29-131)shared(43-46)set_model(91-100)
libs/core/kiln_ai/datamodel/test_model_cache.py (2)
libs/core/kiln_ai/datamodel/basemodel.py (2)
KilnBaseModel(265-469)mark_as_readonly(291-293)libs/core/kiln_ai/datamodel/model_cache.py (5)
ModelCache(29-131)set_model(91-100)get_model(68-79)invalidate(102-104)clear(106-107)
libs/core/kiln_ai/datamodel/model_cache.py (2)
libs/core/kiln_ai/datamodel/basemodel.py (3)
KilnBaseModel(265-469)mutable_copy(320-325)model_type(288-289)libs/core/kiln_ai/datamodel/test_model_cache.py (1)
model_cache(24-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (macos-13)
🔇 Additional comments (4)
libs/core/kiln_ai/datamodel/model_cache.py (2)
68-79: get_model semantics look good.Shared instance for readonly=True and mutable_copy() otherwise is correct and matches the new contract.
91-100: Good guard: disallow caching mutable models.Runtime check on _readonly prevents accidental caching of mutable instances.
libs/core/kiln_ai/datamodel/test_model_cache.py (2)
169-199: Readonly copy vs mutable copy assertions look solid.Validates that readonly returns the same instance and mutable returns a deep copy with _readonly False.
335-346: Good negative test: caching mutable models should raise.Confirms runtime guard in ModelCache.set_model.
What does this PR do?
This PR introduces a readonly protection mechanism for
KilnBaseModelinstances. When aKilnBaseModelis marked as readonly, any attempts to mutate its attributes will raise aReadOnlyMutationError. This prevents accidental modification of cached models, ensuring data integrity.The
ModelCacheis updated to automatically mark cached models as readonly. Callers who require a mutable copy can obtain one using the newmutable_copy()method, which is designed to create a deep copy with the readonly flag reset.Related Issues
None.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores