Asserts compute_save_path_display() never returns a backslash when the
save_dir is under the user's home directory, regardless of host OS.
Reproduces the original bug on Windows (failed message before the fix:
AssertionError: '\' unexpectedly found in
'~/l30d_save_path__luu2g76\Documents\Last30Days\british-airways-middle-east-raw-v3.md'
)
and locks in the contract on POSIX hosts (passes trivially today; would
fail if anyone removes .as_posix() in the future).
Verified by temporarily reverting the fix and confirming RED, then
re-applying the fix and confirming GREEN. All 13 CliV3Tests pass.
PR #366 routes Reddit URLs found in web-search results through the public
Reddit JSON API to recover thread body + top comments (the Claude Code
WebFetch tool blocks reddit.com directly). That bypass is sound and the
fixed problem is real - but the always-on shape ignores user intent on
source gating.
A user who sets EXCLUDE_SOURCES=reddit to suppress Reddit results would
still get Reddit content smuggled back in via web-search URLs that
happen to point at reddit.com threads. This contradicts the suppression
contract that EXCLUDE_SOURCES is supposed to provide (see
lib/pipeline.available_sources where the same env var gates the
top-level Reddit source).
Add a _reddit_excluded(config) check in web_search() that mirrors the
parsing pattern from lib/pipeline (comma-separated, case-insensitive,
whitespace-tolerant). When reddit is in EXCLUDE_SOURCES, skip the
enrichment pass entirely - the web results themselves still flow
through, but they're not augmented with Reddit body/comments.
Four new tests in test_grounding_v3.py cover:
- EXCLUDE_SOURCES=reddit skips enrichment
- case-insensitive parsing matches REDDIT/Reddit/whitespace-padded/csv
- Other sources in EXCLUDE_SOURCES don't trigger the gate
- Enrichment runs normally when reddit isn't excluded
19/19 grounding tests pass.
Addresses two concerns surfaced during PR #376 review:
1. **SSH option-injection on the host value.** The original PR uses
shlex.quote() on the remote command and added a `--` option terminator
in front of the host, but neither one stops a hostile env var like
`LAST30DAYS_YT_SSH_HOST=-oProxyCommand=...` from being read in the
first place. Tighten `_ytdlp_ssh_host()` to validate the host against
`^[a-zA-Z0-9._-]+$` (plain hostname/SSH-config-alias shape: letters,
digits, dot, underscore, hyphen). Any value that doesn't match logs a
warning to stderr and returns None, so the wrap function falls back to
local execution. The `--` terminator stays as defense-in-depth for the
case where a valid host happens to start with `-`, but the regex closes
the door on the env var reaching ssh at all.
2. **Env var naming consistency.** Existing skill-internal config knobs
spell out their domain: `LAST30DAYS_X_BACKEND`, `LAST30DAYS_X_MODEL`,
`LAST30DAYS_PLANNER_MODEL`, `LAST30DAYS_RERANK_MODEL`, etc. The module
is `youtube_yt.py`, the source key is `youtube`, the function family
is `is_youtube_*()` — `YT` was the odd abbreviation out. Rename to
`LAST30DAYS_YOUTUBE_SSH_HOST` so the variable matches the user mental
model ("route YouTube fetches via residential IP") and the codebase's
spelled-out convention.
Adds three new tests:
- test_host_alias_with_dash_prefix_is_rejected (validator rejects `-o...`)
- test_host_alias_with_shell_metacharacters_is_rejected (rejects spaces, ;, $, `, &)
- test_host_alias_validator_accepts_realistic_aliases (allows FQDNs, IPs, bare aliases)
The existing test_wrap_cmd_uses_option_terminator is rewritten to use a
valid host value (since an invalid one is now filtered upstream) and
continues to assert the `--` terminator placement as defense-in-depth.
44/44 youtube_yt tests pass (40 prior + 4 net new validator tests).
Three changes from automated review on PR #376:
1. Add `--` option terminator before host in _wrap_ytdlp_cmd (P1 security)
Prevents SSH option injection if LAST30DAYS_YT_SSH_HOST were ever set
to a value starting with `-` (e.g. `-oProxyCommand=...`). Low
exploitability since the env var is user-controlled config — but the
fix is a single arg and turns a self-harm footgun into no footgun.
2. Hoist `import shlex` to module-level (P2 style)
Pure stdlib import, no reason for the deferred form. Cleaner.
3. Cache _ytdlp_ssh_host() result in fetch_transcript (P2 style)
Was being called 2-3x per video; the function is cheap (env lookup
+ strip) so this is purely about readability.
Adds test_wrap_cmd_uses_option_terminator covering the security fix
explicitly with a `-oFoo=bar` host value. Updates index assertions in
the two existing tests that check command shape (host is now at index
4, command string at 5, with `--` at 3).
Adds LAST30DAYS_YT_SSH_HOST env var (or `~/.config/last30days/.env` key).
When set, yt-dlp YouTube search invocations are wrapped as
`ssh <host> "yt-dlp ..."` so they run on a residential-IP machine.
Motivation: when last30days runs on a datacenter VPS (Hetzner,
DigitalOcean, AWS, etc.), `ytsearch:` queries return 0 results because
YouTube's bot-wall fingerprints datacenter IP ranges before any cookie
check runs. Cookies alone don't fix this — the IP reputation is checked
first. Verified across yt-dlp stable 2026.03.17 and nightly builds.
The existing fallbacks (browser cookies, residential proxy services,
excluding YouTube) all have downsides: cookies expire, proxies cost
money, exclusion loses signal. Many users with a Mac mini, Pi, or
home server can host yt-dlp on their own residential IP — this just
needs an SSH alias and a one-line env var to wire it up.
Behaviour:
- Default (env var unset): identical to before, no shape change.
- Env var set: search command list is wrapped with `ssh -o BatchMode=yes
<host> "<shell-quoted yt-dlp invocation>"`. is_ytdlp_installed()
returns True without a local PATH check (the binary lives on the
remote host).
- Transcript path: when SSH-routing is on, skips the yt-dlp transcript
path (which writes a VTT file we couldn't easily read back over SSH)
and uses the existing _fetch_transcript_direct HTTP fallback. The
timedtext API isn't bot-walled, so this works fine on datacenter IPs.
Setup pitfall documented in the function docstring: on macOS hosts
with Homebrew, `eval "$(/opt/homebrew/bin/brew shellenv zsh)"` must
live in ~/.zshenv (not just ~/.zprofile) — non-login SSH shells don't
source .zprofile, so without this `ssh macmini "yt-dlp ..."` returns
"command not found" while interactive SSH works fine.
Tests: 10 new cases covering env var read, whitespace stripping,
empty-value handling, command wrapping passthrough/active modes,
shlex quoting, is_ytdlp_installed short-circuit, and end-to-end
search_youtube wrapping. Full test suite: 0 new failures (the 14
pre-existing failures in test_store, test_watchlist, test_setup_openclaw,
test_safari_cookies, test_version_consistency are unchanged on main).
Verified live: 0 results → 4 real hits for "claude code" search from a
Hetzner VPS routed through a Mac mini exit node on Tailscale.
The Gemini 3.1 Flash Lite preview model is being discontinued on
May 25, 2026. Per Google's GA announcement, the underlying model
architecture is identical and only the model identifier needs to
be updated from `gemini-3.1-flash-lite-preview` to
`gemini-3.1-flash-lite`.
Also relaxes the `_require_gemini_31_preview` guard to accept any
`gemini-3.1-*` identifier (renamed to `_require_gemini_31`), so the
GA name and the still-preview `gemini-3.1-pro-preview` both pass.
CI was running only test_plugin_contract.py and test_version_consistency.py
(2 of 84 test files), masking 13 rotted tests across 4 clusters. The suite is
fully offline-safe (1402 tests in ~7s without network), so the narrow scope
wasn't gating integration flakiness; it was just stale. validate.yml now runs
`uv run pytest` against the full suite.
Engine fix: store.findings_from_report is rerank-first. ranked_candidates is
the primary persistence path; hackernews/polymarket are unconditionally
supplemented from items_by_source because they rank poorly but matter for
watchlists. When ranked_candidates was empty (rerank failed or skipped),
reddit, x, and every other source were silently dropped. The supplement loop
now falls back to all sources only when ranked_candidates is empty; the normal
path is unchanged.
Test repairs:
- test_store.py (6) + test_watchlist_commands.py (2): cascade from the engine fix
- test_get_new_findings_filters_by_date (latent): local-time vs SQLite UTC
flake — switched to datetime.now(timezone.utc)
- TestPollDeviceAuth (3): mock_time.time side_effect lists too short after
impl added a last_reminder call — padded timeout test, pinned others to
return_value=0 (loops terminate via urlopen, not the clock)
- test_bare_run_emits_web_promo: engine reads ~/.config/last30days/.env, so
a contributor's saved EXA/PARALLEL key made grounding "available" and
suppressed the web promo. Also missing X made the "x" promo preempt "web".
Set LAST30DAYS_CONFIG_DIR="", subprocess cwd=tmpdir, XAI_API_KEY stub.
PR #382 introduced an `effective_retries` widening on the first gaierror,
but the widening leaked: every non-DNS error path (HTTPError, non-DNS
URLError, OSError) was gated on `effective_retries - 1` and so inherited
the expanded bound. A caller passing `retries=2` who hit DNS-then-non-DNS
got 3 attempts instead of 2 — contrary to the PR description and the
fail-fast intent of small retry budgets.
Fix:
- Gate every non-DNS sleep/retry decision on the caller's original
`retries`, not the widened `effective_retries`.
- Add an explicit `break` in each non-DNS branch when the original
budget is exhausted, so the (possibly widened) outer loop bound
can't pull us into an extra attempt.
Adds two regression tests covering the DNS-then-non-DNS-URLError and
DNS-then-OSError sequences flagged in Greptile review on PR #382.
Transient DNS resolution failures (socket.gaierror, surfaced as
urllib.error.URLError with reason=gaierror) were retried with the
generic URLError handler — linear backoff (2s, 4s, 6s) and bounded by
the caller-passed `retries` parameter. For callers that pass small
retry values (e.g. lib/reddit.py::_subreddit_search uses retries=2), a
single first-attempt DNS hiccup followed by one quick retry on the
still-flaky resolver would exhaust the retry budget and wipe a whole
subreddit sweep — which the caller's broad `except Exception` then
silent-empties as `[]`.
Fix:
- Distinguish URLError-with-gaierror-reason from generic URLError via
a new `_is_dns_failure()` helper.
- For DNS failures, use exponential backoff (1s, 2s, 4s, ...) instead
of the linear default.
- For DNS failures, expand the effective retry budget to at least
MIN_DNS_RETRIES (=3) on first occurrence, so callers that passed
`retries=2` still get a meaningful retry budget for the transient
case. Non-DNS URLErrors and HTTPErrors keep the caller's value.
- DNS attempts are counted separately (`dns_attempts`) so unrelated
URLError or OSError failures within the same call don't accidentally
expand the budget further.
Reported during a community-signal pass where the Reddit subreddit
sweep silently returned zero items after a first-round transient DNS
hiccup. The fix lives at the http layer (where the retry loop is)
rather than per-source so every caller benefits.
Tests:
- Verifies a caller-passed retries=2 still gets MIN_DNS_RETRIES=3
attempts on gaierror.
- Verifies gaierror-then-success returns successfully on attempt 2.
- Verifies the exponential-backoff sleep pattern (1s, 2s) on the
retry attempts before exhaustion.
- Verifies a non-DNS URLError (ConnectionRefusedError reason) does
NOT expand the retry budget — only true DNS failures do.
All 12 http tests pass (8 baseline + 4 new). No regressions in the
broader test suite (1373 pass / 14 fail, vs 1369 pass / 14 fail on
main — the 14 failures are pre-existing and unrelated to this PR).
Twitter's edge intermittently serves an HTML anti-bot interstitial in
place of JSON when the bird-search subprocess hits a per-query rate
limit. Before this fix, that response made json.loads raise
JSONDecodeError and _run_bird_search() returned {"error": ..., "items":
[]} with the parsed exception message — silent-empty against an
orchestrator that has no way to distinguish "Twitter served HTML; retry
likely succeeds" from "no tweets matched the query."
Surfaced during a community-signal pass where a Karpathy-LLM-wiki
subquery returned zero X items, while a second identical run a few
seconds later returned full results.
Fix:
- Extract the subprocess invocation into _invoke_bird_subprocess() so
the retry loop can call it multiple times cleanly. Returns
(result, terminal_error) — terminal_error is non-None for
unrecoverable cases (subprocess timeout, spawn failure) that should
NOT be retried.
- In _run_bird_search(), wrap the json.loads parse in a retry loop
bounded by MAX_JSON_DECODE_RETRIES (=2) with JSON_DECODE_RETRY_DELAY
(=5s) between attempts.
- On non-JSON stdout, log a diagnostic that names the shape
(`looks_html`, first-80-chars stdout preview, attempt counter) so
silent-empty failures become legible in logs.
- On retry exhaustion, return an error dict whose message explicitly
names "anti-bot interstitial" as the likely cause, distinguishing
this failure from a genuine no-results case.
Subprocess timeout, spawn failure, and non-zero return-code paths are
unchanged — those are terminal and don't retry.
Tests:
- Verifies HTML-then-JSON returns success on attempt 2.
- Verifies all-HTML returns the diagnostic error dict mentioning the
anti-bot interstitial cause.
- Verifies subprocess timeout is NOT retried.
All 12 bird_x tests pass (9 baseline + 3 new).
Two related drifts surfaced while reviewing PR #399 (EXCLUDE_SOURCES) —
docs claimed several SC-backed sources required INCLUDE_SOURCES opt-in
that the code didn't actually enforce, and threads was inconsistently
gated relative to its same-key siblings.
This commit picks the "code as source of truth + EXCLUDE_SOURCES as
suppression knob" model and aligns docs to match. It also promotes
threads to the same auto-on tier as tiktok and instagram, since all
three share the SC key and per-call cost shape — there was no real
product reason for threads being opt-in while the other two weren't.
The resulting source-gating model is three-tier and intentional:
• **Auto-on if backing infra present** (suppress via EXCLUDE_SOURCES):
reddit, HN, polymarket, X, youtube, github, bluesky, truthsocial,
grounding, **tiktok, instagram, threads**
• **INCLUDE_SOURCES persistent opt-in** (cost/billing reasons):
perplexity (different paid API — OpenRouter),
tiktok_comments / youtube_comments (N× extra SC calls per video)
• **--search per-query opt-in** (relevance reasons):
pinterest (visual pins, narrow utility),
xiaohongshu (Chinese-market specific)
Changes:
- env.py: `is_threads_available()` drops the INCLUDE_SOURCES check,
now mirrors tiktok/instagram (SC key → True). Docstring updated.
- tests/test_env_v3.py: new `ThreadsAvailabilityTests` class locks in
the new contract and includes a regression guard ("INCLUDE_SOURCES
should not be needed").
- SKILL.md: lines 333-338 rewritten so the model's "Build
ACTIVE_SOURCES_LIST" checklist reflects what the engine actually
runs. Drops false INCLUDE_SOURCES requirement for
tiktok/instagram/threads; corrects pinterest to mention --search;
adds missing INCLUDE_SOURCES=perplexity requirement.
- README: same alignment for the user-facing "Everything else in v3"
section.
Note on EXCLUDE_SOURCES references in the new docs: the suppression
flag is wired up in PR #399. SKILL.md and README mention EXCLUDE_SOURCES
as the opt-out path; that prose is forward-looking until #399 lands.
The behavior changes in this PR (threads auto-on) are self-contained
and don't require #399 to function — but for users who want to suppress
the newly-auto-on threads source, #399 needs to land first.
Two related fixes that surface when running last30days with multi-keyword
themed queries (e.g. "claude, personal agents, agentic infra"). Both bugs
caused entire sources to silently return zero items.
YouTube (ScrapeCreators)
SC's /v1/youtube/search rejects ?keyword= with HTTP 400:
{"error":"missing_parameter","message":"You must provide a query"}
The canonical SC parameter for that endpoint is `query`. Other SC
endpoints we use (Reddit, TikTok, Instagram) happened to work because
they use their own per-endpoint parameter names — YouTube was the lone
outlier.
Hacker News (Algolia)
Multi-keyword theme queries returned zero hits across every theme.
Algolia treats query= as strict AND across tokens, so a 4-5 word query
like "claude, personal agents, agentic infra" matches no stories.
Three changes in hackernews.py:
1. Hoist comma/hyphen flattening into _flatten_query_for_algolia() so
search_hackernews and _title_matches_query normalize the query the
same way — addresses Greptile P2 #2 about the two callsites needing
to stay in sync.
2. Pass `optionalWords` for all-but-the-first token so Algolia ranks
by token-overlap instead of requiring every token.
3. Relax _title_matches_query from all-words to any-word, *but match
on word boundaries (\b<word>\b) rather than naive substring* —
addresses Greptile P2 #1, which flagged that the previous any-word
relaxation would let "ai" falsely match "email" or "rail".
Token-overlap relevance scoring at parse time already demotes weak
matches, so word-boundary any-word matching is safe.
Tests: added coverage for no-token-in-title rejection, word-boundary vs
substring, and hyphen/comma flattening alignment between the search
parameter and the post-filter.
Co-authored-by: Trevin Chow <trevin@trevinchow.com>
The original PR added EXCLUDE_SOURCES filtering to pipeline.available_sources()
and to the check-config.sh banner, but env.py::get_config() builds its config
dict from a hardcoded keys list that didn't include EXCLUDE_SOURCES. The
result: setting EXCLUDE_SOURCES in the environment silently no-op'd through
the Python pipeline. Only the bash hook (which reads shell env directly)
worked. The PR's unit tests didn't catch this because they construct config
dicts directly, bypassing get_config().
Changes:
- Add ('EXCLUDE_SOURCES', '') to env.py's keys list so the env var actually
propagates into config.
- Add an end-to-end regression test that goes through get_config() rather
than constructing config dicts directly.
- Document EXCLUDE_SOURCES in SKILL.md's source-list checklist so the model
invoking the skill knows to subtract excluded sources before displaying
the active-sources line. (Per AGENTS.md: engine flags without SKILL.md
prose are incomplete — the agent invoking the skill won't know the flag
exists.)
Addresses Greptile review on PR #407:
- P1: setup-keychain.sh ALL_KEYS was missing GOOGLE_GENAI_API_KEY and
XIAOHONGSHU_API_BASE relative to _load_keychain's inline list, so users
manually storing those keys would not see them in --list and the
interactive prompt would never offer to set them.
Hoist the canonical key list into lib/env.py::KEYCHAIN_KEYS, have
get_config() pass it through, and add a parity test that parses
ALL_KEYS out of setup-keychain.sh and asserts equality. Drift is now
caught at CI time instead of after a user reports a missing key.
- P2: os.environ.get("USER", "") silently returned "" under sudo, in
Docker without --env USER, or in CI runners that strip USER. The
resulting `security find-generic-password -a ""` call would never
match items stored by setup-keychain.sh, so all lookups silently
returned nothing. Fall back to pwd.getpwuid(os.getuid()).pw_name when
USER is absent.
The P2 process-listing comment ("secret visible briefly via ps because
security has no stdin path for -w") has no clean fix — the README
already documents the manual `security add-generic-password` invocation
as an alternative for users with strict secret hygiene.
The same `^version:\s*"([^"]+)"\s*$` regex (or a slight variant) was
duplicated across three files: render.py inline, test_plugin_contract.py
local helper, test_version_consistency.py local helper. A future change
to the SKILL.md frontmatter version format would have needed to update
three places without any compile-time pressure to keep them in sync.
New skills/last30days/scripts/lib/skill_meta.py provides:
- `_VERSION_RE` private compiled pattern (accepts double-quoted,
single-quoted, or unquoted YAML version scalars per the widening
landed in 997708a)
- `read_skill_version(skill_md_path: Path) -> str | None` helper that
catches OSError + UnicodeDecodeError and returns None on miss
Callers updated:
- render.py::_skill_version now calls skill_meta.read_skill_version
inside the SKILL.md fallback loop, returning `read_skill_version(...) or "?"`.
Semantically equivalent to the old break-after-first-SKILL.md logic.
- test_plugin_contract.py and test_version_consistency.py import the
helper instead of defining the regex inline. Both files use the
established sys.path.insert pattern.
Added tests/test_skill_meta.py with 6 direct unit tests covering the
helper's full contract: missing file, undecodable bytes, no-version-line,
and all three quoting styles (double, single, unquoted). Previously the
helper was only exercised transitively through render._skill_version().
Added test_skill_md_uses_double_quoted_version to
test_version_consistency.py — the old per-test regex incidentally
asserted "this repo's SKILL.md uses double-quotes" by being strict;
the shared helper accepts all three styles, so the assertion is now
explicit instead of implicit.
Code-reviewed by ce-code-review (8 reviewers); safe_auto fixes applied
inline (rename to _VERSION_RE, group or-chain instead of generator,
docstring tightened, dropped unnecessary `from __future__ import
annotations`, tightened signature to Path-only).
Conftest.py refactor for the sys.path.insert duplication across ~20 test
files filed as issue #411 — out of scope for this PR (touches many
files, separate concern).
Test results: 23 passed in the affected test set (16 prior + 6 new
test_skill_meta tests + 1 new double-quote assertion). Full suite shows
same 13 pre-existing failures as main; zero new failures.
Adds the macOS Keychain as the lowest-priority credential source on Darwin.
Items stored as generic passwords with service name "last30days-<KEY>" for
the current user are picked up automatically by get_config() — file env
and process env still win on collision.
No new config knob: behavior is strictly additive. On non-Darwin (or when
the `security` binary is missing) the loader is a no-op, so Linux/Windows
behavior is unchanged.
Priority (highest wins):
1. Environment variables
2. .claude/last30days.env (per-project)
3. ~/.config/last30days/.env (global)
4. macOS Keychain items prefixed last30days- (new)
Includes:
- lib/env.py: KEYCHAIN_SERVICE_PREFIX constant, _load_keychain helper
(platform-gated, shutil.which-gated, subprocess-error tolerant),
wiring into get_config before get_openai_auth so OPENAI_API_KEY can
come from Keychain too, _CONFIG_SOURCE reports "keychain" when no
file source is present.
- scripts/setup-keychain.sh: bash helper with interactive set,
--list, --delete, --replace modes. Uses `security add-generic-password`.
- tests/test_env_keychain.py: 12 tests covering platform gate,
missing-binary gate, success path, whitespace stripping, subprocess
errors swallowed, get_config precedence, and an OPENAI_AUTH wiring
regression test.
- tests/test_env_cookies.py: existing integration test mocks the new
_load_keychain hook so it stays hermetic on Darwin developer
machines that have real keychain entries.
- README.md: new "macOS Keychain (optional)" subsection under
"Bring your own keys" documenting setup-keychain.sh and the manual
`security add-generic-password` invocation.
Tested on macOS with a populated keychain and against the existing pytest
suite — CI-tracked tests (test_plugin_contract.py, test_version_consistency.py)
plus all env-touching tests pass. Pre-existing unrelated failures in
test_store.py / test_watchlist_commands.py / test_setup_openclaw.py /
test_footer_nudge_suppression.py are untouched.
Every job sync.sh did has a better replacement:
- Per-harness skill dirs (~/.claude/skills, ~/.codex/skills, ~/.agents/skills):
`npx skills add . -g -y` writes to every detected harness's home dir and
uses symlinks by default. Edits propagate live — no re-deploy step.
- Hermes (~/.hermes/skills/research/last30days):
`hermes skills install mvanhorn/last30days-skill --force` pulls from
GitHub and handles the deploy itself. The script wrapping was redundant.
- OpenClaw variant: `clawhub install last30days-official` is what users
already run per the README; the maintainer doesn't need a separate
variant-deploy step in the public repo's scripts.
- Claude marketplace cache (~/.claude/plugins/cache/...): this was a
"test against the official install path" hack we shouldn't have been
recommending. With PR #400's resolver collapse, STEP 0 no longer
enforces the cache as the only valid SKILL.md location. Just install
the skill normally via `npx skills` or the marketplace.
Cleanup:
- DELETE skills/last30days/scripts/sync.sh
- tests/test_version_consistency.py — drop test_sync_cache_path_uses_skill_version
- CLAUDE.md — replace the sync.sh command + rule with `npx skills add . -g -y`
- HERMES_SETUP.md — Installation now uses `hermes skills install --force`;
developer-alternative section shows the symlink pattern for live editing
- render.py — _skill_version docstring no longer attributes the
".claude-plugin absent" case to sync.sh; explains it via per-harness
install paths in general
- .github/PULL_REQUEST_TEMPLATE.md — drop the "Ran bash scripts/sync.sh"
checklist item
CHANGELOG and historical docs (release notes, plan files) keep their
existing sync.sh mentions as accurate history.
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.
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.
Trevin Chow