When yt-dlp is installed but stale (or otherwise unable to fetch transcripts
for any returned videos), runs previously reported YouTube as fully
successful in two user-facing surfaces:
1. Footer (render.py): showed "N videos | M views" with no indication
that zero transcripts were captured. The "with transcripts" segment
was conditionally suppressed when the count was zero - converting
the canonical stale-binary failure mode into a silent absence at
the very surface users read for "did this work?".
2. Quality nudge (quality_nudge.py): classified YouTube as "active"
based purely on yt-dlp installation + absence of a top-level error.
Per-video transcript-fetch ratio was never inspected. A run that
returned N videos with 0 transcripts (canonical stale-binary
failure) was reported as fully active.
The engine itself logs the failure correctly at default stderr level
(`[YouTube] Got transcripts for 0/N videos (N failed)`), but that line
gets buried in 100+ lines of parallel-source progress output and is
contradicted by the success-shaped footer and nudge that follow.
This change makes both conclusion surfaces honest:
* render.py footer always renders "M/N with transcripts" so the ratio
is visible regardless of value. Zero is no longer hidden. Format is
M/N (not bare M) so the denominator is in the message and the user
does not have to cross-reference the "videos" count.
* quality_nudge.py adds a third tier between "active" and "missing":
"degraded". Triggered when yt-dlp is installed AND videos were
returned AND transcript-fetch ratio is below threshold (default 50%,
tunable via DEGRADED_TRANSCRIPT_THRESHOLD env var). Emits an
actionable nudge: "YouTube returned N videos but only M transcripts
captured. The most common cause is a stale yt-dlp binary - YouTube's
caption format changes frequently and old binaries silently fail
every transcript. Update via your package manager: scoop update
yt-dlp (Windows), brew upgrade yt-dlp (macOS), or pip install -U
yt-dlp."
* last30days.py populates youtube_videos_count and
youtube_transcripts_count in the research_results dict it passes to
compute_quality_score, enabling the new degraded check at the call
site.
Threshold rationale: 50% accommodates a few legitimate
caption-disabled videos in a multi-video result, but a stale-binary
run that fails every transcript trips the nudge cleanly.
Score impact: degradation is informational, not score-affecting.
YouTube still counts as "active" in score_pct so users do not see
their score drop for a fixable client-side issue. The nudge directs
them to their own package manager.
Tests:
* tests/test_quality_nudge.py: 6 new TestYouTubeDegraded cases cover
zero-transcripts-flags-degraded, partial-above-threshold-does-not-flag,
zero-videos-does-not-flag (no false positives on absence),
one-of-three-flags-degraded, threshold-tunable-via-config, and
degraded-does-not-affect-score.
* tests/test_render_v3.py: 4 new YoutubeFooterTranscriptRatioTests
cases cover zero-transcripts-with-videos-renders-zero-over-total
(the regression repro), partial-renders-ratio, full-renders-ratio,
and no-videos-suppresses-entire-segment.
All 29 new test cases verified GREEN with the fix and RED without it
(temp-reverted both files separately to confirm each test catches the
specific regression it asserts).
Integration validation: ran the engine against an intentionally stale
yt-dlp 2025.03.31 binary placed first on PATH. Pre-fix the footer
showed `YouTube: 3 videos | 386,815 views` (no transcript signal).
Post-fix the footer shows `YouTube: 3 videos | 386,815 views | 0/3
with transcripts` and stderr emits "Degraded: YouTube" plus the
actionable update-yt-dlp nudge.
Out of scope (deserves its own PR): exposing transcripts_captured in
the EVIDENCE FOR SYNTHESIS block so the synthesizing model can flag
degradation in prose. Larger schema-touching change.
render.py, ui.py, and last30days.py had hardcoded "v3.0.0" in titles
and headers while plugin.json was at 3.1.1. Use _skill_version()
(reads from plugin.json at runtime) so version strings stay in sync.
Fixes#284
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.
Web search backends (Brave, Exa, Serper) can return Reddit URLs as
results. Claude Code's WebFetch blocks reddit.com, so the model can't
retrieve full thread content. After web search, detect Reddit URLs
and fetch body text + top comments via reddit.com/.json endpoint
using the skill's own HTTP library.
Fixes#324
The skill advertises ScrapeCreators as offering "10,000 free API calls" in
six places. The actual free tier on the ScrapeCreators pricing page is
"100 credits free · No credit card required · Credits never expire" — a
100x overstatement that surprises users on signup.
Reporter (#367) burned through their full free allocation on a single
/last30days run after taking the 10,000-call claim at face value. They
verified the actual tier directly against scrapecreators.com plus an
independent review at fahimai.com.
Sweep:
- hooks/scripts/check-config.sh:110 (SessionStart hook tip line)
- README.md:228 (Sources × Cost table row)
- HERMES_SETUP.md:62 (Optional: ScrapeCreators bullet)
- skills/last30days/scripts/lib/ui.py:199 (PROMO_SINGLE_KEY["reddit"])
- skills/last30days/SKILL.md:1648 ("PAYG after 10,000 free API calls")
- skills/last30days/SKILL.md:1661 ("10,000 free API calls, then PAYG")
Wording defaults to the provider's own framing — "100 free credits" — and
keeps PAYG language where it was already explicit, since the paid step is
the part users were actually getting blindsided by.
CI gates: tests/test_plugin_contract.py (4) + tests/test_version_consistency.py (4)
all pass. shellcheck clean. No tests pin the "10,000" string.
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.
Greptile flagged that _cli_query's --since parsing uses datetime.now()
(local time) while first_seen is stored via SQLite's datetime('now') (UTC).
The same bug exists in three other call sites that compare against either
first_seen or run_date (both UTC):
- get_daily_cost: "today" defaults to local date, returns wrong day's cost
near UTC midnight
- get_stats: "7 days ago" cutoff for runs_7d / successful_7d
- get_trending: "N days ago" cutoff for finding activity ranking
- _cli_query: "N days ago" cutoff for --since flag (Greptile's flag)
All four now use datetime.now(timezone.utc). Same root cause and same fix
as the test_get_new_findings_filters_by_date repair in the previous commit.
Address Greptile review nit. DEFAULT_JUDGE_MODEL now reuses the
constant from lib/providers.py instead of duplicating the literal,
so a future identifier change only needs one edit.
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>
Greptile flagged that `entity_reports and args.emit == "html"` appeared in
two places — once when computing the footer display path, again when calling
save_output. The else-branches differ between the two callsites (the display
needs `report.topic` as fallback; the save call needs `None` so save_output
falls back to the report's own topic), so collapsing into one shared
expression would be wrong, but hoisting just the condition into a single
`is_comparison_html` bool eliminates the risk of drift while keeping the
two callsites' fallback semantics distinct.
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.
The Step 1 and comparison-mode resolver loops walked a hardcoded list
of install paths trying to find scripts/last30days.py. Two problems:
1. The list was never exhaustive — it covered ~/.codex/skills, but not
~/.claude/skills, ~/.cursor/skills, ~/.gemini/skills, ~/.copilot/skills,
~/.hermes/skills/research, etc. PR #406 was about to fix that by
enumerating more paths, but enumeration is the wrong shape.
2. The resolver could pick a different install than the SKILL.md the
model loaded from. Spec-vs-engine divergence is subtle and confusing
when it triggers.
The model already knows the SKILL.md path it loaded (from its Read tool
result). Templating that into the bash block is strictly better than
guessing across an enumerated list:
- Works for every harness without enumeration (Hermes, Cursor, anything
new) because we just use wherever the harness loaded SKILL.md from
- Aligns spec with engine — the engine runs from the same install the
spec was read from
- Deletes ~80 lines of bash across Step 1 + comparison-mode + the
prose preamble describing the resolver
Mechanics:
- SKILL_DIR placeholder in both bash blocks — model substitutes the
absolute path of the directory containing the SKILL.md it just Read
- One-line validation `[ ! -f "$SKILL_DIR/scripts/last30days.py" ]`
catches bad templating with a clear error
- All references to $SKILL_ROOT replaced with $SKILL_DIR (badge
VERSION lookup, prose description in the LAW-7 preamble area)
- STEP 0 unchanged — different concern (marketplaces stale-clone hop)
Version 3.2.3 -> 3.2.4 (behavior change: install paths the resolver
list never enumerated now work; install paths it did enumerate work
the same way they used to but via the SKILL_DIR template).
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.
Greptile's review flagged mktemp -t as non-portable between BSD and GNU.
The suggested replacement (mktemp "$TMPDIR/...XXXXXX.json") is correct
about dropping -t but still puts X's in the middle of the template name
(XXXXXX.json), which BSD mktemp does not substitute — only X's at the
end of the basename are replaced on BSD. Verified on macOS:
mktemp "$TMPDIR/last30days-test.XXXXXX.json"
→ /var/folders/.../last30days-test.XXXXXX.json (X's left literal)
The fully portable form uses trailing X's and drops the .json suffix
(engine reads by path, not extension):
mktemp "$TMPDIR/last30days-test.XXXXXX"
→ /var/folders/.../last30days-test.DXAHzR (X's substituted)
Verified on bash and zsh, BSD/macOS. GNU/Linux is already fine since
GNU substitutes X's wherever they appear in the basename.
Applied to both --competitors-plan (comparison-mode block) and --plan
(Step 1 block) tmpfile writes.
Closes#403.
The SKILL.md templates instructed the model to invoke the engine with
inline single-quoted JSON: `--plan '$JSON'` and `--competitors-plan '{...}'`.
When any resolved field value contained an apostrophe (common in `context`
strings like "McDonald's", "people's choice", or contracted forms like
"don't", "won't"), the inner `'` closed the outer single-quote and broke
shell parsing before the engine was even invoked.
Observed during PR #400 testing: a Codex run hit the trap and self-healed
by re-encoding, wasting one engine invocation and ~30s of latency.
Fix: switch both templates to the heredoc + tmpfile pattern. The engine's
`parse_plan()` and `parse_competitors_plan()` already check
`os.path.isfile(plan_str)` and read from disk — only the SKILL.md prose
needed to change.
The quoted heredoc marker (<<'PLAN_EOF') is load-bearing: it suppresses
shell interpolation so apostrophes, $, backticks, etc. pass through verbatim.
A trap on EXIT cleans up the tmpfile after the engine call returns.
LAW 7's "MUST contain --plan" self-check guidance and Step 1's invocation
example both updated to reference the file form. Comparison-mode invocation
block updated the same way for --competitors-plan.
Version bump 3.2.2 -> 3.2.3 because this is a behavior change users
running comparison-mode queries will notice (no more "shell quoting error,
retrying" sequences on apostrophe-containing context strings).
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.
zsh errors on globs that match nothing instead of returning the literal
pattern (bash's default), and `2>/dev/null` does not suppress the error
because it comes from the shell's glob expansion before `ls` even runs.
Under Codex (which executes the SKILL.md bash via zsh), STEP 0 and the
Step 1 / comparison-mode resolvers emitted noisy "no matches found"
errors on machines without a Claude plugin cache populated.
Replaces all three `ls -d $HOME/.claude/plugins/cache/last30days-skill/last30days/*/`
invocations with `find ... -mindepth 1 -maxdepth 1 -type d 2>/dev/null`.
find is POSIX-portable, errors silently when the base dir doesn't exist,
and never triggers shell glob errors. `sort -V | tail -1` precedence
preserved (verified: picks 3.10.0 over 3.2.1 over 3.1.0). Trailing-slash
strip removed because find doesn't append slashes.
Observed in Codex session running /last30days against PR #400 with the
Claude plugin cache deleted - bash output was:
zsh:1: no matches found: /Users/.../last30days/*/
After fix: clean empty output, exit 0, STEP 0 correctly treats it as
"no cache present, do not hop", resolver falls through to per-harness
skill dirs as designed.
Trevin Chow