initialize ruff and mypy - #51
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughCI and tooling moved to Python 3.12 and mypy configured; backend interfaces extended (new search_namespaces) and filesystem backend refactored to a FilesystemNamespace model; KaizenClient gains namespace/entity listing helpers; assorted typing, lint, demo, plugin logging, docs, and small formatting updates across the repo. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as KaizenClient
participant Backend as BaseEntityBackend
participant FS as Filesystem (JSON)
Client->>Backend: all_namespaces(limit)
Backend->>FS: read namespace file(s)
FS-->>Backend: JSON content
Backend->>Backend: parse -> FilesystemNamespace model
Backend-->>Client: list[Namespace]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 3: Revert the manual change to the version field in pyproject.toml (the
line setting version = "0.2.0") so the file returns to the state before this
commit, and do not commit any manual version bumps; instead trigger the existing
GitHub release workflow (.github/workflows/release-github.yaml) with the
appropriate bump argument (e.g., bump-version: minor) so python-semantic-release
performs the version update, commit, and tag automatically.
Update uv.lock update version version downgrade
e8dfd01 to
0bc3789
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/unit/test_client.py (1)
146-158:⚠️ Potential issue | 🟠 Major
test_get_all_entitiesis an exact duplicate oftest_search_entitiesand tests the wrong method.Both tests mock
search_entities, callkaizen_client.search_entities(namespace_id="foobar", query="name")identically, and assert identical outcomes. Thetest_get_all_entitiestest should callkaizen_client.get_all_entities(namespace_id="foobar")instead, sinceget_all_entitiesinternally delegates tosearch_entitieswithquery=None.kaizen/db/sqlite_manager.py (2)
52-75:⚠️ Potential issue | 🟠 MajorMissing ROLLBACK when
IntegrityErroris caught.Line 58 issues an explicit
BEGIN. WhenIntegrityErroris caught on line 69, the exception is re-raised asNamespaceAlreadyExistsExceptionbut the transaction opened byBEGINis never rolled back. The next method that callsBEGINon the same connection will fail with "cannot start a transaction within a transaction".Proposed fix
except sqlite3.IntegrityError as e: + self.connection.execute("ROLLBACK") raise NamespaceAlreadyExistsException(f'Namespace "{namespace_id}" already exists.') from e
34-50:⚠️ Potential issue | 🔴 CriticalLock reuse causes deadlock; missing ROLLBACK on IntegrityError leaves transaction open.
Two issues in this class:
Deadlock in
reset(): Holdsself._lockat line 124, then calls_create_namespace_table()at line 129, which immediately tries to acquire the same non-reentrantthreading.Lock→ deadlock. Extract the table-creation logic into an unlocked helper method (e.g.,_create_namespace_table_unlocked()) called by bothreset()and__enter__()while already holding the lock. Or switch tothreading.RLock.Missing ROLLBACK in
create_namespace(): Whensqlite3.IntegrityErroris caught at line 70, no ROLLBACK is executed. TheBEGINtransaction from line 57 remains open. The next operation attemptingBEGINwill fail with "cannot start a transaction within a transaction." Addself.connection.execute("ROLLBACK")in theIntegrityErrorhandler before raising.kaizen/frontend/client/kaizen_client.py (1)
39-41:⚠️ Potential issue | 🟡 MinorIncorrect docstring — copy-paste from
get_namespace_details.The docstring says "Get details about a specific namespace." but this method lists all namespaces.
Fix
def all_namespaces(self, limit: int = 10) -> list[Namespace]: - """Get details about a specific namespace.""" + """List all namespaces.""" return self.backend.search_namespaces(limit)
🤖 Fix all issues with AI agents
In `@kaizen/sync/phoenix_sync.py`:
- Around line 188-200: The parsing logic in the block using self._parse_content
assumes every element has a "message" key which can raise KeyError and cause all
parsed outputs to be discarded; change the list comprehensions that build
output_msgs (the branch for parsed_output being a list and the branch for
parsed_output["choices"]) to safely filter and extract only items that actually
contain "message" (e.g., use a conditional comprehension or .get and check for
None) and preserve any valid messages; ensure the fallback still creates
output_msgs as [{"role": "assistant", "content": output_val}] when nothing valid
is found, and keep the existing exception logging via logger.debug with
self._format_payload_summary(output_val).
🧹 Nitpick comments (17)
.github/workflows/check-code.yaml (1)
1-1: Workflow name is now misleading.The workflow is named "Check Formatting with Ruff" but it now covers lockfile validation, linting, formatting, and type-checking (mypy). Consider renaming to something broader, e.g., "Code Quality Checks".
Proposed fix
-name: Check Formatting with Ruff +name: Code Quality Checksexamples/low_code/manual_phoenix_demo.py (2)
8-9: Relative path inload_dotenvis fragile.
"../../../.env"assumes the script is run from its own directory. If invoked from a different working directory, this silently won't load. This is acceptable for a demo, but consider usingpathlib.Path(__file__).resolve().parents[3] / ".env"for robustness if this becomes a shared pattern.
71-72: Moveimport jsonto the top of the file.Placing imports inside a loop body is unconventional. While Python caches modules so there's no runtime cost, moving it to the top-level imports (line 1–5) is more idiomatic.
Proposed fix
At the top of the file:
import os +import json from dotenv import load_dotenvIn the loop body:
if tc.function.name == "add": - import json - args = json.loads(tc.function.arguments)kaizen/llm/tips/tips.py (1)
97-97: Nit: use asetliteral for the membership test.
in ["action", "reasoning"]performs a linear scan; asetliteral is both idiomatic and (marginally) faster.Suggested change
- "num_steps": len([s for s in agent_steps if s["type"] in ["action", "reasoning"]]), + "num_steps": len([s for s in agent_steps if s["type"] in {"action", "reasoning"}]),plugins/kaizen/skills/recall/scripts/retrieve_entities.py (1)
38-55: Module-level logging executes at import time.
log("Script started")and the environment/argument logging blocks run unconditionally when the module is loaded, not just when invoked as__main__. If this file is ever imported as a library (e.g., for testing or reuse ofload_entities), these side effects will fire. Consider moving them insidemain()or guarding them withif __name__ == "__main__".plugins/kaizen/skills/learn/scripts/save_entities.py (2)
41-42: Same module-level side effect concern as inretrieve_entities.py.
log("Script started")runs at import time. Since this script is primarily invoked as__main__, the practical risk is low, but it's worth guarding withif __name__ == "__main__"for hygiene — especially iffind_entities_fileorload_existing_entitiesare ever reused via import.
18-39: Extract duplicated_get_log_dir(),LOG_FILE, andlog()into a shared logging module.These three entities are duplicated verbatim across
retrieve_entities.pyandsave_entities.py, differing only in the[retrieve]vs[save]log tag. Refactor them intoplugins/kaizen/skills/_logging.pywith the tag as a parameter tolog(). This eliminates duplication and provides a single point for future changes to log format, rotation, or security hardening.♻️ Sketch of a shared logging utility
# plugins/kaizen/skills/_logging.py import datetime import getpass import os import tempfile def _get_log_dir(): """Get user-scoped log directory with restrictive permissions.""" try: uid = os.getuid() except AttributeError: uid = getpass.getuser() log_dir = os.path.join(tempfile.gettempdir(), f"kaizen-{uid}") os.makedirs(log_dir, mode=0o700, exist_ok=True) return log_dir LOG_FILE = os.path.join(_get_log_dir(), "kaizen-plugin.log") def log(message, tag="general"): """Append a timestamped message to the log file.""" if not os.environ.get("KAIZEN_DEBUG"): return timestamp = datetime.datetime.now().strftime("%Y-%m-%d %H:%M:%S") with open(LOG_FILE, "a", encoding="utf-8") as f: f.write(f"[{timestamp}] [{tag}] {message}\n")Then in each script:
from kaizen.skills._logging import log # usage: log("Script started", tag="save")kaizen/sync/phoenix_sync.py (2)
196-198: Remove the dead# passcomment.This appears to be a leftover from a previous revision. It adds noise without value.
Proposed fix
else: # Fallback for simple string output output_msgs = [{"role": "assistant", "content": output_val}] - # pass
206-229: Consider extracting the duplicated message-parsing loop into a helper.The block at lines 158–182 (input messages) and lines 206–229 (output messages) are nearly identical — both parse individual message strings, extract
role/content/tool_callsvia dual-key lookup, and build the mapped message dict. The only difference is the"type"field ("prompt"vs"completion"). A small private method like_parse_message_list(raw_msgs, msg_type)would reduce duplication and make future key-mapping changes less error-prone.examples/low_code/simple_openai.py (1)
19-19: Consider breaking this long line for readability.This single-line call is quite wide (~120 characters). If Ruff's configured line length permits it, this is technically fine, but the previous multi-line form was arguably more readable for an example file meant to onboard users.
♻️ Suggested multi-line formatting
- response = client.chat.completions.create(model=model, messages=[{"role": "user", "content": "What is 2 + 2?"}], max_tokens=10) + response = client.chat.completions.create( + model=model, + messages=[{"role": "user", "content": "What is 2 + 2?"}], + max_tokens=10, + )kaizen/llm/conflict_resolution/conflict_resolution.py (1)
46-62:prompt_filevariable is reused for two different template paths.
prompt_fileis assigned on Line 52 (default conflict resolution prompt) and then reassigned on Line 60 (main conflict resolution template). While functionally fine since the first value is consumed immediately, it can be confusing during debugging. Consider a distinct name likedefault_prompt_fileon Line 52.♻️ Suggested rename for clarity
if custom_update_entities_prompt is None: - prompt_file = Path(__file__).parent / "prompts/default_conflict_resolution.jinja2" - custom_update_entities_prompt = Template(prompt_file.read_text()).render() + default_prompt_file = Path(__file__).parent / "prompts/default_conflict_resolution.jinja2" + custom_update_entities_prompt = Template(default_prompt_file.read_text()).render()kaizen/db/sqlite_manager.py (1)
112-118:delete_namespacelacks error handling around the transaction.Unlike
create_namespaceandreset,delete_namespacehas notry/excepttoROLLBACKif theDELETEorCOMMITfails. While unlikely for a simple PK delete, it's inconsistent and leaves the connection in a broken transaction state on failure.Proposed fix
def delete_namespace(self, namespace_id: str): assert self._lock is not None assert self.connection is not None with self._lock: - self.connection.execute("BEGIN") - self.connection.execute("DELETE FROM namespaces WHERE id = ?", (namespace_id,)) - self.connection.execute("COMMIT") + try: + self.connection.execute("BEGIN") + self.connection.execute("DELETE FROM namespaces WHERE id = ?", (namespace_id,)) + self.connection.execute("COMMIT") + except Exception as e: + self.connection.execute("ROLLBACK") + logger.error(f"Failed to delete namespace: {e}") + raisedemo/filesystem/server.py (2)
375-387: Loop variablepathshadows the function parameter.Line 377
for path in entries:reuses the name of the function parameter (pathon line 356). Whilevalid_pathhas already captured the resolved value, this shadowing is confusing and would be flagged by linters like Ruff'sA001/PLW1509or Pylint.Rename the loop variable
- for path in entries: - entry_path = os.path.join(valid_path, path) + for entry_name in entries: + entry_path = os.path.join(valid_path, entry_name) is_dir = os.path.isdir(entry_path) try: stats = os.stat(entry_path) size = stats.st_size if not is_dir else 0 except Exception: size = 0 - detailed_entries.append({"name": path, "is_dir": is_dir, "size": size}) + detailed_entries.append({"name": entry_name, "is_dir": is_dir, "size": size})
13-13: Inconsistent use ofList[str](typing) vslist[str](built-in).The file imports
Listfromtypingand uses it in function signatures (e.g., line 162List[str] | None), but also uses the lowercaselist[str]in type annotations (lines 373, 376). Since Python 3.9+ supportslist[...]natively and this PR targets 3.12, consider replacing allListusages with the built-inlistfor consistency.Also applies to: 162-162, 417-417, 493-493
kaizen/frontend/client/kaizen_client.py (1)
39-49:all_namespacesandsearch_namespacesare identical in implementation.Both delegate to
self.backend.search_namespaces(limit)with the same signature and default. Consider keeping only one, or differentiating their intent (e.g.,search_namespacescould accept filter criteria in the future).kaizen/backend/filesystem.py (2)
99-117:search_namespacesuses rawjson.loads+ dict access, inconsistent with other methods.Every other data-loading path uses
_load_namespace_data()→FilesystemNamespace.model_validate(), but this method parses JSON manually and accessesdata["id"],data["entities"], etc. directly. If the schema evolves (e.g., a field is renamed or gains a validator), this path won't benefit from the model.Consider reusing
FilesystemNamespace.model_validate(data)here, with the sametry/exceptfor fault tolerance:Proposed fix
def search_namespaces(self, limit: int = 10) -> list[Namespace]: """Search for namespaces.""" namespaces = [] with self._lock: for file_path in self.data_dir.glob("*.json"): try: - data = json.loads(file_path.read_text()) + data = FilesystemNamespace.model_validate(json.loads(file_path.read_text())) namespaces.append( Namespace( - id=data["id"], - created_at=datetime.datetime.fromisoformat(data["created_at"]), - num_entities=len(data["entities"]), + id=data.id, + created_at=data.created_at, + num_entities=len(data.entities), ) ) - except (json.JSONDecodeError, KeyError): + except (json.JSONDecodeError, KeyError, Exception): continue
68-87:num_entitiesstored in JSON will be stale after entity mutations.
create_namespacesetsnum_entities=0on line 83, butupdate_entitiesanddelete_entity_by_idnever updatedata.num_entitiesbefore calling_save_namespace_data. The stored value drifts fromlen(data.entities).Currently safe because all readers recompute
num_entitiesdynamically fromlen(data.entities), but the stale field in the JSON is misleading. Consider either droppingnum_entitiesfromFilesystemNamespace(since it's always derived) or updating it before save.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / UX
Chores
Documentation