fix: add missing ensure_skills_clone helper function#67
Merged
Conversation
Codex P1: PR #66 calls ensure_skills_clone() in all pack installers but the function was never defined in packs/common.sh, causing 'command not found' silently wrapped in || true. This adds the helper with: - Git availability check (skips gracefully if missing) - Update existing repo (same origin fetch + checkout) - Partial dir recovery (move aside + fresh clone) - Shallow clone (--depth 1 to minimize bandwidth) - Non-fatal error handling (warn mode default, fail mode optional) All pack installers can now properly clone loki-skills on bootstrap.
![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: e5c19ea798
ℹ️ 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".
| local origin_url="$(cd "$target_dir" && git config --get remote.origin.url 2>/dev/null)" | ||
| if [[ "$origin_url" == "$repo_url" ]]; then | ||
| # Same origin, safe to update | ||
| if (cd "$target_dir" && git fetch origin "$branch" && git checkout "$branch") &>/dev/null; then |
There was a problem hiding this comment.
git fetch origin "$branch" && git checkout "$branch" does not move an existing local branch to the newly fetched origin/$branch, so this path can return success while leaving an outdated skills checkout. In practice, if the local branch is behind, the function logs "updated" and exits 0 even though no new commits were applied; use a fast-forward/pull/reset step after fetch to ensure the working tree matches the remote tip.
Useful? React with 👍 / 👎.
| fi | ||
|
|
||
| # Fresh clone (shallow) | ||
| if ! git clone --depth 1 --branch "$branch" "$repo_url" "$target_dir" &>/dev/null; then |
There was a problem hiding this comment.
The fresh-clone path calls git clone directly into target_dir but never creates $(dirname "$target_dir"). On first-run installs where that parent path does not exist (for example ~/.local/share/lowkey/skills), clone fails immediately and skills are not installed, even though the installers continue via || true.
Useful? React with 👍 / 👎.
royosherove
added a commit
that referenced
this pull request
May 18, 2026
PR #66+#67 shipped with incorrect skills directory paths for several packs. This correction PR aligns all pack install scripts with official documentation verified via Tavily web search (2026-05-18). **Changes:** packs/pi/install.sh: - pi/agent/skills/ → pi/agent/extensions/ - Pi loads TypeScript extensions from extensions/, not direct skills packs/hermes/install.sh: - ~/.local/share/lowkey/skills → ~/.hermes/skills/ - Hermes auto-discovers from ~/.hermes/skills/ with slash-command triggers packs/codex-cli/install.sh: - ~/.local/share/lowkey/skills → ~/.codex/skills/ - Codex auto-discovers SKILL.md files from ~/.codex/skills/ on startup - Invoke with $<skill-name> packs/claude-code/install.sh: - ~/.local/share/lowkey/skills → ~/.claude/skills/ - Claude Code auto-discovers SKILL.md from ~/.claude/skills/ - Invoke with / prefix + always-on context via CLAUDE.md packs/ironclaw/install.sh: - Remove skills pre-install (MCP-native only) - IronClaw extends via MCP servers, not local skills - Reference docs: BOOTSTRAP-MCPORTER.md **Verified paths (per official docs):** - openclaw: ~/.openclaw/workspace/skills/ ✅ (unchanged) - pi: ~/.pi/agent/extensions/ (was skills/) - hermes: ~/.hermes/skills/ (was shared cache) - codex-cli: ~/.codex/skills/ (was shared cache) - claude-code: ~/.claude/skills/ (was shared cache) - ironclaw: MCP only (removed pre-install) - kiro-cli: MCP servers (deferred for Phase 3) - nemoclaw: inherits openclaw pattern **Testing:** - All 6 affected pack install.sh scripts validated - ensure_skills_clone() helper still works with new paths - Non-fatal error handling preserved Fixes: PR #66+#67 path mismatch
royosherove
added a commit
that referenced
this pull request
May 18, 2026
* fix: add missing ensure_skills_clone helper function Codex P1: PR #66 calls ensure_skills_clone() in all pack installers but the function was never defined in packs/common.sh, causing 'command not found' silently wrapped in || true. This adds the helper with: - Git availability check (skips gracefully if missing) - Update existing repo (same origin fetch + checkout) - Partial dir recovery (move aside + fresh clone) - Shallow clone (--depth 1 to minimize bandwidth) - Non-fatal error handling (warn mode default, fail mode optional) All pack installers can now properly clone loki-skills on bootstrap. * fix: correct skills paths for all packs per official documentation PR #66+#67 shipped with incorrect skills directory paths for several packs. This correction PR aligns all pack install scripts with official documentation verified via Tavily web search (2026-05-18). **Changes:** packs/pi/install.sh: - pi/agent/skills/ → pi/agent/extensions/ - Pi loads TypeScript extensions from extensions/, not direct skills packs/hermes/install.sh: - ~/.local/share/lowkey/skills → ~/.hermes/skills/ - Hermes auto-discovers from ~/.hermes/skills/ with slash-command triggers packs/codex-cli/install.sh: - ~/.local/share/lowkey/skills → ~/.codex/skills/ - Codex auto-discovers SKILL.md files from ~/.codex/skills/ on startup - Invoke with $<skill-name> packs/claude-code/install.sh: - ~/.local/share/lowkey/skills → ~/.claude/skills/ - Claude Code auto-discovers SKILL.md from ~/.claude/skills/ - Invoke with / prefix + always-on context via CLAUDE.md packs/ironclaw/install.sh: - Remove skills pre-install (MCP-native only) - IronClaw extends via MCP servers, not local skills - Reference docs: BOOTSTRAP-MCPORTER.md **Verified paths (per official docs):** - openclaw: ~/.openclaw/workspace/skills/ ✅ (unchanged) - pi: ~/.pi/agent/extensions/ (was skills/) - hermes: ~/.hermes/skills/ (was shared cache) - codex-cli: ~/.codex/skills/ (was shared cache) - claude-code: ~/.claude/skills/ (was shared cache) - ironclaw: MCP only (removed pre-install) - kiro-cli: MCP servers (deferred for Phase 3) - nemoclaw: inherits openclaw pattern **Testing:** - All 6 affected pack install.sh scripts validated - ensure_skills_clone() helper still works with new paths - Non-fatal error handling preserved Fixes: PR #66+#67 path mismatch --------- Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.com>
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.
P1 Fix: Codex detected that PR #66 calls
ensure_skills_clone()in all pack installers but the function was never defined in packs/common.sh, causing 'command not found' errors silently wrapped in|| true.Changes
ensure_skills_clone()helper to packs/common.shImpact