Point MPG users to canonical Gecode skills repository#5
Point MPG users to canonical Gecode skills repository#5zayenz wants to merge 4 commits intopython-build-scriptsfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38466ae77b
ℹ️ 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".
| DEST="${1:-${CODEX_HOME:-$DEST_DEFAULT}/skills}" | ||
|
|
||
| if [[ "$DEST" == "$HOME/.codex/skills/skills" ]]; then | ||
| DEST="$HOME/.codex/skills" |
There was a problem hiding this comment.
Normalize destination path when CODEX_HOME already ends with skills
The destination is built as ${CODEX_HOME}/skills, but DEST_DEFAULT already includes skills, and the subsequent fix-up only handles the hard-coded $HOME/.codex/skills/skills case. If a user sets CODEX_HOME to a custom path that already points at a skills directory (for example in CI/dev containers), this script installs into <path>/skills/skills, so Codex won’t read the newly copied skills from the expected location.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds the entire
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User wants Gecode skills] --> B{Install method?}
B -->|Canonical| C["npx skills add Gecode/gecode-skills"]
B -->|Local snapshot| D["skills/install-skills.sh"]
C --> E["Gecode/gecode-skills repo"]
E --> F["~/.codex/skills/"]
D --> G["MPG skills/ directory"]
G --> F
H["README.md"] -->|Documents| C
I["skills/README.md"] -->|Clarifies snapshot policy| G
Last reviewed commit: 38466ae |
| DEST_DEFAULT="${HOME}/.codex/skills" | ||
| DEST="${1:-${CODEX_HOME:-$DEST_DEFAULT}/skills}" |
There was a problem hiding this comment.
Fragile default-path computation
DEST_DEFAULT is already $HOME/.codex/skills, so when CODEX_HOME is unset the expression ${CODEX_HOME:-$DEST_DEFAULT}/skills evaluates to $HOME/.codex/skills/skills — a double /skills suffix. The guard on lines 11-13 only catches this specific default; if a user sets CODEX_HOME="$HOME/.codex/skills" the same double-append occurs and the guard won't fire.
Consider dropping the /skills suffix from DEST_DEFAULT so the append on line 9 produces the correct path in all cases:
| DEST_DEFAULT="${HOME}/.codex/skills" | |
| DEST="${1:-${CODEX_HOME:-$DEST_DEFAULT}/skills}" | |
| DEST_DEFAULT="${HOME}/.codex" | |
| DEST="${1:-${CODEX_HOME:-$DEST_DEFAULT}/skills}" |
This also makes the special-case guard on lines 11-13 unnecessary.
| @@ -0,0 +1,43 @@ | |||
| # MPG Case-Study Skill Audit | |||
|
|
|||
| Scope: `/Users/zayenz/gecode/MPG/docs/src/chapters/case-studies/*.tex.in` | |||
There was a problem hiding this comment.
Hardcoded local filesystem path
This line (and lines 37-41 below) contains an absolute path from a development machine (/Users/zayenz/gecode/MPG/...). Since this file is committed to the repository, these should be repository-relative paths instead.
| Scope: `/Users/zayenz/gecode/MPG/docs/src/chapters/case-studies/*.tex.in` | |
| Scope: `docs/src/chapters/case-studies/*.tex.in` |
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-general-knowledge/SKILL.md` | ||
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-modeling/SKILL.md` | ||
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-propagator-implementation/SKILL.md` | ||
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-brancher-implementation/SKILL.md` | ||
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-memory-handling/SKILL.md` |
There was a problem hiding this comment.
More hardcoded local paths
Same issue as line 3 — these absolute paths should be repository-relative for portability.
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-general-knowledge/SKILL.md` | |
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-modeling/SKILL.md` | |
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-propagator-implementation/SKILL.md` | |
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-brancher-implementation/SKILL.md` | |
| - Updated `/Users/zayenz/gecode/MPG/skills/gecode-memory-handling/SKILL.md` | |
| - Updated `skills/gecode-general-knowledge/SKILL.md` | |
| - Updated `skills/gecode-modeling/SKILL.md` | |
| - Updated `skills/gecode-propagator-implementation/SKILL.md` | |
| - Updated `skills/gecode-brancher-implementation/SKILL.md` | |
| - Updated `skills/gecode-memory-handling/SKILL.md` |
Summary
Gecode/gecode-skillsskills/README.mddocumenting that MPGskills/is a reference snapshot, not the publish sourceskills/install-skills.shfor local snapshot installation, with explicit canonical-source noticePolicy Chosen
Gecode/gecode-skillsUser-facing install path
Notes
Gecode/gecode-skills