From 2f277dfc66c3b96c5a912db50a88095f4dcd18fb Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 15 May 2026 22:34:52 -0700 Subject: [PATCH] fix(skill): address greptile P1+P2 review feedback on PR #400 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- skills/last30days/SKILL.md | 15 ++++++++++++--- skills/last30days/scripts/lib/render.py | 6 ++++-- tests/test_skill_version.py | 24 ++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/skills/last30days/SKILL.md b/skills/last30days/SKILL.md index be86600..3075933 100644 --- a/skills/last30days/SKILL.md +++ b/skills/last30days/SKILL.md @@ -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. diff --git a/skills/last30days/scripts/lib/render.py b/skills/last30days/scripts/lib/render.py index f4c977e..7e4aeff 100644 --- a/skills/last30days/scripts/lib/render.py +++ b/skills/last30days/scripts/lib/render.py @@ -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(): diff --git a/tests/test_skill_version.py b/tests/test_skill_version.py index d36d16e..f7a8e07 100644 --- a/tests/test_skill_version.py +++ b/tests/test_skill_version.py @@ -66,6 +66,30 @@ class SkillVersionFallbackTests(unittest.TestCase): with patch.object(render, "__file__", str(fake_render)): self.assertEqual("8.8.8", render._skill_version()) + def test_manifest_missing_version_key_falls_back_to_skill_md_frontmatter(self): + # Valid JSON, but no "version" key. Old behavior returned "?" and never tried + # SKILL.md. Fix from greptile review: fall through to SKILL.md fallback. + skill_dir = self.tmp_path / "skill_root" + fake_render = self._make_render_at(skill_dir) + (skill_dir / ".claude-plugin").mkdir() + (skill_dir / ".claude-plugin" / "plugin.json").write_text('{"name": "x"}') + self._write_skill_md(skill_dir, 'version: "4.4.4"') + + with patch.object(render, "__file__", str(fake_render)): + self.assertEqual("4.4.4", render._skill_version()) + + def test_manifest_empty_version_string_falls_back_to_skill_md_frontmatter(self): + # Manifest version present but empty โ€” treat as missing so the badge + # doesn't emit a useless "๐ŸŒ last30days v ยท synced ..." line. + skill_dir = self.tmp_path / "skill_root" + fake_render = self._make_render_at(skill_dir) + (skill_dir / ".claude-plugin").mkdir() + (skill_dir / ".claude-plugin" / "plugin.json").write_text('{"version": ""}') + self._write_skill_md(skill_dir, 'version: "2.2.2"') + + with patch.object(render, "__file__", str(fake_render)): + self.assertEqual("2.2.2", render._skill_version()) + def test_corrupt_inner_manifest_does_not_shadow_valid_outer_manifest(self): outer = self.tmp_path / "outer" inner = outer / "skill_root"