fix: make LibraVDB installer fully automated#82
Conversation
e200072 to
9250fc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/install.md`:
- Around line 144-150: The example assumes ~/.local/bin is on PATH; update the
docs so the commands will work as-is by either (A) invoking the binary with its
full path after chmod (use "~/.local/bin/libravdbd serve" instead of "libravdbd
serve"), or (B) adding an explicit PATH export step before running (e.g.,
"export PATH=$HOME/.local/bin:$PATH") so the existing "libravdbd serve"
invocation succeeds; make the change near the shown chmod +x and libravdbd serve
lines.
In `@scripts/auto-install.sh`:
- Around line 495-501: The early return when the installed daemon matches the
desired version skips PATH bootstrapping; ensure PATH setup still runs before
returning by invoking append_path_once and exporting the temporary PATH (the
same logic used elsewhere) prior to the return in the block that checks
daemon_version/bin_path/tag_norm, and keep the call to provision_manual_assets;
update the block surrounding daemon_version, current_norm, and tag_norm so
append_path_once and the PATH export occur unconditionally when the daemon is
current.
- Around line 1062-1064: The install_daemon_macos_brew function currently masks
failures because errexit is disabled and several brew commands lack explicit
failure propagation; update install_daemon_macos_brew so each critical brew
command (e.g., any brew tap, brew install, and brew services commands inside
that function) is followed by "|| return 1" to ensure the function returns
non-zero on failure, allowing the caller's if ! install_daemon_macos_brew check
to trigger the fallback install_daemon_manual path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d831beb3-31ac-48b0-beb9-1e134a48c5eb
📒 Files selected for processing (19)
README.mddocs/configuration.mddocs/development.mddocs/embedding-profiles.mddocs/install.mddocs/installation.mddocs/models.mddocs/security.mddocs/uninstall.mdopenclaw.plugin.jsonpackage.jsonscripts/auto-install.shsrc/index.tssrc/plugin-runtime.tssrc/sidecar.tstest/integration/checklist-validation.test.tstest/integration/sidecar-lifecycle.test.tstest/unit/plugin-runtime.test.tstest/unit/sidecar.test.ts
|
CodeRabbit caught some valid script edge cases (the PATH append on early exit and the If a user installs via the non-Homebrew path, the script downloads:
When running This means running The
|
9250fc7 to
65b02a4
Compare
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)
scripts/auto-install.sh (1)
1029-1033:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUninstall leaves installer-managed ONNX/model assets behind.
At Line 1029 through Line 1033, uninstall performs service/config/plugin/binary cleanup but never removes
~/.local/share/libravdb/onnxruntimeand~/.local/share/libravdb/models, which are created by manual provisioning and can leave ~1GB orphaned data.Suggested patch
+remove_manual_assets() { + local asset_root + asset_root="$(manual_asset_root)" + if [[ "$DRY_RUN" -eq 1 ]]; then + info "[dry-run] rm -rf ${asset_root}/onnxruntime ${asset_root}/models" + return 0 + fi + rm -rf "${asset_root}/onnxruntime" "${asset_root}/models" + info "Removed installer-managed assets under ${asset_root}." +} + run_uninstall_mode() { local os os="$(detect_os)" @@ stop_daemon_services "$os" uninstall_openclaw_config uninstall_plugin_package remove_manual_daemon_binary + remove_manual_assets🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/auto-install.sh` around lines 1029 - 1033, The uninstall flow currently calls stop_daemon_services, uninstall_openclaw_config, uninstall_plugin_package and remove_manual_daemon_binary but never removes installer-managed runtime/model assets, leaving ~/.local/share/libravdb/onnxruntime and ~/.local/share/libravdb/models behind; add removal of these directories to the uninstall path (either by a new helper like uninstall_libravdb_assets or by extending the existing uninstall sequence) to recursively delete those two paths if present (handle non-existence safely and avoid removing unrelated files). Ensure the new removal is invoked alongside the other cleanup calls so the assets are cleaned up during uninstall.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/auto-install.sh`:
- Around line 1029-1033: The uninstall flow currently calls
stop_daemon_services, uninstall_openclaw_config, uninstall_plugin_package and
remove_manual_daemon_binary but never removes installer-managed runtime/model
assets, leaving ~/.local/share/libravdb/onnxruntime and
~/.local/share/libravdb/models behind; add removal of these directories to the
uninstall path (either by a new helper like uninstall_libravdb_assets or by
extending the existing uninstall sequence) to recursively delete those two paths
if present (handle non-existence safely and avoid removing unrelated files).
Ensure the new removal is invoked alongside the other cleanup calls so the
assets are cleaned up during uninstall.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 145526e5-37fb-4b1f-8667-b5556399bf05
📒 Files selected for processing (21)
README.mddocs/development.mddocs/install.mddocs/installation.mddocs/performance-and-tuning.mddocs/security.mddocs/uninstall.mdpackage.jsonscripts/auto-install.shscripts/build-daemon.shscripts/postinstall.jsscripts/setup.tssrc/index.tssrc/plugin-runtime.tssrc/sidecar.tstest/integration/checklist-validation.test.tstest/integration/sidecar-lifecycle.test.tstest/unit/cli.test.tstest/unit/memory-runtime.test.tstest/unit/plugin-runtime.test.tstest/unit/sidecar.test.ts
✅ Files skipped from review due to trivial changes (9)
- package.json
- test/unit/sidecar.test.ts
- docs/performance-and-tuning.md
- README.md
- src/index.ts
- docs/development.md
- docs/install.md
- docs/installation.md
- test/integration/sidecar-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/unit/plugin-runtime.test.ts
- docs/uninstall.md
- src/sidecar.ts
- docs/security.md
- src/plugin-runtime.ts
|
@compoodment addressed in 3c148db. |
|
Reviewed the follow-up commit. Three fixes all land correctly:
I had a concern about brew error propagation in One minor note (not blocking): LGTM. |
|
you guys do know its is not getting merged, not even close |
Resolve installer conflicts with current plugin-runtime and sidecar tests. Preserve the automated installer provisioning hint and defer sidecar retry reset until the connection stability window so integration remains green.
|
Two blockers on this PR: 1. Merge conflict with current Validated with: bash -n scripts/auto-install.sh
pnpm run check
pnpm run test:integration2. Scope leak — sidecar retry semantics: This installer PR also changes The only sidecar test updates are provisioning-hint/fallback-profile expectations — no deterministic stability-window coverage. This PR can silently merge the #162 behavior without the review/test coverage we're requiring there. Please split/drop the sidecar retry change from this installer PR, or explicitly make this PR own the #132/#162 behavior with the same required coverage:
— Vale |
Review — ValeSummaryThis PR adds an automated installer ( What this does well
Concerns
VerdictThe installer functionality is needed and the Homebrew fallback is a real improvement. But this PR is too broad — it mixes installer, sidecar, and plugin-runtime changes. Recommend splitting into: (a) sidecar stability timer, (b) plugin-runtime error message, (c) installer + docs. The installer script itself needs better testability and a strategy for version pinning. – Vale |
|
Note: PR #231 (merged) deleted several files this PR touches — |
Summary
npx --yes @xdarkicex/openclaw-memory-libravdb --yesplugins.entriesOpenClaw config shapeplugins.slots.memoryandplugins.slots.contextEnginebge-small-en-v1.5Installation issues addressed
openclaw plugins install, which enabled the package but did not assign the required exclusive slots or writeplugins.entries[libravdb-memory].configscripts/auto-install.shwrote staleplugins.configs; it now writesplugins.entrieswhile assigning bothmemoryandcontextEngineslotsreturn 0; failures now return nonzero and trigger fallbackbge-small-en-v1.5for fallback, notall-minilm-l6-v2Verification
bash -n scripts/auto-install.shbash scripts/auto-install.sh --dry-run --yesbash scripts/auto-install.sh --yesnpm pack --dry-runpnpm exec tsc -p tsconfig.tests.json && node --test .ts-build/test/integration/checklist-validation.test.js .ts-build/test/unit/sidecar.test.js .ts-build/test/unit/plugin-runtime.test.js .ts-build/test/integration/sidecar-lifecycle.test.jspnpm checkpnpm run plugin:checkopenclaw memory statusSummary by CodeRabbit
New Features
openclaw memory status.Documentation
Improvements