fix: ensure_skills_clone helper semantics + correct operator messaging#69
Conversation
Fixes 5 critical issues found in Codex review of PR #68: 1. Git fetch doesn't update working copy: Changed 'git fetch + checkout' to 'git reset --hard origin/$branch' so re-running pack gets fresh skills 2. Helper destroys working skills on failure: Only move-aside existing if clone succeeds; in warn mode preserve stale checkout over nothing 3. Silent failures: All 5 packs now check return code before logging success; uses 'ok' on success, 'warn' on optional failure 4. Pi messaging contradicts spec: Updated to reflect that pi auto-discovers SKILL.md folders, not just reference docs 5. IronClaw points to non-guaranteed path: Changed from '~/.openclaw/workspace/skills/' to loki-skills repo link Helper now preserves backups and prevents destructive updates in warn mode. All pack callers use truthful status messages.
![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: 9176790bc3
ℹ️ 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 | ||
| log "Skills repo update failed; keeping existing checkout (warn mode)" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
When git fetch/reset fails for an existing skills checkout, this branch returns success (0) in warn mode, so callers print a green “Skills installed” message even though no update occurred. In the current install scripts, that hides real update failures (for example, transient network/auth errors or a damaged local repo) and prevents the fallback clone path from running, leaving stale or broken skills while reporting success.
Useful? React with 👍 / 👎.
1 participant
Fixes from Codex Review of PR #68
5 critical production issues addressed:
1. Git fetch doesn't update working copy
git fetch origin + git checkout→git reset --hard origin/$branch2. Helper destroys working skills on failure
3. Silent failures everywhere
ensure_skills_clonereturn code before loggingokon success,warnon optional failure4. Pi messaging contradicts spec
SKILL.mdfolders)5. IronClaw points to non-guaranteed path
~/.openclaw/workspace/skills/(other pack)Verification
✅ Codex review: 0 issues
✅ Architect review: 0 issues
All PR #68 paths remain correct. Helper is now production-safe.