feat(openclaw): pre-install loki-skills library on bootstrap#64
Conversation
Skills now ship with the openclaw pack out of the box. Clones inceptionstack/loki-skills into ~/.openclaw/workspace/skills during pack install (Phase 2), so OpenClaw's skill auto-discovery picks them up immediately without needing the manual BOOTSTRAP-SKILLS.md flow. - packs/common.sh: add shared LOKI_SKILLS_REPO_URL constant. This is the only shared piece -- each pack owns its own install step so pack-specific layout / wiring can diverge (Hermes external_dirs, Pi extensions, IronClaw MCP, etc). - packs/openclaw/install.sh: clone (or fast-forward) skills repo right after workspace setup. Writes the same .bootstrapped-skills marker BOOTSTRAP-SKILLS.md uses so the manual first-boot flow becomes a no-op. Best-effort: clone failure does not fail install. Other packs (hermes, nemoclaw, pi, ironclaw, kiro-cli, codex-cli, roundhouse, claude-code) unchanged for now; add the equivalent step to their own install.sh when ready.
…ys write marker Address codex review on PR #64: - High: previous code permanently wedged after a partial clone where ${SKILLS_DIR} existed but had no .git inside. Now: detect that case, rmdir if empty (leave alone if non-empty + warn), then re-test and fresh-clone. On clone failure, rm -rf the partial dir so the next pack run starts clean. - Medium: marker was only written on a fresh clone. A pre-seeded / manually-cloned skills tree never flipped the manual first-boot flow to a no-op. Now: write_skills_marker() runs on every successful path (fresh clone, fast-forward, untouched-because-mismatch). - Medium: previous code blindly 'git pull' on whatever repo lived at ${SKILLS_DIR}. Now: read remote.origin.url and verify it matches ${LOKI_SKILLS_REPO_URL} before pulling; if mismatch, leave the tree untouched and warn (still write the marker, recording the source). The Low (env-overridable repo URL) is intentional -- it's the documented override knob for forks/mirrors and is only consumable on the install host where the operator already has root.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e74ae5f855
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| EXISTING_ORIGIN="$(git -C "${SKILLS_DIR}" config --get remote.origin.url 2>/dev/null || echo '')" | ||
| if [[ "${EXISTING_ORIGIN}" != "${LOKI_SKILLS_REPO_URL}" ]]; then | ||
| warn "loki-skills origin mismatch (expected ${LOKI_SKILLS_REPO_URL}, found ${EXISTING_ORIGIN:-none}) -- leaving existing tree untouched" | ||
| write_skills_marker "existing:${EXISTING_ORIGIN:-unknown}" |
There was a problem hiding this comment.
Do not mark skills as bootstrapped when the existing skills checkout points at a different origin. In this branch the script intentionally leaves the tree untouched, but write_skills_marker still creates memory/.bootstrapped-skills, and deploy/brain/BOOTSTRAP.md treats that marker as “skills setup complete.” On hosts where ~/.openclaw/workspace/skills is an unrelated repo, users will skip the manual bootstrap flow and never get the expected loki-skills library.
Useful? React with 👍 / 👎.
Apply clean-code review (no behavior change): - Extract Method: replace the 50-line procedural if/elif/elif block at the top level with named functions, each doing one thing (skills_write_marker, skills_origin_url, skills_origin_matches_expected, skills_count_entries, skills_dir_is_empty, skills_update_existing, skills_prepare_for_fresh_clone, skills_fresh_clone, skills_install). - Newspaper Rule: install.sh top-level reads as 'step / skills_install', with implementation details below. The state machine is now explicit in skills_install -- no implicit cross-block coupling, no 'Re-test' comment-as-deodorant. - Single guard: 'command -v git' is checked once in skills_install instead of twice across separate if blocks. - Self-documenting predicates: 'skills_origin_matches_expected', 'skills_dir_is_empty' replace inline expression conditions. Verified: bash -n clean, shellcheck clean for the new block, pack tests 60/0/0 pass, plus a 6-scenario harness covering fresh clone, fast-forward, origin-mismatch refusal, partial-dir self-heal, non-empty non-repo preservation, and missing-git graceful skip.
Final review (codex) caught: writing .bootstrapped-skills on the origin-mismatch path tells the bootstrap flow 'skills already done', which suppresses BOOTSTRAP-SKILLS.md as a recovery path even though the canonical loki-skills tree is NOT installed. A repointed origin means the user opted out of canonical. Leave the marker absent so the manual recovery path remains available; warn explicitly that the marker was NOT written. Verified via the harness: T3 (mismatch) now ends with marker absent; T1 (fresh clone) and T2 (fast-forward) still write it.
… kill pack install Belt-and-suspenders for the best-effort contract. Even with the per-branch error handling, set -euo pipefail could still fire on something unexpected inside skills_install or its helpers (e.g., pipefail on find|wc, an unguarded command-sub in a future helper, or a bug in this code). Wrap the orchestrator call in a subshell + ||-warn so ANY failure -- errexit, pipefail, explicit exit, etc -- is contained and the pack install proceeds. Verified: forcing 'exit 99' inside skills_install now produces only a warn, pack install continues, parent script exits 0.
Address codex review on commit 16496b9: - Comment was overclaiming: subshell + ||-guard contains RUNTIME failures (errexit, pipefail, exit, signals, 127s) but NOT parse-time syntax errors in install.sh itself -- those must be caught by bash -n in CI. Comment now reflects this accurately. - Warn message dropped the failing exit status, making ops triage harder. Capture rc via '( cmd ) || _rc=$?' (the if-! form would have flipped $? to 0) and surface it in the warn, plus point at BOOTSTRAP-SKILLS.md as the documented recovery path. - Note: the bash gotcha that errexit is disabled inside subshells whose result is tested by ||/&&/if is documented but doesn't matter here -- every external command in skills_install and its helpers is already wrapped in explicit if/else or '|| true', so failures propagate via tested return values. inherit_errexit does NOT override this and would not help. Verified: rc=99 from explicit 'exit 99' is now reported correctly (was rc=0 with the if-! form). Pack tests still 60/0/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71ff2c3eb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else | ||
| warn "loki-skills fast-forward failed -- keeping existing copy" | ||
| fi | ||
| skills_write_marker "${LOKI_SKILLS_REPO_URL}" |
There was a problem hiding this comment.
Only successful installs should create memory/.bootstrapped-skills, but skills_update_existing writes the marker even when git pull --ff-only fails. This can happen when ~/.openclaw/workspace/skills is a git repo with matching origin but no valid/upstream checkout (e.g., interrupted or manually initialized repo), leaving no usable skills while BOOTSTRAP logic skips recovery because the marker now exists.
Useful? React with 👍 / 👎.
Address codex inline P2 on commit 71ff2c3 (PR #64): skills_update_existing wrote the .bootstrapped-skills marker even when 'git pull --ff-only' failed. If the repo was corrupt/empty (interrupted clone, bare 'git init' with matching origin), the marker would suppress BOOTSTRAP-SKILLS.md recovery while leaving the agent with no actual skills. Now: only write the marker when fast-forward fails IF the tree has non-empty content (entries > 0). An empty tree -> no marker -> manual recovery path remains available. Verified via the harness: corrupt empty repo with matching origin now leaves the marker absent. Pack tests still 60/0/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c4eef3915
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Do NOT write the bootstrap marker here. A repointed origin means the | ||
| # canonical loki-skills tree is not installed; leaving the marker absent | ||
| # preserves the manual BOOTSTRAP-SKILLS.md recovery path for the agent. | ||
| warn "loki-skills origin mismatch (expected ${LOKI_SKILLS_REPO_URL}, found ${origin:-none}) -- leaving existing tree untouched, skills marker NOT written" | ||
| return 0 |
There was a problem hiding this comment.
When skills_update_existing detects an origin mismatch it returns success without touching memory/.bootstrapped-skills, so a marker written by an earlier successful run remains in place. In that state deploy/brain/BOOTSTRAP.md still sees the marker and skips the manual skills bootstrap, even though this branch now considers the current checkout non-canonical and refuses to update it. Deleting (or invalidating) the marker on this path would preserve the documented recovery flow.
Useful? React with 👍 / 👎.
#65) The openclaw pack now pre-installs the loki-skills library on EC2 bootstrap (PR #64), so the manual BOOTSTRAP-SKILLS.md flow is no longer essential -- it's a recovery path. - bootstraps/essential/BOOTSTRAP-SKILLS.md -> bootstraps/optional/BOOTSTRAP-SKILLS.md - Reframe the file's preamble: 'Optional / Recovery', list the recovery triggers (missing git, clone failure, origin mismatch, non-openclaw pack), tell users to skip if .bootstrapped-skills already exists. - README.md: drop from essential list, add to optional list with a note that the openclaw pack auto-installs. - deploy/README.md: drop from essential bootstrap table. - wiki/Bootstrap-Scripts-Guide.md: move section from Essential to Optional, update description to describe it as manual/recovery. - bootstraps/essential/BOOTSTRAP-MCPORTER.md: update cross-reference path to ../optional/BOOTSTRAP-SKILLS.md. deploy/brain/BOOTSTRAP.md unchanged: it points at an external URL on inceptionstack/loki-agent (different repo), and the agent now skips that step naturally because the openclaw pack pre-writes the .bootstrapped-skills marker. Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.com>
1 participant
What
OpenClaw pack now clones
inceptionstack/loki-skillsinto~/.openclaw/workspace/skillsduring pack install, so skills are available out-of-the-box without running the manualBOOTSTRAP-SKILLS.mdflow.How
packs/common.sh— add sharedLOKI_SKILLS_REPO_URLconstant (override-able via env). This is the only shared piece.packs/openclaw/install.sh— pack-specific install step after workspace setup. Clones on first run, fast-forwards on re-runs. Writes the.bootstrapped-skillsmarker so the first-boot bootstrap doc becomes a no-op.Why this shape (per Roy's directive)
Each pack will own its own install step (Hermes
external_dirs, Pi extensions, IronClaw MCP wiring all differ). Only the repo URL is shared incommon.sh.Scope
openclaw— added nowhermes,nemoclaw,pi,ironclaw,kiro-cli,codex-cli,roundhouse,claude-code— to follow per-packSafety
warn+ continue (does not fail pack install).gitdir → fast-forwardgitis missing (it's installed bybootstrap.shPhase 1, so this is defense-in-depth)LOKI_SKILLS_REPO_URLenv varTested
bash -nsyntax check on both files ✅