refactor(skill): apply ce-code-review fixes — bump to 3.2.2, fallback tests, comparison resolver
12 fixes from the multi-agent code review on PR #400: Version 3.2.1 -> 3.2.2 across all manifests (SKILL.md frontmatter + body header, pyproject.toml, .claude-plugin/{plugin,marketplace}.json, sync.sh cache path). The PR ships observable behavior changes (STEP 0 logic flip, resolver order change, badge fallback) that should not silently appear under the same version number — the new fallback reads SKILL.md version directly so the badge would otherwise be misleading. render.py::_skill_version: - `import re` moved to module top - _VERSION_RE extracted as a module-level compiled pattern that accepts double-quoted, single-quoted, and unquoted YAML version scalars - `break` -> `continue` on corrupt manifest, so a corrupt inner manifest no longer shadows a valid outer one - Wrap SKILL.md read_text() in try/except for UnicodeDecodeError to keep badge emission from crashing on mis-encoded SKILL.md - Docstring clarifies precedence; inline comment marks the fallback boundary between the manifest walk and the SKILL.md walk tests/test_skill_version.py (new): 7 unit tests for the fallback paths (manifest absent, manifest corrupt, corrupt-inner + valid-outer, both absent, SKILL.md without version, single-quoted, unquoted). tests/test_plugin_contract.py: tombstone test asserting .codex-plugin/ stays removed (was the only CI guard against accidental reintroduction). SKILL.md: - STEP 0 bash echoes CLAUDE_CACHE_LATEST so the model can see the resolved value when deciding whether to hop - "Both shapes ship in the wild" comment now names the two cache layouts (nested {cache}/{version}/skills/last30days/ vs flat {cache}/{version}/) - Comparison-mode bash invocation gets its own inline SKILL_ROOT resolver (latent gap: the contract tells the model to skip Step 1 on comparison queries, so SKILL_ROOT was previously unset there) CHANGELOG.md: [Unreleased] entries for the resolver rewrite and the breaking removal of Codex native-plugin support. All 9 reviewer personas surfaced findings; 3 cross-reviewer corroboration clusters were promoted (import re, "both shapes" comment, missing fallback tests). Maintainability follow-up flagged: regex now duplicated across render.py and 2 test files; could consolidate via shared lib/skill_meta.py helper in a future PR.
This commit is contained in:
Trevin Chow
@@ -11,7 +11,7 @@
|
||||
{
|
||||
"name": "last30days",
|
||||
"description": "Research any topic across Reddit, X, YouTube, TikTok, Instagram, Hacker News, Polymarket, GitHub, and 5+ more sources. AI agent scores by upvotes, likes, and real money - not editors.",
|
||||
"version": "3.2.1",
|
||||
"version": "3.2.2",
|
||||
"author": {
|
||||
"name": "Matt Van Horn",
|
||||
"url": "https://github.com/mvanhorn"
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "last30days",
|
||||
"version": "3.2.1",
|
||||
"version": "3.2.2",
|
||||
"description": "Research any topic across Reddit, X, YouTube, TikTok, Instagram, Hacker News, Polymarket, GitHub, and 5+ more sources. AI agent scores by upvotes, likes, and real money - not editors.",
|
||||
"author": {
|
||||
"name": "Matt Van Horn",
|
||||
|
||||
@@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
|
||||
- Rename "Digg AI 1000" to just "Digg" in user-facing output (footer line, source label, inline-quote suffix, why_relevant, container attribution). Internal references to the upstream Digg AI 1000 product remain in code comments and docstrings.
|
||||
- Bump `POSTS_PER_CLUSTER` from 3 to 5 and the render-side display limit from 2 to 3 to match the per-source enrichment caps used by Reddit, HN, YouTube, TikTok, and GitHub. The previous 3/2 caps routinely truncated cluster context (e.g. dropped a Jason Calacanis quote tweet on a `cli-printing-press` run).
|
||||
- Rewrite SKILL.md path resolution. STEP 0 narrows from a global canonical-path enforcement to a Claude-Code-marketplaces-only stale-clone guard. Step 1 SKILL_ROOT resolver walks a single precedence list (Claude plugin cache, then `~/.codex/skills/`, `~/.agents/skills/`, repo checkout, `./.skills/last30days` for `npx skills add`, CWD, Gemini). Adds SKILL.md frontmatter fallback to `render.py::_skill_version` so the badge no longer prints `v?` on installs that don't include `.claude-plugin/plugin.json`.
|
||||
|
||||
### Removed
|
||||
|
||||
- **BREAKING for Codex native-plugin users:** `.codex-plugin/plugin.json` and the matching SKILL_ROOT resolver branch in SKILL.md Step 1. Codex users should install via `npx skills add mvanhorn/last30days-skill` or copy the skill to `~/.codex/skills/last30days/` (which `sync.sh` already writes to in this repo).
|
||||
|
||||
## [3.2.0] - 2026-05-09
|
||||
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
[project]
|
||||
name = "last30days-skill"
|
||||
version = "3.2.1"
|
||||
version = "3.2.2"
|
||||
description = "Multi-source last-30-days research skill"
|
||||
readme = "README.md"
|
||||
requires-python = ">=3.12"
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: last30days
|
||||
version: "3.2.1"
|
||||
version: "3.2.2"
|
||||
description: "Research what people actually say about any topic in the last 30 days. Pulls posts and engagement from Reddit, X, YouTube, TikTok, Hacker News, Polymarket, GitHub, and the web."
|
||||
argument-hint: 'last30days nvidia earnings reaction | last30days AI video tools | last30days what users want in react'
|
||||
allowed-tools: Bash, Read, Write, AskUserQuestion, WebSearch
|
||||
@@ -69,6 +69,7 @@ Before reading anything else in this file, check whether you loaded SKILL.md fro
|
||||
```bash
|
||||
CLAUDE_CACHE_LATEST=$(ls -d "$HOME/.claude/plugins/cache/last30days-skill/last30days"/*/ 2>/dev/null | sort -V | tail -1)
|
||||
CLAUDE_CACHE_LATEST="${CLAUDE_CACHE_LATEST%/}"
|
||||
echo "CLAUDE_CACHE_LATEST=$CLAUDE_CACHE_LATEST"
|
||||
```
|
||||
|
||||
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.
|
||||
@@ -233,7 +234,7 @@ If your Bash call to `last30days.py` does NOT include the FULL pre-flight checkl
|
||||
|
||||
---
|
||||
|
||||
# last30days v3.2.1: Research Any Topic from the Last 30 Days
|
||||
# last30days v3.2.2: Research Any Topic from the Last 30 Days
|
||||
|
||||
> **Permissions overview:** Reads public web/platform data and optionally saves research briefings to `LAST30DAYS_MEMORY_DIR` (defaults to `~/Documents/Last30Days`). X/Twitter search uses optional user-provided tokens (AUTH_TOKEN/CT0 env vars). Bluesky search uses optional app password (BSKY_HANDLE/BSKY_APP_PASSWORD env vars - create at bsky.app/settings/app-passwords). All credential usage and data writes are documented in the [Security & Permissions](#security--permissions) section.
|
||||
|
||||
@@ -582,6 +583,30 @@ When the user asks "X vs Y" (or "X vs Y vs Z"), the engine fans out N full `pipe
|
||||
|
||||
**Invocation:**
|
||||
```bash
|
||||
# Comparison mode skips Step 1, so resolve SKILL_ROOT inline here (same precedence
|
||||
# walk as Step 1 — keep the two in sync if you edit either).
|
||||
SKILL_ROOT=""
|
||||
CLAUDE_PLUGIN_ROOT="$(ls -d "$HOME/.claude/plugins/cache/last30days-skill/last30days/"*/ 2>/dev/null | sort -V | tail -1)"
|
||||
CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT%/}"
|
||||
if [ -n "$CLAUDE_PLUGIN_ROOT" ]; then
|
||||
if [ -f "$CLAUDE_PLUGIN_ROOT/skills/last30days/scripts/last30days.py" ]; then
|
||||
SKILL_ROOT="$CLAUDE_PLUGIN_ROOT/skills/last30days"
|
||||
elif [ -f "$CLAUDE_PLUGIN_ROOT/scripts/last30days.py" ]; then
|
||||
SKILL_ROOT="$CLAUDE_PLUGIN_ROOT"
|
||||
fi
|
||||
fi
|
||||
if [ -z "$SKILL_ROOT" ] || [ ! -f "$SKILL_ROOT/scripts/last30days.py" ]; then
|
||||
for dir in \
|
||||
"$HOME/.codex/skills/last30days" \
|
||||
"$HOME/.agents/skills/last30days" \
|
||||
"./skills/last30days" \
|
||||
"./.skills/last30days" \
|
||||
"." \
|
||||
"${GEMINI_EXTENSION_DIR:-}"; do
|
||||
[ -n "$dir" ] && [ -f "$dir/scripts/last30days.py" ] && SKILL_ROOT="$dir" && break
|
||||
done
|
||||
fi
|
||||
|
||||
"${LAST30DAYS_PYTHON}" "${SKILL_ROOT}/scripts/last30days.py" "{TOPIC_A} vs {TOPIC_B} vs {TOPIC_C}" \
|
||||
--emit=compact \
|
||||
--save-dir="${LAST30DAYS_MEMORY_DIR}" \
|
||||
@@ -873,7 +898,8 @@ Store your plan as `QUERY_PLAN_JSON` - you'll pass it to the script in the next
|
||||
# then common per-harness skill dirs, then a repo checkout.
|
||||
SKILL_ROOT=""
|
||||
|
||||
# 1. Claude Code plugin cache (versioned). Both shapes ship in the wild — pick the freshest.
|
||||
# 1. Claude Code plugin cache (versioned, sort -V picks freshest). Two cache layouts ship in the wild:
|
||||
# nested ({cache}/{version}/skills/last30days/scripts/...) and flat ({cache}/{version}/scripts/...).
|
||||
CLAUDE_PLUGIN_ROOT="$(ls -d "$HOME/.claude/plugins/cache/last30days-skill/last30days/"*/ 2>/dev/null | sort -V | tail -1)"
|
||||
CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT%/}"
|
||||
if [ -n "$CLAUDE_PLUGIN_ROOT" ]; then
|
||||
|
||||
@@ -4,6 +4,7 @@ from __future__ import annotations
|
||||
|
||||
import json
|
||||
import pathlib
|
||||
import re
|
||||
from collections import Counter
|
||||
from datetime import date
|
||||
from urllib.parse import urlparse
|
||||
@@ -11,15 +12,23 @@ from urllib.parse import urlparse
|
||||
from . import dates, schema
|
||||
|
||||
|
||||
_VERSION_RE = re.compile(
|
||||
r'''^version:\s*(?:"([^"]+)"|'([^']+)'|(\S+))\s*$''',
|
||||
re.MULTILINE,
|
||||
)
|
||||
|
||||
|
||||
def _skill_version() -> str:
|
||||
"""Read plugin version from .claude-plugin/plugin.json, falling back to SKILL.md frontmatter.
|
||||
|
||||
sync.sh does not copy .claude-plugin/ to non-cache install dirs (~/.codex/skills,
|
||||
~/.agents/skills, Hermes), so SKILL.md frontmatter is the fallback that keeps the
|
||||
badge from emitting v? on those installs. Returns "?" only if both sources are missing.
|
||||
"""
|
||||
import re
|
||||
|
||||
A corrupt manifest at one ancestor does not shadow a valid manifest at a deeper one
|
||||
(continue, not break). YAML frontmatter accepts double-quoted, single-quoted, or
|
||||
unquoted version scalars.
|
||||
"""
|
||||
here = pathlib.Path(__file__).resolve()
|
||||
for parent in here.parents:
|
||||
manifest = parent / ".claude-plugin" / "plugin.json"
|
||||
@@ -27,14 +36,18 @@ def _skill_version() -> str:
|
||||
try:
|
||||
return json.loads(manifest.read_text()).get("version", "?")
|
||||
except (json.JSONDecodeError, OSError):
|
||||
break
|
||||
continue
|
||||
|
||||
# No 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():
|
||||
match = re.search(r'^version:\s*"([^"]+)"\s*$', skill_md.read_text(), re.MULTILINE)
|
||||
try:
|
||||
match = _VERSION_RE.search(skill_md.read_text())
|
||||
except (OSError, UnicodeDecodeError):
|
||||
break
|
||||
if match:
|
||||
return match.group(1)
|
||||
return next(g for g in match.groups() if g is not None)
|
||||
break
|
||||
return "?"
|
||||
|
||||
|
||||
@@ -15,7 +15,7 @@ COMMON_TARGETS=(
|
||||
# against the working tree so /last30days reflects local dev without
|
||||
# waiting for a release. Do NOT add ~/.claude/skills/last30days - it
|
||||
# creates a duplicate slash-command entry alongside the plugin version.
|
||||
"$HOME/.claude/plugins/cache/last30days-skill/last30days/3.2.1"
|
||||
"$HOME/.claude/plugins/cache/last30days-skill/last30days/3.2.2"
|
||||
"$HOME/.agents/skills/last30days"
|
||||
"$HOME/.codex/skills/last30days"
|
||||
)
|
||||
|
||||
@@ -22,6 +22,12 @@ def _skill_version() -> str:
|
||||
|
||||
|
||||
class TestPluginContract(unittest.TestCase):
|
||||
def test_codex_plugin_scaffold_stays_removed(self) -> None:
|
||||
# .codex-plugin/ was removed in the resolver-collapse refactor; Codex users
|
||||
# install via `npx skills add` or `~/.codex/skills/`. A reintroduction would
|
||||
# silently fork the install surface.
|
||||
self.assertFalse((ROOT / ".codex-plugin").exists())
|
||||
|
||||
def test_versions_match_across_manifests(self) -> None:
|
||||
pyproject = tomllib.loads((ROOT / "pyproject.toml").read_text(encoding="utf-8"))
|
||||
version = pyproject["project"]["version"]
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
"""Unit tests for render._skill_version() fallback paths.
|
||||
|
||||
The function reads version from .claude-plugin/plugin.json first, then falls back
|
||||
to SKILL.md frontmatter. These tests use monkeypatch to swap the render module's
|
||||
__file__ attribute, which controls where the walk starts.
|
||||
"""
|
||||
|
||||
import sys
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "skills" / "last30days" / "scripts"))
|
||||
|
||||
from lib import render
|
||||
|
||||
|
||||
class SkillVersionFallbackTests(unittest.TestCase):
|
||||
def setUp(self):
|
||||
# tmp_path equivalent for unittest
|
||||
import tempfile
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.tmp_path = Path(self._tmp.name)
|
||||
|
||||
def tearDown(self):
|
||||
self._tmp.cleanup()
|
||||
|
||||
def _make_render_at(self, parent: Path) -> Path:
|
||||
"""Place a dummy render.py inside parent and return its path."""
|
||||
parent.mkdir(parents=True, exist_ok=True)
|
||||
fake_render = parent / "render.py"
|
||||
fake_render.write_text("")
|
||||
return fake_render
|
||||
|
||||
def _write_manifest(self, parent: Path, version: str | None) -> None:
|
||||
"""Write .claude-plugin/plugin.json under parent. version=None writes corrupt JSON."""
|
||||
d = parent / ".claude-plugin"
|
||||
d.mkdir(parents=True, exist_ok=True)
|
||||
if version is None:
|
||||
(d / "plugin.json").write_text("{not valid json")
|
||||
else:
|
||||
(d / "plugin.json").write_text(f'{{"version": "{version}"}}')
|
||||
|
||||
def _write_skill_md(self, parent: Path, frontmatter_version_line: str | None) -> None:
|
||||
"""Write SKILL.md with frontmatter. None writes a SKILL.md with no version line."""
|
||||
if frontmatter_version_line is None:
|
||||
body = "---\nname: test\n---\n# body\n"
|
||||
else:
|
||||
body = f"---\nname: test\n{frontmatter_version_line}\n---\n# body\n"
|
||||
(parent / "SKILL.md").write_text(body)
|
||||
|
||||
def test_manifest_absent_falls_back_to_skill_md_frontmatter(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, 'version: "9.9.9"')
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("9.9.9", render._skill_version())
|
||||
|
||||
def test_manifest_corrupt_falls_back_to_skill_md_frontmatter(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_manifest(skill_dir, version=None) # corrupt
|
||||
self._write_skill_md(skill_dir, 'version: "8.8.8"')
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("8.8.8", render._skill_version())
|
||||
|
||||
def test_corrupt_inner_manifest_does_not_shadow_valid_outer_manifest(self):
|
||||
outer = self.tmp_path / "outer"
|
||||
inner = outer / "skill_root"
|
||||
fake_render = self._make_render_at(inner)
|
||||
self._write_manifest(inner, version=None) # corrupt at inner
|
||||
self._write_manifest(outer, version="7.7.7") # valid at outer
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("7.7.7", render._skill_version())
|
||||
|
||||
def test_neither_source_present_returns_question_mark(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
# No manifest, no SKILL.md anywhere under tmp_path
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("?", render._skill_version())
|
||||
|
||||
def test_skill_md_without_version_returns_question_mark(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, frontmatter_version_line=None)
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("?", render._skill_version())
|
||||
|
||||
def test_unquoted_yaml_version_is_accepted(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, "version: 6.6.6")
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("6.6.6", render._skill_version())
|
||||
|
||||
def test_single_quoted_yaml_version_is_accepted(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, "version: '5.5.5'")
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("5.5.5", render._skill_version())
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user