fix(platform-integrations): add missing entity_io lib to Bob#105
Merged
Conversation
Bob's Python scripts import entity_io via sys.path but the lib/ directory was never included in the Bob kaizen-lite source or copied during install.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCopies Claude's Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer
participant ClaudeSrc as ClaudeSource
participant BobFS as .bob
participant Skill as BobSkill
participant KaizenLib as kaizen-lib/entity_io
Installer->>ClaudeSrc: locate kaizen-lite/lib
ClaudeSrc-->>Installer: return lib path
Installer->>BobFS: copy lib -> .bob/kaizen-lib
Installer-->>BobFS: update status (kaizen-lib present)
Skill->>BobFS: start and search upward for kaizen-lib
BobFS-->>Skill: return .bob/kaizen-lib path
Skill->>KaizenLib: import entity_io from .bob/kaizen-lib
KaizenLib-->>Skill: provide entity I/O functions (save/retrieve)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-integrations/bob/kaizen-lite/lib/entity_io.py`:
- Around line 258-265: The code normalizes entity_type for the directory but
still uses the original entity["type"] when serializing, causing on-disk
metadata mismatch; fix by persisting the normalized value into the entity before
serialization (e.g., assign entity["type"] = entity_type) or else update the
call to entity_to_markdown to accept/use the normalized entity_type so the
frontmatter and directory remain consistent (refer to the entity_type variable,
slugify call, and entity_to_markdown invocation).
- Around line 84-96: The code in get_default_entities_dir currently ignores the
KAIZEN_ENTITIES_DIR env var and only uses CLAUDE_PROJECT_ROOT or a local
.kaizen/entities; update get_default_entities_dir to first check
os.environ.get("KAIZEN_ENTITIES_DIR") and if set use Path(KAIZEN_ENTITIES_DIR)
(creating it with mkdir(parents=True, exist_ok=True)) and return its resolved
path; keep the existing fallback to CLAUDE_PROJECT_ROOT/.kaizen/entities and
then to .kaizen/entities so find_entities_dir()/save_entities logic will honor
KAIZEN_ENTITIES_DIR on first save.
- Around line 274-289: The current claiming logic around unique_filename/target
is racy and uses os.rename (which fails on Windows); change it to loop until you
successfully create the placeholder with os.open(..., os.O_CREAT | os.O_EXCL |
os.O_WRONLY) (retrying unique_filename() on FileExistsError) so the claim is
atomic, always close the claim_fd, then use os.replace(tmp_path, target) instead
of os.rename to move the temp file atomically cross-platform; keep the existing
cleanup for fd and tmp_path in the outer except block.
In `@platform-integrations/install.sh`:
- Around line 505-507: The install script currently copies the entire
bob_source_lite / "lib" tree into bob_target / "lib" via copy_tree which can
overwrite user files; change the install step that uses copy_tree (referenced by
copy_tree, bob_source_lite, bob_target and the .bob/lib target) to only copy the
specific Kaizen-owned files needed (e.g., entity_io.py used by save_entities.py)
or explicitly list and copy __init__.py as a managed file instead of the whole
directory, and update the uninstall logic (the code that removes the directory
around Line 544) to remove only those managed files rather than deleting the
entire .bob/lib directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5889a0c9-fcb7-4e50-b312-b52253773fc2
📒 Files selected for processing (3)
platform-integrations/bob/kaizen-lite/lib/__init__.pyplatform-integrations/bob/kaizen-lite/lib/entity_io.pyplatform-integrations/install.sh
…kaizen-lib Remove duplicate entity_io.py from Bob source — installer now copies from the canonical location in the plugin. Rename target dir from .bob/lib to .bob/kaizen-lib to avoid conflicts with other tools.
Addresses CodeRabbit review finding: Honor KAIZEN_ENTITIES_DIR when creating the store
Addresses CodeRabbit review finding: Persist the normalized type value
…rm compatibility Addresses CodeRabbit review finding: Replace the claiming logic with a loop and use os.replace()
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@platform-integrations/bob/kaizen-lite/skills/kaizen-recall/scripts/retrieve_entities.py`:
- Around line 9-10: The hardcoded upward traversal using
Path(__file__).resolve().parent.parent.parent.parent (used in the
sys.path.insert call in retrieve_entities.py) can miss cases where kaizen-lib is
installed under a different ancestor (e.g., .bob/kaizen-lib); replace that
single-line insertion with logic that walks the script's ancestors to locate a
directory named "kaizen-lib" and only insert that path when found (use
Path(__file__).resolve().parents to iterate and check (candidate /
"kaizen-lib").is_dir()), falling back to the original behavior or raising a
clear error if not found; update the sys.path.insert usage in
retrieve_entities.py to use this discovered lib_path so it mirrors the corrected
approach used for save_entities.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 714d38bd-0ab2-43aa-8442-62a7b7221695
📒 Files selected for processing (3)
platform-integrations/bob/kaizen-lite/skills/kaizen-learn/scripts/save_entities.pyplatform-integrations/bob/kaizen-lite/skills/kaizen-recall/scripts/retrieve_entities.pyplatform-integrations/install.sh
✅ Files skipped from review due to trivial changes (1)
- platform-integrations/install.sh
…izen-lib Addresses CodeRabbit review finding: Same path traversal issue as save_entities.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-integrations/claude/plugins/kaizen-lite/lib/entity_io.py (1)
282-298:⚠️ Potential issue | 🟠 MajorOrphaned 0-byte file on exception after successful claim.
If an exception occurs between
os.close(claim_fd)(line 287) andos.replace(tmp_path, target)(line 292), the exception handler cleans uptmp_pathbut leaves the 0-bytetargetfile behind. This can accumulate orphaned files over time if failures occur.Additionally, consider adding a retry limit to the
while Trueloop as a defensive measure, even thoughunique_filename()should eventually find a unique name.🐛 Proposed fix to clean up target on exception
# Atomically claim the target using O_EXCL; retry on race - while True: + target = None + for _ in range(1000): # Defensive retry limit target = unique_filename(type_dir, slug) try: claim_fd = os.open(str(target), os.O_CREAT | os.O_EXCL | os.O_WRONLY) os.close(claim_fd) break except FileExistsError: continue + else: + raise OSError(f"Could not claim unique filename after retries in {type_dir}") os.replace(tmp_path, target) return target except BaseException: if fd is not None: os.close(fd) if os.path.exists(tmp_path): os.unlink(tmp_path) + if target is not None and os.path.exists(target): + try: + os.unlink(target) + except OSError: + pass raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/claude/plugins/kaizen-lite/lib/entity_io.py` around lines 282 - 298, The code can leave a 0-byte orphaned target file if an exception occurs after os.close(claim_fd) and before os.replace(tmp_path, target); update the error handling so that in the exception block you also check for and unlink the claimed target (the `target` path) if it exists, ensuring claim_fd and fd are closed and tmp_path is removed; additionally add a retry limit to the loop that calls unique_filename (where claim is done with os.open / os.O_CREAT | os.O_EXCL) so the while True that generates `target` will bail with a clear error after N attempts instead of looping forever.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@platform-integrations/claude/plugins/kaizen-lite/lib/entity_io.py`:
- Around line 282-298: The code can leave a 0-byte orphaned target file if an
exception occurs after os.close(claim_fd) and before os.replace(tmp_path,
target); update the error handling so that in the exception block you also check
for and unlink the claimed target (the `target` path) if it exists, ensuring
claim_fd and fd are closed and tmp_path is removed; additionally add a retry
limit to the loop that calls unique_filename (where claim is done with os.open /
os.O_CREAT | os.O_EXCL) so the while True that generates `target` will bail with
a clear error after N attempts instead of looping forever.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8872fcea-abc6-454d-9bd6-d43e77bca182
📒 Files selected for processing (4)
platform-integrations/bob/kaizen-lite/skills/kaizen-learn/scripts/save_entities.pyplatform-integrations/bob/kaizen-lite/skills/kaizen-recall/scripts/retrieve_entities.pyplatform-integrations/claude/plugins/kaizen-lite/lib/entity_io.pyplatform-integrations/install.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- platform-integrations/bob/kaizen-lite/skills/kaizen-recall/scripts/retrieve_entities.py
- platform-integrations/install.sh
- platform-integrations/bob/kaizen-lite/skills/kaizen-learn/scripts/save_entities.py
Addresses CodeRabbit review finding: 0-byte orphaned target file on error
Collaborator
|
I guess the question is should we have it in a lib or duplicated under scripts? Roo does not have the concept of lib |
Contributor
Author
Right now it's separate from each skill: This seems to work in Bob. Do you prefer duplicated scripts within each skill directory? |
Collaborator
No, Just wanting to make sure we documented this and made the intentional decision to have it as a library even though roo does not support it. |
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
retrieve_entities.py,save_entities.py) importentity_ioviasys.pathbut thelib/directory was missing from the Bob kaizen-lite sourcelib/entity_io.pyand__init__.pytoplatform-integrations/bob/kaizen-lite/install.shto copy, remove, and check thelib/directory during Bob install/uninstall/statusRelated: #103
Test plan
./platform-integrations/install.sh install --platform bob --dir /tmp/test --dry-runand verifylib/copy appearsretrieve_entities.pywithoutModuleNotFoundError./platform-integrations/install.sh uninstall --platform bob --dir /tmp/test --dry-runand verifylib/removal appears./platform-integrations/install.sh status --dir /tmp/testand verifylib/entity_io.pystatus line appearsSummary by CodeRabbit