feat(platform-integrations): add portable bash/Python installer#100
Merged
Conversation
Adds install.sh and INSTALL_SPEC.md to platform-integrations/.
install.sh is a single-file bash/Python hybrid CLI that installs Kaizen
Lite (and optionally Full) integrations for Bob, Roo, and Claude Code
into any project directory.
Features:
- Subcommands: install, uninstall, status
- Auto-detects installed platforms (bob/roo/claude) from PATH and dot-folders
- Interactive platform selection when --platform is omitted
- --platform {bob,roo,claude,all} and --mode {lite,full} flags
- --dry-run flag on install and uninstall: prints every operation that
would be performed without touching the filesystem or running subprocesses
- Idempotent: JSON config files use atomic upsert-by-key; YAML files use
PyYAML merge (with sentinel comment block fallback if PyYAML is absent);
directory copies use shutil.copytree(dirs_exist_ok=True)
- Atomic writes via temp-file + os.replace() throughout
- JSON parse error recovery: backs up corrupt files before starting fresh
- Remote install via curl | bash using GitHub archive tarball (no git required)
- Local install auto-detected when platform-integrations/ is found relative
to the script, with a fallback for repo-root placement
- KAIZEN_REPO and KAIZEN_VERSION env var overrides for CI/pinned installs
- KAIZEN_DEBUG=1 for verbose per-operation logging
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a single-file hybrid installer (bash launcher + embedded Python) and a spec for Kaizen platform integrations, plus end-to-end tests and a release build change to stamp the script version. Implements install/uninstall/status for Bob, Roo, and Claude with atomic JSON/YAML updates, sentinel merging, and dry-run support. Changes
Sequence DiagramsequenceDiagram
actor User
participant Bash as "Installer (bash)"
participant Py as "Python Installer"
participant FS as "File System"
participant ClaudeCLI as "Claude CLI"
User->>Bash: run install.sh [args]
Bash->>Bash: validate python3 >= 3.8
Bash->>Bash: resolve source (local or GitHub tarball)
Bash->>Py: exec embedded installer (source + args)
Py->>Py: parse CLI, detect platforms (dirs & PATH)
alt platform = bob
Py->>FS: copy/merge skills, update `.bob/custom_modes.yaml` (sentinel), upsert `.bob/mcp.json` (atomic)
else platform = roo
Py->>FS: copy skills, upsert/remove `.roomodes` (JSON or sentinel YAML)
else platform = claude
Py->>ClaudeCLI: run `claude plugin install/uninstall/list` if available
ClaudeCLI-->>Py: success/fallback output
end
Py->>User: report results (success / failure / dry-run)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 2
🧹 Nitpick comments (3)
platform-integrations/install.sh (3)
431-464: Interactive prompt may fail silently in piped remote installs.When run via
curl | bashwithout a--platformflag,interactive_selectcallsinput()which reads from the pipe rather than the user's terminal. This will likely result in an empty string, causing the script to print "Cancelled." and exit without clear guidance.Consider detecting the non-interactive context and printing a helpful error message instead:
💡 Proposed fix to detect non-interactive mode
def interactive_select(detected): """Prompt user to choose platforms. Returns list of selected platform names.""" + if not sys.stdin.isatty(): + error("No --platform specified and stdin is not a terminal.") + error("For non-interactive installs, use: --platform {bob,roo,claude,all}") + sys.exit(1) + print() print("Detected platforms:")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 431 - 464, interactive_select currently calls input() which will read from a piped stdin (e.g. curl | bash) and silently cancel; update interactive_select to detect non-interactive stdin (use sys.stdin.isatty() or os.isatty(sys.stdin.fileno())) at the top of the function and, if not interactive, print a clear error message instructing the user to run with the --platform flag or run the installer interactively, then exit with a non-zero status; keep the rest of interactive_select logic unchanged so interactive cases still prompt normally.
503-514: Unusedmodeparameter inuninstall_bob.The function accepts a
modeparameter (defaulting to"full") but never uses it. The MCP server config is unconditionally removed frommcp.jsonregardless of the mode. Either remove the unused parameter or make the MCP removal conditional onmode == "full"to match the install behavior.💡 Option A: Remove unused parameter
-def uninstall_bob(target_dir, mode="full"): +def uninstall_bob(target_dir): bob_target = Path(target_dir) / ".bob" info(f"Uninstalling Bob from {bob_target}")💡 Option B: Make MCP removal conditional
-def uninstall_bob(target_dir, mode="full"): +def uninstall_bob(target_dir, mode="lite"): bob_target = Path(target_dir) / ".bob" info(f"Uninstalling Bob from {bob_target}") remove_dir(bob_target / "skills" / "kaizen-learn") remove_dir(bob_target / "skills" / "kaizen-recall") remove_file(bob_target / "commands" / "kaizen:learn.md") remove_file(bob_target / "commands" / "kaizen:recall.md") remove_yaml_custom_mode(bob_target / "custom_modes.yaml", BOB_SLUG) - remove_json_key(bob_target / "mcp.json", ["mcpServers", "kaizen"]) + if mode == "full": + remove_json_key(bob_target / "mcp.json", ["mcpServers", "kaizen"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 503 - 514, The uninstall_bob function currently ignores its mode parameter; either remove the unused mode argument from uninstall_bob(signature) and all callers, or make the MCP removal conditional so it only runs for full uninstalls: wrap the remove_json_key(bob_target / "mcp.json", ["mcpServers", "kaizen"]) call inside a check for mode == "full" (or equivalent), keeping the rest of uninstall_bob (remove_dir/remove_file/remove_yaml_custom_mode) unchanged; reference the uninstall_bob function, BOB_SLUG, and the remove_json_key call when applying the change.
23-27: UnusedBOLDvariable.The
BOLDvariable is defined but never used in the script. This was flagged by static analysis (SC2034). Consider removing it or using it in log output where emphasis is needed.🧹 Proposed fix to remove unused variable
if [ -t 1 ]; then - BOLD='\033[1m'; GREEN='\033[0;32m'; YELLOW='\033[0;33m' + GREEN='\033[0;32m'; YELLOW='\033[0;33m' RED='\033[0;31m'; CYAN='\033[0;36m'; RESET='\033[0m' else - BOLD=''; GREEN=''; YELLOW=''; RED=''; CYAN=''; RESET='' + GREEN=''; YELLOW=''; RED=''; CYAN=''; RESET='' fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 23 - 27, Remove or use the unused BOLD variable: either delete the BOLD assignment from the color variable block (the BOLD='...\033[1m' and the empty BOLD='' branch) to satisfy SC2034, or update log/echo statements that output headings to wrap them with BOLD/RESET so the variable becomes used; target the color setup block where BOLD, GREEN, YELLOW, RED, CYAN, RESET are defined to apply the change and keep the rest of the variables intact.
🤖 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/INSTALL_SPEC.md`:
- Around line 215-220: Remove the incorrect reference to a `--verbose` flag in
the "Logging" section; update the bullet list under "## Logging" to only
document that normal output uses `✓`/`✗`/`→` indicators and that verbose
behavior is enabled via the `KAIZEN_DEBUG=1` environment variable (remove the
`--verbose` bullet and any mention of equivalence to `KAIZEN_DEBUG=1`), ensuring
the remaining text mentions only `KAIZEN_DEBUG=1` as the way to enable verbose
output.
- Around line 47-55: The documentation omits the --dry-run flag for both
subcommands; update the "install options" and "uninstall options" sections to
add a --dry-run flag entry (e.g., "--dry-run Perform a
trial run without making changes") so the CLI spec matches the implemented
behavior for install and uninstall.
---
Nitpick comments:
In `@platform-integrations/install.sh`:
- Around line 431-464: interactive_select currently calls input() which will
read from a piped stdin (e.g. curl | bash) and silently cancel; update
interactive_select to detect non-interactive stdin (use sys.stdin.isatty() or
os.isatty(sys.stdin.fileno())) at the top of the function and, if not
interactive, print a clear error message instructing the user to run with the
--platform flag or run the installer interactively, then exit with a non-zero
status; keep the rest of interactive_select logic unchanged so interactive cases
still prompt normally.
- Around line 503-514: The uninstall_bob function currently ignores its mode
parameter; either remove the unused mode argument from uninstall_bob(signature)
and all callers, or make the MCP removal conditional so it only runs for full
uninstalls: wrap the remove_json_key(bob_target / "mcp.json", ["mcpServers",
"kaizen"]) call inside a check for mode == "full" (or equivalent), keeping the
rest of uninstall_bob (remove_dir/remove_file/remove_yaml_custom_mode)
unchanged; reference the uninstall_bob function, BOB_SLUG, and the
remove_json_key call when applying the change.
- Around line 23-27: Remove or use the unused BOLD variable: either delete the
BOLD assignment from the color variable block (the BOLD='...\033[1m' and the
empty BOLD='' branch) to satisfy SC2034, or update log/echo statements that
output headings to wrap them with BOLD/RESET so the variable becomes used;
target the color setup block where BOLD, GREEN, YELLOW, RED, CYAN, RESET are
defined to apply the change and keep the rest of the variables intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42f47018-3486-4074-b529-ceb48d2e58f5
📒 Files selected for processing (2)
platform-integrations/INSTALL_SPEC.mdplatform-integrations/install.sh
Add sed command to python-semantic-release build_command that automatically updates SCRIPT_VERSION in platform-integrations/install.sh during the release process. This ensures that install scripts fetched from tag URLs already know their own version and can download the correct tarball without requiring users to set KAIZEN_VERSION manually. Changes: - Update pyproject.toml build_command to sed SCRIPT_VERSION to the release tag - Add SCRIPT_VERSION variable to install.sh (defaults to 'main') - Update install.sh documentation to reflect automated versioning - Update INSTALL_SPEC.md to document the new behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
platform-integrations/install.sh (3)
32-36: Remove unusedBOLDvariable.Static analysis correctly flags
BOLDas unused. It's defined but never referenced in the script.🧹 Proposed fix
if [ -t 1 ]; then - BOLD='\033[1m'; GREEN='\033[0;32m'; YELLOW='\033[0;33m' + GREEN='\033[0;32m'; YELLOW='\033[0;33m' RED='\033[0;31m'; CYAN='\033[0;36m'; RESET='\033[0m' else - BOLD=''; GREEN=''; YELLOW=''; RED=''; CYAN=''; RESET='' + GREEN=''; YELLOW=''; RED=''; CYAN=''; RESET='' fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 32 - 36, Remove the unused BOLD variable by deleting its declarations in both the colored and non-colored branches (the BOLD='\033[1m' entry in the color vars block and the BOLD='' in the else block) so only used color variables (GREEN, YELLOW, RED, CYAN, RESET) remain; ensure no other code references BOLD (search for the symbol BOLD) and run shellcheck to confirm the warning is resolved.
440-474: Inconsistent TTY handling in interactive_select.Line 446 uses hardcoded ANSI codes (
\033[32m) while the rest of the Python code uses the_c()helper that respectsIS_TTY. This could produce garbled output when stdout is redirected.🧹 Proposed fix
def interactive_select(detected): """Prompt user to choose platforms. Returns list of selected platform names.""" print() print("Detected platforms:") options = list(detected.keys()) for i, name in enumerate(options, 1): - indicator = "\033[32m✓\033[0m" if detected[name] else "·" + indicator = _c("32", "✓") if detected[name] else "·" note = "detected" if detected[name] else "not detected" print(f" {i}. {name} ({note}) {indicator}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 440 - 474, The interactive_select function prints a hardcoded ANSI green check ("\033[32m✓\033[0m") which bypasses the project's TTY-aware helper and can garble output when stdout is redirected; update interactive_select to use the existing _c(...) helper (and respect IS_TTY) for the checkmark and any colorized output so the indicator follows the same TTY logic as the rest of the script (replace the hardcoded "\033[32m✓\033[0m" with a call to _c("✓", "green") or equivalent and ensure any surrounding logic still uses the detected dict and options list).
372-416: Regex-based YAML parsing is fragile but acceptable given constraints.The
load_roo_mode_from_yamlfunction uses regex to parse YAML to avoid a PyYAML dependency. This works for the expected format but may fail on valid YAML variations (e.g., multi-line strings with different indent styles, quoted values with special characters, or alternative list syntaxes).Consider adding a warning in debug mode if critical fields are empty, to help diagnose parsing failures:
if not slug: if KAIZEN_DEBUG: warn(f"Could not extract 'slug' field from {source_path}") return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 372 - 416, The regex YAML parsing in load_roo_mode_from_yaml can silently fail (e.g., missing slug); update the function to emit a debug warning when critical fields are empty: when slug is falsey, check a KAIZEN_DEBUG flag and call warn (or an existing logger) with a clear message including source_path, then return None; similarly consider logging when other important fields (name, roleDefinition, firstMessage) are unexpectedly empty to aid diagnosis.platform-integrations/INSTALL_SPEC.md (1)
24-30: Add language specifiers to fenced code blocks.Several code blocks lack language specifiers which helps with syntax highlighting and accessibility:
🧹 Proposed fix for code block languages
Lines 24-30:
-``` +```text https://github.com/${KAIZEN_REPO}/archive/refs/heads/main.tar.gzOr a pinned version:
-+text
https://github.com/${KAIZEN_REPO}/archive/refs/tags/v${VERSION}.tar.gzLines 42-58:
-``` +```text install.sh <command> [options] ...Lines 114-116: ```diff - ``` + ```bash claude --plugin-dir /path/to/platform-integrations/claude/plugins/kaizen-lite ```Also applies to: 42-58, 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/INSTALL_SPEC.md` around lines 24 - 30, Add explicit language specifiers to the fenced code blocks in INSTALL_SPEC.md: update the two URL blocks containing "https://github.com/${KAIZEN_REPO}/archive/..." to use ```text, change the install usage block starting with "install.sh <command> [options]" to ```text, and change the claude plugin example containing "claude --plugin-dir /path/to/platform-integrations/claude/plugins/kaizen-lite" to ```bash so syntax highlighting and accessibility are enabled.
🤖 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/install.sh`:
- Around line 500-508: Wrap the mcp.json read in a file-existence and
error-handling guard: before opening mcp_source (the Path built when mode ==
"full"), check mcp_source.exists() and if missing call the script's error/exit
path (or print an error and exit non-zero), and when opening/parsing use
try/except to catch FileNotFoundError and json.JSONDecodeError so you can log a
clear failure message and abort rather than letting the exception bubble; update
the block that uses mcp_source, mcp_target, and upsert_json_key accordingly and
ensure success(...) is only called on successful load and upsert.
---
Nitpick comments:
In `@platform-integrations/INSTALL_SPEC.md`:
- Around line 24-30: Add explicit language specifiers to the fenced code blocks
in INSTALL_SPEC.md: update the two URL blocks containing
"https://github.com/${KAIZEN_REPO}/archive/..." to use ```text, change the
install usage block starting with "install.sh <command> [options]" to ```text,
and change the claude plugin example containing "claude --plugin-dir
/path/to/platform-integrations/claude/plugins/kaizen-lite" to ```bash so syntax
highlighting and accessibility are enabled.
In `@platform-integrations/install.sh`:
- Around line 32-36: Remove the unused BOLD variable by deleting its
declarations in both the colored and non-colored branches (the BOLD='\033[1m'
entry in the color vars block and the BOLD='' in the else block) so only used
color variables (GREEN, YELLOW, RED, CYAN, RESET) remain; ensure no other code
references BOLD (search for the symbol BOLD) and run shellcheck to confirm the
warning is resolved.
- Around line 440-474: The interactive_select function prints a hardcoded ANSI
green check ("\033[32m✓\033[0m") which bypasses the project's TTY-aware helper
and can garble output when stdout is redirected; update interactive_select to
use the existing _c(...) helper (and respect IS_TTY) for the checkmark and any
colorized output so the indicator follows the same TTY logic as the rest of the
script (replace the hardcoded "\033[32m✓\033[0m" with a call to _c("✓", "green")
or equivalent and ensure any surrounding logic still uses the detected dict and
options list).
- Around line 372-416: The regex YAML parsing in load_roo_mode_from_yaml can
silently fail (e.g., missing slug); update the function to emit a debug warning
when critical fields are empty: when slug is falsey, check a KAIZEN_DEBUG flag
and call warn (or an existing logger) with a clear message including
source_path, then return None; similarly consider logging when other important
fields (name, roleDefinition, firstMessage) are unexpectedly empty to aid
diagnosis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84c94677-0525-43cd-84d7-92f402343fd0
📒 Files selected for processing (3)
platform-integrations/INSTALL_SPEC.mdplatform-integrations/install.shpyproject.toml
…comprehensive tests - Fix regex patterns to handle YAML list items with optional dash prefix - Change .roomodes creation to prefer YAML format (roo-code standard) - Simplify .roomodes handling logic for better maintainability - Add comprehensive test suite for platform integrations: * test_preservation.py: Critical tests ensuring user data is never overwritten * test_idempotency.py: Tests for safe repeated installations * AGENTS.md: Complete testing guide and requirements documentation These changes ensure the installer safely handles existing user configurations and can be run multiple times without data loss.
…n install script - Fix YAML structure initialization for empty custom mode files - Correct regex patterns to match proper indentation levels in Roo mode YAML - Refactor test runner command building for better readability - Use subprocess check parameter for cleaner error handling
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/platform_integrations/AGENTS.md (1)
23-29: Add language specifier to fenced code block.The code block showing test structure should have a language specifier for consistency with other code blocks in the document.
📝 Proposed fix
-``` +```text tests/platform_integrations/ ├── AGENTS.md # This file ├── conftest.py # Fixtures and helpers ├── test_preservation.py # CRITICAL: User data preservation tests └── test_idempotency.py # Idempotency tests -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/AGENTS.md` around lines 23 - 29, Update the fenced code block in AGENTS.md to include a language specifier (use "text") so it matches other blocks; locate the code block showing the tests tree in AGENTS.md and change the opening fence from ``` to ```text and leave the closing fence as ``` to ensure proper formatting and consistency.tests/platform_integrations/conftest.py (1)
16-18: Consider registeringplatform_integrationsmarker inpyproject.toml.The marker is locally registered in
conftest.pyviaconfig.addinivalue_line(), which suppresses warnings. However, per the coding guidelines, pytest markers should beunit,e2e, orphoenix. Consider either:
- Adding
platform_integrationsto the markers list inpyproject.tomlfor consistency, or- Using
e2emarker since these are end-to-end tests that run the actual installer script.Based on learnings: "Use pytest markers
unitfor unit tests,e2efor end-to-end tests, andphoenixfor Phoenix integration tests"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/conftest.py` around lines 16 - 18, The pytest marker "platform_integrations" is being registered in pytest_configure in conftest.py; update the project to follow the agreed markers by either (A) adding "platform_integrations" to the markers list in pyproject.toml so the marker is globally declared, or (B) change the tests to use the standard "e2e" marker instead and remove or update the config.addinivalue_line call in pytest_configure; locate the pytest_configure function in conftest.py and adjust the marker name or remove the addinivalue_line, and if choosing option A, add the matching entry under [tool.pytest.ini_options] markers in pyproject.toml.platform-integrations/install.sh (2)
32-36: UnusedBOLDvariable.The
BOLDvariable is defined but never used in the bash section. While this is minor, it adds clutter.🧹 Proposed fix
if [ -t 1 ]; then - BOLD='\033[1m'; GREEN='\033[0;32m'; YELLOW='\033[0;33m' + GREEN='\033[0;32m'; YELLOW='\033[0;33m' RED='\033[0;31m'; CYAN='\033[0;36m'; RESET='\033[0m' else - BOLD=''; GREEN=''; YELLOW=''; RED=''; CYAN=''; RESET='' + GREEN=''; YELLOW=''; RED=''; CYAN=''; RESET='' fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 32 - 36, Remove the unused BOLD variable from the bash color declarations: delete BOLD from both the ANSI escape assignment and the empty-string fallback so only used color variables (GREEN, YELLOW, RED, CYAN, RESET) remain; update any references to BOLD if they exist elsewhere (search for symbol BOLD) or remove them to avoid undefined variable usage.
372-416: Regex-based YAML parsing is fragile but acceptable for this use case.The
load_roo_mode_from_yamlfunction uses regex to extract fields from YAML, which works for the known source file format. However, this approach may break if the YAML structure changes (e.g., different indentation, multiline values with special characters, or comments within fields).Consider documenting this limitation or adding a comment noting the expected source format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 372 - 416, The regex YAML parsing in load_roo_mode_from_yaml is fragile; update its docstring and/or add an inline comment to document the exact expected Roo .roomodes format (assumptions about indentation, single-line fields, block scalars using | or |-, list items prefixed by "- ", and no embedded comments), and add a small runtime safeguard (e.g., log a clear warning or raise an error when slug is missing or when extracted multiline/list values look malformed) so callers know parsing is best-effort; reference the function name load_roo_mode_from_yaml and the helper extracts extract, extract_block, extract_list when adding these comments and the validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/platform_integrations/AGENTS.md`:
- Around line 254-258: Update the Roo Lite Mode section so its description of
creating `.roomodes` matches the Merge mode rules: change the line that
currently says "If doesn't exist: Create as JSON" to indicate the correct
creation format (i.e., create as YAML) for `.roomodes`; ensure the phrase
appears under the Roo Lite Mode heading and that `.roomodes` is consistently
described across both sections.
- Around line 79-80: Update the documentation text that currently reads "When
`.roomodes` doesn't exist, it's created as JSON (modern format)" to correctly
state that `.roomodes` is created as YAML when absent; search for the sentence
in AGENTS.md and replace "JSON (modern format)" with "YAML" (or "YAML (modern
format)" if you want to retain the parenthetical), ensuring the wording matches
how the installer (install.sh) and test_idempotency.py create `.roomodes`.
---
Nitpick comments:
In `@platform-integrations/install.sh`:
- Around line 32-36: Remove the unused BOLD variable from the bash color
declarations: delete BOLD from both the ANSI escape assignment and the
empty-string fallback so only used color variables (GREEN, YELLOW, RED, CYAN,
RESET) remain; update any references to BOLD if they exist elsewhere (search for
symbol BOLD) or remove them to avoid undefined variable usage.
- Around line 372-416: The regex YAML parsing in load_roo_mode_from_yaml is
fragile; update its docstring and/or add an inline comment to document the exact
expected Roo .roomodes format (assumptions about indentation, single-line
fields, block scalars using | or |-, list items prefixed by "- ", and no
embedded comments), and add a small runtime safeguard (e.g., log a clear warning
or raise an error when slug is missing or when extracted multiline/list values
look malformed) so callers know parsing is best-effort; reference the function
name load_roo_mode_from_yaml and the helper extracts extract, extract_block,
extract_list when adding these comments and the validation.
In `@tests/platform_integrations/AGENTS.md`:
- Around line 23-29: Update the fenced code block in AGENTS.md to include a
language specifier (use "text") so it matches other blocks; locate the code
block showing the tests tree in AGENTS.md and change the opening fence from ```
to ```text and leave the closing fence as ``` to ensure proper formatting and
consistency.
In `@tests/platform_integrations/conftest.py`:
- Around line 16-18: The pytest marker "platform_integrations" is being
registered in pytest_configure in conftest.py; update the project to follow the
agreed markers by either (A) adding "platform_integrations" to the markers list
in pyproject.toml so the marker is globally declared, or (B) change the tests to
use the standard "e2e" marker instead and remove or update the
config.addinivalue_line call in pytest_configure; locate the pytest_configure
function in conftest.py and adjust the marker name or remove the
addinivalue_line, and if choosing option A, add the matching entry under
[tool.pytest.ini_options] markers in pyproject.toml.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8ebf7af-6e15-4a08-bf4e-45ce93b7f0e9
📒 Files selected for processing (5)
platform-integrations/install.shtests/platform_integrations/AGENTS.mdtests/platform_integrations/conftest.pytests/platform_integrations/test_idempotency.pytests/platform_integrations/test_preservation.py
Comment on lines
+79
to
+80
| - When `.roomodes` doesn't exist, it's created as JSON (modern format) | ||
|
|
There was a problem hiding this comment.
Documentation inconsistency: .roomodes is created as YAML, not JSON.
The documentation states "When .roomodes doesn't exist, it's created as JSON (modern format)", but the installer (install.sh line 561-563) and the test (test_idempotency.py line 133-146) both confirm it's created as YAML when the file doesn't exist.
📝 Proposed fix
**Roo Platform:**
- Multiple installs with JSON `.roomodes` produce identical results
- Multiple installs with YAML `.roomodes` produce identical results
-- When `.roomodes` doesn't exist, it's created as JSON (modern format)
+- When `.roomodes` doesn't exist, it's created as YAML (roo-code preferred format)📝 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
| - When `.roomodes` doesn't exist, it's created as JSON (modern format) | |
| **Roo Platform:** | |
| - Multiple installs with JSON `.roomodes` produce identical results | |
| - Multiple installs with YAML `.roomodes` produce identical results | |
| - When `.roomodes` doesn't exist, it's created as YAML (roo-code preferred format) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/platform_integrations/AGENTS.md` around lines 79 - 80, Update the
documentation text that currently reads "When `.roomodes` doesn't exist, it's
created as JSON (modern format)" to correctly state that `.roomodes` is created
as YAML when absent; search for the sentence in AGENTS.md and replace "JSON
(modern format)" with "YAML" (or "YAML (modern format)" if you want to retain
the parenthetical), ensuring the wording matches how the installer (install.sh)
and test_idempotency.py create `.roomodes`.
Comment on lines
+254
to
+258
| 3. Merge mode into `.roomodes`: | ||
| - If JSON: Array upsert by slug | ||
| - If YAML: Sentinel block merge | ||
| - If doesn't exist: Create as JSON | ||
|
|
There was a problem hiding this comment.
Same documentation inconsistency for Roo Lite Mode section.
This section also incorrectly states .roomodes is created as JSON when it doesn't exist.
📝 Proposed fix
3. Merge mode into `.roomodes`:
- If JSON: Array upsert by slug
- If YAML: Sentinel block merge
- - If doesn't exist: Create as JSON
+ - If doesn't exist: Create as YAML (roo-code preferred format)📝 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
| 3. Merge mode into `.roomodes`: | |
| - If JSON: Array upsert by slug | |
| - If YAML: Sentinel block merge | |
| - If doesn't exist: Create as JSON | |
| 3. Merge mode into `.roomodes`: | |
| - If JSON: Array upsert by slug | |
| - If YAML: Sentinel block merge | |
| - If doesn't exist: Create as YAML (roo-code preferred format) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/platform_integrations/AGENTS.md` around lines 254 - 258, Update the Roo
Lite Mode section so its description of creating `.roomodes` matches the Merge
mode rules: change the line that currently says "If doesn't exist: Create as
JSON" to indicate the correct creation format (i.e., create as YAML) for
`.roomodes`; ensure the phrase appears under the Roo Lite Mode heading and that
`.roomodes` is consistently described across both sections.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
platform-integrations/install.sh (1)
511-519:⚠️ Potential issue | 🟡 MinorThe full-mode source parse still needs a clean failure path.
The new
exists()check only covers the missing-file case.json.load()andmcp_data["mcpServers"]["kaizen"]will still raise raw exceptions on a corrupt or shape-mismatched source file, so this should fail with a controlled installer error before the target config is touched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/install.sh` around lines 511 - 519, Ensure the installer fails cleanly on malformed or unexpected MCP source content by wrapping the open/json.load and key access in a try/except that catches JSONDecodeError/KeyError (and general Exception as fallback), log a clear installer error via error(...) including the mcp_source path and exception details, and exit with sys.exit(1) before calling upsert_json_key; specifically guard the block that reads mcp_source, loads mcp_data, and extracts mcp_data["mcpServers"]["kaizen"] (referencing variables mcp_source, mcp_data, kaizen_server, mcp_target and function upsert_json_key) so the target is not modified on parse/shape failures.
🧹 Nitpick comments (1)
tests/platform_integrations/conftest.py (1)
73-111: The runner can't drive the interactive path it documents.The docstring advertises
platform=Nonefor interactive mode, but this helper has no way to send a selection to the subprocess. That leaves the prompt flow untestable from the shared runner.Possible shape
def run( self, command: str, platform: Optional[str] = None, mode: Optional[str] = None, dry_run: bool = False, expect_success: bool = True, env: Optional[Dict[str, str]] = None, + input_text: Optional[str] = None, ) -> subprocess.CompletedProcess: @@ - result = subprocess.run(cmd, capture_output=True, text=True, env=test_env, check=expect_success) + result = subprocess.run( + cmd, + capture_output=True, + text=True, + input=input_text, + env=test_env, + check=expect_success, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/conftest.py` around lines 73 - 111, The run helper documents an interactive path when platform=None but cannot drive stdin; update the run method signature to accept an optional input (or stdin_input) parameter and docstring, and when provided call subprocess.run with input=stdin_input and stdin=subprocess.PIPE (or pass the input string directly), so the prompt flow can be exercised; make sure to keep building cmd the same (using self.script_path and self.project_dir), merge env into test_env, and pass env=test_env, capture_output=True, text=True, and check=expect_success to subprocess.run so tests can send selections to the subprocess.
🤖 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/INSTALL_SPEC.md`:
- Around line 164-178: The documented sentinel markers are missing the required
"kaizen:" prefix and therefore don't match the actual markers used by the
implementation and tests; update the example block and the Install/Uninstall
instructions to use the exact markers "# >>>kaizen:kaizen-lite<<<" and "#
<<<kaizen:kaizen-lite<<<" (including the "kaizen:" namespace) so the
detection/replacement and removal logic can correctly find the managed block.
- Around line 68-72: Update the Roo detection row in the INSTALL_SPEC.md
platforms table to include the `.roo/` directory as a detection signal (e.g.,
"`.roomodes` or `.roo/` file/dir exists in target dir, OR `roo` or `roo-code` on
PATH") so it matches the behavior of detect_platforms(); ensure the wording
references the `.roo/` directory alongside `.roomodes` and keeps the existing
PATH alternatives.
- Around line 105-108: The installer currently creates a YAML `.roomodes` by
calling merge_yaml_custom_mode() when the target is missing, contradicting the
spec which mandates JSON; update install_roo() so that when `.roomodes` does not
exist it either calls a JSON-creating routine (e.g., merge_json_custom_mode() or
a new function that writes JSON) or change merge_yaml_custom_mode() to emit JSON
when the target is absent, and ensure the log message no longer reports “(YAML)”
in that branch; keep slug upsert behavior intact (slug: kaizen-lite) and run
existing tests to validate the JSON creation path.
In `@platform-integrations/install.sh`:
- Around line 303-311: is_json_file() currently swallows JSONDecodeError and
returns False, causing install_roo() to treat corrupt JSON files as YAML and
skip the backup-and-reset flow; change the logic so JSONDecodeError is
distinguished from FileNotFoundError: have is_json_file() only return False for
FileNotFoundError (or introduce a new is_valid_json_file()/detect_json_state()
that returns OK / MISSING / INVALID), and update install_roo() to check for
INVALID JSON (JSONDecodeError) and invoke the documented backup-and-reset
recovery path instead of taking the YAML branch; reference the is_json_file
function and the install_roo function when making the change.
- Around line 192-204: read_json currently performs file writes on
json.JSONDecodeError (creates a "<path>.kaizen.bak"), which causes unwanted
mutations when callers like status_bob read from read-only paths or in dry-run
flows; change read_json to be side-effect free by removing the backup/write
behavior — on JSONDecodeError just warn and return {} (or add an explicit
parameter like allow_backup=False and only perform the shutil.copy2 when
allow_backup is True), and update callers that need the backup to pass
allow_backup=True (e.g., any code that intentionally wants to persist a backup),
leaving status_bob and dry-run callers using the default no-backup behavior.
- Around line 396-404: The extract_block function's regex stops at the first
blank line because it uses .+ which doesn't match empty lines; update the regex
in extract_block to allow blank lines (e.g., use a pattern that matches any char
including newlines such as [\s\S] or use DOTALL/.*? with a proper terminator) so
the block scalar captures all indented lines until the next top-level key, and
ensure the existing min_indent calculation still ignores purely empty lines when
unindenting; keep the fallback to extract(field) unchanged.
In `@tests/platform_integrations/conftest.py`:
- Around line 16-18: In pytest_configure, register the standard e2e marker
alongside platform_integrations by calling config.addinivalue_line for "markers"
with an entry for "e2e: end-to-end tests" (in addition to the existing
platform_integrations line); update the pytest_configure function so both
markers are declared (refer to the pytest_configure function and the
config.addinivalue_line call to locate the change).
---
Duplicate comments:
In `@platform-integrations/install.sh`:
- Around line 511-519: Ensure the installer fails cleanly on malformed or
unexpected MCP source content by wrapping the open/json.load and key access in a
try/except that catches JSONDecodeError/KeyError (and general Exception as
fallback), log a clear installer error via error(...) including the mcp_source
path and exception details, and exit with sys.exit(1) before calling
upsert_json_key; specifically guard the block that reads mcp_source, loads
mcp_data, and extracts mcp_data["mcpServers"]["kaizen"] (referencing variables
mcp_source, mcp_data, kaizen_server, mcp_target and function upsert_json_key) so
the target is not modified on parse/shape failures.
---
Nitpick comments:
In `@tests/platform_integrations/conftest.py`:
- Around line 73-111: The run helper documents an interactive path when
platform=None but cannot drive stdin; update the run method signature to accept
an optional input (or stdin_input) parameter and docstring, and when provided
call subprocess.run with input=stdin_input and stdin=subprocess.PIPE (or pass
the input string directly), so the prompt flow can be exercised; make sure to
keep building cmd the same (using self.script_path and self.project_dir), merge
env into test_env, and pass env=test_env, capture_output=True, text=True, and
check=expect_success to subprocess.run so tests can send selections to the
subprocess.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbb19635-c679-40bf-8a84-1eae75b75079
📒 Files selected for processing (3)
platform-integrations/INSTALL_SPEC.mdplatform-integrations/install.shtests/platform_integrations/conftest.py
Comment on lines
+68
to
+72
| | Platform | Detection signals | | ||
| |----------|-------------------| | ||
| | bob | `.bob/` dir exists in target dir, OR `bob` on PATH | | ||
| | roo | `.roomodes` file exists in target dir, OR `roo` or `roo-code` on PATH | | ||
| | claude | `.claude/` dir exists in target dir, OR `claude` on PATH | |
There was a problem hiding this comment.
Document .roo/ as a Roo detection signal.
detect_platforms() also treats an existing .roo/ directory as Roo, so this table is under-documenting when auto-detect will offer/select Roo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-integrations/INSTALL_SPEC.md` around lines 68 - 72, Update the Roo
detection row in the INSTALL_SPEC.md platforms table to include the `.roo/`
directory as a detection signal (e.g., "`.roomodes` or `.roo/` file/dir exists
in target dir, OR `roo` or `roo-code` on PATH") so it matches the behavior of
detect_platforms(); ensure the wording references the `.roo/` directory
alongside `.roomodes` and keeps the existing PATH alternatives.
Comment on lines
+105
to
+108
| 3. Merge mode entry from `skills/.roomodes` → `.roomodes` in project dir | ||
| - Target `.roomodes` may be JSON or YAML; detected by trying `json.loads` first | ||
| - Upsert by `slug: kaizen-lite` (JSON: array upsert; YAML: sentinel block) | ||
| - If target does not exist, create as JSON |
There was a problem hiding this comment.
The spec says a missing .roomodes file becomes JSON, but the installer creates YAML.
install_roo() calls merge_yaml_custom_mode() when .roomodes is absent and even reports (YAML), so this contract is out of sync with the actual behavior and tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-integrations/INSTALL_SPEC.md` around lines 105 - 108, The installer
currently creates a YAML `.roomodes` by calling merge_yaml_custom_mode() when
the target is missing, contradicting the spec which mandates JSON; update
install_roo() so that when `.roomodes` does not exist it either calls a
JSON-creating routine (e.g., merge_json_custom_mode() or a new function that
writes JSON) or change merge_yaml_custom_mode() to emit JSON when the target is
absent, and ensure the log message no longer reports “(YAML)” in that branch;
keep slug upsert behavior intact (slug: kaizen-lite) and run existing tests to
validate the JSON creation path.
Comment on lines
+164
to
+178
| ```yaml | ||
| customModes: | ||
| - slug: other-mode | ||
| ... | ||
| # >>>kaizen-lite<<< | ||
| - slug: kaizen-lite | ||
| name: Kaizen Lite | ||
| ... | ||
| # <<<kaizen-lite<<< | ||
| ``` | ||
|
|
||
| **Install**: check if sentinel `# >>>kaizen-lite<<<` exists in file. If yes, replace the block | ||
| between sentinels. If no, append sentinel block to end of file. | ||
|
|
||
| **Uninstall**: find sentinel start and end lines, remove all lines between them (inclusive). |
There was a problem hiding this comment.
The sentinel syntax here doesn't match the managed block the script writes.
The implementation and test helpers use # >>>kaizen:kaizen-lite<<< / # <<<kaizen:kaizen-lite<<<; documenting the markers without the kaizen: prefix will mislead anyone trying to detect or remove the managed block from this spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-integrations/INSTALL_SPEC.md` around lines 164 - 178, The documented
sentinel markers are missing the required "kaizen:" prefix and therefore don't
match the actual markers used by the implementation and tests; update the
example block and the Install/Uninstall instructions to use the exact markers "#
>>>kaizen:kaizen-lite<<<" and "# <<<kaizen:kaizen-lite<<<" (including the
"kaizen:" namespace) so the detection/replacement and removal logic can
correctly find the managed block.
Comment on lines
+192
to
+204
| def read_json(path): | ||
| """Read a JSON file, return {} if not found. Back up and reset on parse error.""" | ||
| path = str(path) | ||
| try: | ||
| with open(path) as f: | ||
| return json.load(f) | ||
| except FileNotFoundError: | ||
| return {} | ||
| except json.JSONDecodeError: | ||
| bak = path + ".kaizen.bak" | ||
| warn(f"Could not parse {path} — backing up to {bak} and starting fresh.") | ||
| shutil.copy2(path, bak) | ||
| return {} |
There was a problem hiding this comment.
Keep read_json() side-effect free.
This helper writes <file>.kaizen.bak on JSONDecodeError, and status_bob() calls it from a read-only path. A status run against a corrupt config will therefore mutate the project, and the same thing happens in --dry-run flows that read JSON before deciding what to write.
Also applies to: 547-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-integrations/install.sh` around lines 192 - 204, read_json currently
performs file writes on json.JSONDecodeError (creates a "<path>.kaizen.bak"),
which causes unwanted mutations when callers like status_bob read from read-only
paths or in dry-run flows; change read_json to be side-effect free by removing
the backup/write behavior — on JSONDecodeError just warn and return {} (or add
an explicit parameter like allow_backup=False and only perform the shutil.copy2
when allow_backup is True), and update callers that need the backup to pass
allow_backup=True (e.g., any code that intentionally wants to persist a backup),
leaving status_bob and dry-run callers using the default no-backup behavior.
Comment on lines
+303
to
+311
| def is_json_file(path): | ||
| """Detect whether a file is JSON (vs YAML) by attempting to parse it.""" | ||
| try: | ||
| with open(str(path)) as f: | ||
| content = f.read().strip() | ||
| json.loads(content) | ||
| return True | ||
| except (FileNotFoundError, json.JSONDecodeError): | ||
| return False |
There was a problem hiding this comment.
Don't collapse "invalid JSON" into "YAML".
is_json_file() returns False on JSONDecodeError, and install_roo() uses that boolean to take the YAML branch. A corrupt JSON .roomodes will skip the documented backup-and-reset flow and get rewritten as YAML instead of being recovered as JSON.
Also applies to: 576-586
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-integrations/install.sh` around lines 303 - 311, is_json_file()
currently swallows JSONDecodeError and returns False, causing install_roo() to
treat corrupt JSON files as YAML and skip the backup-and-reset flow; change the
logic so JSONDecodeError is distinguished from FileNotFoundError: have
is_json_file() only return False for FileNotFoundError (or introduce a new
is_valid_json_file()/detect_json_state() that returns OK / MISSING / INVALID),
and update install_roo() to check for INVALID JSON (JSONDecodeError) and invoke
the documented backup-and-reset recovery path instead of taking the YAML branch;
reference the is_json_file function and the install_roo function when making the
change.
Comment on lines
+396
to
+404
| def extract_block(field): | ||
| # Match block scalar (|- or |) at mode definition level (4 spaces, no list dash) | ||
| m = re.search(rf"^ {field}:\s*\|-?\s*\n((?:[ \t]+.+\n?)*)", text, re.MULTILINE) | ||
| if m: | ||
| lines = m.group(1).splitlines() | ||
| # Find minimum indent | ||
| min_indent = min((len(l) - len(l.lstrip()) for l in lines if l.strip()), default=0) | ||
| return "\n".join(l[min_indent:] for l in lines) | ||
| return extract(field) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
import re
text = Path("platform-integrations/roo/kaizen-lite/skills/.roomodes").read_text()
for field in ("roleDefinition", "customInstructions"):
m = re.search(rf"^ {field}:\s*\|-?\s*\n((?:[ \t]+.+\n?)*)", text, re.MULTILINE)
captured = m.group(1) if m else ""
print(f"{field}: captured_lines={len(captured.splitlines())}")
print(captured)
print("-----")
PYRepository: AgentToolkit/kaizen
Length of output: 637
🏁 Script executed:
cat platform-integrations/roo/kaizen-lite/skills/.roomodesRepository: AgentToolkit/kaizen
Length of output: 3449
extract_block() truncates block scalars at the first blank line.
The regex pattern ((?:[ \t]+.+\n?)*) uses .+, which cannot match empty lines. When a blank line is encountered, the pattern stops matching, causing data loss:
roleDefinition: Only captures the first line; loses ~95% of content including the WORKFLOW and ENFORCEMENT RULES sectionscustomInstructions: Only captures the first four bullets; loses the PRE-COMPLETION GATE and Rules sections
This breaks the mode definition by stripping critical mandatory instructions that should be included in the JSON output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-integrations/install.sh` around lines 396 - 404, The extract_block
function's regex stops at the first blank line because it uses .+ which doesn't
match empty lines; update the regex in extract_block to allow blank lines (e.g.,
use a pattern that matches any char including newlines such as [\s\S] or use
DOTALL/.*? with a proper terminator) so the block scalar captures all indented
lines until the next top-level key, and ensure the existing min_indent
calculation still ignores purely empty lines when unindenting; keep the fallback
to extract(field) unchanged.
4 tasks
This was referenced Apr 2, 2026
This was referenced Apr 17, 2026
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
1 participant
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.
Adds install.sh to
./platform-integrationsand testsinstall.sh is a single-file bash/Python hybrid CLI that installs Kaizen Lite (and optionally Full) integrations for Bob, Roo, and Claude Code into any project directory.
Features:
Summary by CodeRabbit