fix(skill): address greptile P1+P2 review feedback on PR #400
Two real bugs flagged in the automated review of PR #400; both small. 1. render.py::_skill_version manifest with no "version" key `json.loads(manifest.read_text()).get("version", "?")` returned "?" immediately on a valid JSON manifest that lacked the "version" key, never falling through to the SKILL.md frontmatter fallback. Contradicted the docstring's "Returns '?' only if both sources are missing" contract. Same shape if version is present but empty string ("" produces the broken badge `🌐 last30days v · synced ...`). Fix: pull the version out of the parsed dict, then `continue` to the next ancestor if it's None or empty. Falls through to the SKILL.md walk only after exhausting every ancestor. 2. SKILL.md STEP 0 re-read target hardcoded to nested cache layout STEP 0 told the model to re-read from `$CLAUDE_CACHE_LATEST/skills/last30days/SKILL.md` — the new nested layout. But Step 1's resolver explicitly handles both shapes (nested `{cache}/{version}/skills/last30days/` and flat `{cache}/{version}/`), noting "Both shapes ship in the wild." On an install where the highest-versioned cache happens to be the older flat shape, STEP 0's re-read target wouldn't exist; the model would silently stay on the stale marketplaces/ copy STEP 0 was supposed to move it away from — the exact failure mode this guard was added to prevent. Fix: extend the STEP 0 bash to resolve $CLAUDE_CACHE_SKILL_MD by probing both layouts, then have the model hop to that resolved path instead of constructing the path from a hardcoded suffix. Two new tests in tests/test_skill_version.py cover the missing-key and empty-string cases for fix 1. Fix 2 is exercised via the bash probe at verify time (the STEP 0 prose-contract test isn't unit-testable from Python, but the dual-layout bash is verified to resolve to the correct SKILL.md on both shapes). Stale finding skipped: greptile also flagged a missing try/except on the SKILL.md read_text() call, but that was already addressed during the ce-code-review safe_auto pass earlier in this PR — current code wraps it in `try/except (OSError, UnicodeDecodeError)`, strictly more defensive than the suggested fix.
This commit is contained in:
@@ -68,11 +68,20 @@ Before reading anything else in this file, check whether you loaded SKILL.md fro
|
||||
|
||||
```bash
|
||||
CLAUDE_CACHE_LATEST=$(find "$HOME/.claude/plugins/cache/last30days-skill/last30days" -mindepth 1 -maxdepth 1 -type d 2>/dev/null | sort -V | tail -1)
|
||||
CLAUDE_CACHE_LATEST="${CLAUDE_CACHE_LATEST%/}"
|
||||
echo "CLAUDE_CACHE_LATEST=$CLAUDE_CACHE_LATEST"
|
||||
# Two cache layouts ship in the wild — nested ({version}/skills/last30days/SKILL.md)
|
||||
# and flat ({version}/SKILL.md). Resolve to whichever shape actually exists.
|
||||
CLAUDE_CACHE_SKILL_MD=""
|
||||
if [ -n "$CLAUDE_CACHE_LATEST" ]; then
|
||||
if [ -f "$CLAUDE_CACHE_LATEST/skills/last30days/SKILL.md" ]; then
|
||||
CLAUDE_CACHE_SKILL_MD="$CLAUDE_CACHE_LATEST/skills/last30days/SKILL.md"
|
||||
elif [ -f "$CLAUDE_CACHE_LATEST/SKILL.md" ]; then
|
||||
CLAUDE_CACHE_SKILL_MD="$CLAUDE_CACHE_LATEST/SKILL.md"
|
||||
fi
|
||||
fi
|
||||
echo "CLAUDE_CACHE_SKILL_MD=$CLAUDE_CACHE_SKILL_MD"
|
||||
```
|
||||
|
||||
If the SKILL.md path you just Read contains `/.claude/plugins/marketplaces/` AND `$CLAUDE_CACHE_LATEST` is non-empty, STOP and re-read `$CLAUDE_CACHE_LATEST/skills/last30days/SKILL.md` before proceeding. Otherwise the SKILL.md you have is fine — continue.
|
||||
If the SKILL.md path you just Read contains `/.claude/plugins/marketplaces/` AND `$CLAUDE_CACHE_SKILL_MD` is non-empty, STOP and re-read `$CLAUDE_CACHE_SKILL_MD` before proceeding. Otherwise the SKILL.md you have is fine — continue.
|
||||
|
||||
**Why this specific check:** `~/.claude/plugins/marketplaces/last30days-skill/` is a git clone Claude Code auto-restores to `origin/main` on session start. It can lag the versioned cache by one or more releases. Three 2026-04-22 test runs (Linear, Coinbase) loaded SKILL.md from `marketplaces/`, ran `--help` from the same stale path, did not see the `--competitors` flag that existed in the cache, and fell back to a manual comparison plan. Result: 2 of 3 windows never invoked the feature they were asked to test. STEP 0 defends against that one Claude Code-specific bug.
|
||||
|
||||
|
||||
@@ -34,11 +34,13 @@ def _skill_version() -> str:
|
||||
manifest = parent / ".claude-plugin" / "plugin.json"
|
||||
if manifest.is_file():
|
||||
try:
|
||||
return json.loads(manifest.read_text()).get("version", "?")
|
||||
version = json.loads(manifest.read_text()).get("version")
|
||||
except (json.JSONDecodeError, OSError):
|
||||
continue
|
||||
if version:
|
||||
return version
|
||||
|
||||
# No manifest found at any ancestor — fall back to SKILL.md frontmatter.
|
||||
# No usable manifest found at any ancestor — fall back to SKILL.md frontmatter.
|
||||
for parent in here.parents:
|
||||
skill_md = parent / "SKILL.md"
|
||||
if skill_md.is_file():
|
||||
|
||||
Reference in New Issue
Block a user