diff --git a/AGENTS.md b/AGENTS.md index 6c5d0b9..fc728ea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,8 @@ Agent Skills package for researching any topic across Reddit, X, YouTube, and we - `skills/last30days/scripts/last30days.py` — main research engine - `skills/last30days/scripts/lib/` — search, enrichment, rendering modules - `skills/last30days/scripts/lib/vendor/bird-search/` — vendored X search client +- `docs/solutions/` — documented solutions to past problems (bugs, best practices, workflow patterns), organized by category with YAML frontmatter (`module`, `tags`, `problem_type`) +- `CONCEPTS.md` — shared domain vocabulary (Skill, Engine, Harness, Beta channel) — relevant when orienting to the codebase or discussing project terminology ## Orientation - This is an Agent Skills package, not a CLI tool. The product is the slash-command-invoked skill (`/last30days ` in most harnesses); `scripts/last30days.py` is implementation. Claude Code is the most common host but not the only one — features must work across every harness the skill installs into. diff --git a/CONCEPTS.md b/CONCEPTS.md new file mode 100644 index 0000000..2a4a462 --- /dev/null +++ b/CONCEPTS.md @@ -0,0 +1,23 @@ +# Concepts + +Shared vocabulary for `last30days-skill`. Terms here have a precise project-specific meaning — distinct enough from their general technical sense that a new contributor would need them defined to follow conversations, PR descriptions, or the SKILL.md contract. + +## The package + +### Skill + +A self-contained agent-instructions package consisting of a `SKILL.md` prose contract plus a sibling `scripts/` directory containing the executable code the SKILL.md invokes. The package conforms to the [Agent Skills](https://agentskills.io) open format and installs across every major harness (Claude Code, Codex, Cursor, GitHub Copilot, Gemini CLI, and 50+ others) via `npx skills add`, harness-native plugin installers, or per-harness skill directories. A Skill is the unit of distribution; the Skill is the product. + +### Engine + +The Python script (`scripts/last30days.py`) the Skill's SKILL.md invokes to do the actual research work. The Engine and SKILL.md have a contract: SKILL.md tells the model which flags to pass (`--plan`, `--competitors-plan`, `--x-handle`, `--subreddits`, `--emit=compact`, etc.), and the Engine produces a specific output shape (badge line, ranked evidence clusters, emoji-tree footer) that the model is contractually required to pass through. The Engine is implementation; the SKILL.md prose is the agent-facing surface. + +### Harness + +The agent runtime that loads Skills and invokes them on the user's behalf. Claude Code is the most common Harness for this Skill but not the only one — Codex, Cursor, GitHub Copilot, Gemini CLI, and the rest of the Agent Skills ecosystem also count. "Multi-harness" describes a Skill that works correctly across every Harness it installs into; features written without multi-harness awareness (e.g., engine flags with no SKILL.md integration, or paths hardcoded to one Harness's install layout) regress on Harnesses other than the one they were tested against. + +## Distribution + +### Beta channel + +A parallel install of the Skill, sourced from the private `mvanhorn/last30days-skill-private` repo and installed as `/last30days-beta` rather than `/last30days`. The Beta channel exists so experimental changes can be tested by real users before they ship to the public `/last30days`. Promotion from Beta to public happens via a review PR against this (public) repo — Beta-only changes never ship to public without that PR. The Beta channel workflow guide lives in `BETA.md` in the private repo. diff --git a/docs/plans/2026-04-22-002-feat-competitors-flag-comparison-fanout-plan.md b/docs/plans/2026-04-22-002-feat-competitors-flag-comparison-fanout-plan.md index a13a18c..9d95400 100644 --- a/docs/plans/2026-04-22-002-feat-competitors-flag-comparison-fanout-plan.md +++ b/docs/plans/2026-04-22-002-feat-competitors-flag-comparison-fanout-plan.md @@ -1,4 +1,7 @@ --- + +> **NOTE (added 2026-05-16):** This plan references `bash scripts/sync.sh`. That script was deleted in [PR #405](https://github.com/mvanhorn/last30days-skill/pull/405); the install workflow is now `npx skills add . -g -y` (symlinks the working tree across every detected harness). For context on why sync.sh went away, see [docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md](../solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md). The decisions captured in this plan remain accurate; only the deploy mechanism changed. + title: "feat: --competitors flag for auto-discovered comparison fan-out" type: feat status: active diff --git a/docs/plans/2026-04-22-003-fix-competitors-per-entity-resolution-plan.md b/docs/plans/2026-04-22-003-fix-competitors-per-entity-resolution-plan.md index c5924b7..34d367d 100644 --- a/docs/plans/2026-04-22-003-fix-competitors-per-entity-resolution-plan.md +++ b/docs/plans/2026-04-22-003-fix-competitors-per-entity-resolution-plan.md @@ -1,4 +1,7 @@ --- + +> **NOTE (added 2026-05-16):** This plan references `bash scripts/sync.sh`. That script was deleted in [PR #405](https://github.com/mvanhorn/last30days-skill/pull/405); the install workflow is now `npx skills add . -g -y` (symlinks the working tree across every detected harness). For context on why sync.sh went away, see [docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md](../solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md). The decisions captured in this plan remain accurate; only the deploy mechanism changed. + title: "fix: per-entity resolution, default-2, and stale-path guard for --competitors" type: fix status: active diff --git a/docs/plans/2026-04-22-005-feat-vs-mode-n-passes-competitors-auto-discovery-plan.md b/docs/plans/2026-04-22-005-feat-vs-mode-n-passes-competitors-auto-discovery-plan.md index 88b0479..732789e 100644 --- a/docs/plans/2026-04-22-005-feat-vs-mode-n-passes-competitors-auto-discovery-plan.md +++ b/docs/plans/2026-04-22-005-feat-vs-mode-n-passes-competitors-auto-discovery-plan.md @@ -1,4 +1,7 @@ --- + +> **NOTE (added 2026-05-16):** This plan references `bash scripts/sync.sh`. That script was deleted in [PR #405](https://github.com/mvanhorn/last30days-skill/pull/405); the install workflow is now `npx skills add . -g -y` (symlinks the working tree across every detected harness). For context on why sync.sh went away, see [docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md](../solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md). The decisions captured in this plan remain accurate; only the deploy mechanism changed. + title: "feat: vs mode runs N full passes and --competitors is vs with auto-discovery" type: feat status: active diff --git a/docs/plans/2026-04-22-006-fix-comparison-title-skill-attribution-plan.md b/docs/plans/2026-04-22-006-fix-comparison-title-skill-attribution-plan.md index e578c44..3ef5f60 100644 --- a/docs/plans/2026-04-22-006-fix-comparison-title-skill-attribution-plan.md +++ b/docs/plans/2026-04-22-006-fix-comparison-title-skill-attribution-plan.md @@ -1,4 +1,7 @@ --- + +> **NOTE (added 2026-05-16):** This plan references `bash scripts/sync.sh`. That script was deleted in [PR #405](https://github.com/mvanhorn/last30days-skill/pull/405); the install workflow is now `npx skills add . -g -y` (symlinks the working tree across every detected harness). For context on why sync.sh went away, see [docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md](../solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md). The decisions captured in this plan remain accurate; only the deploy mechanism changed. + title: "fix: comparison title says (/Last30Days) instead of (Last 30 Days)" type: fix status: active diff --git a/docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md b/docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md new file mode 100644 index 0000000..afe95fb --- /dev/null +++ b/docs/solutions/workflow-issues/release-consistency-test-cascade-2026-05-16.md @@ -0,0 +1,219 @@ +--- +title: Release-time consistency tests cause cascade CI failures across all open PRs +date: 2026-05-16 +category: docs/solutions/workflow-issues +module: ci-release-engineering +problem_type: workflow_issue +component: testing_framework +severity: high +applies_when: + - a test asserts consistency between two release-time artifacts (e.g., SKILL.md version and a hardcoded pin in a shell script) + - one artifact is updated as part of a version bump and the other requires a manual lockstep update + - multiple long-lived PRs are open simultaneously against the same base branch +symptoms: + - every open PR's CI fails after a version bump even though the PRs are unrelated to versioning + - the failing test references a stale hardcoded value that was not updated alongside the bumped version + - PR authors must rebase and manually fix an artifact they did not touch +root_cause: missing_workflow_step +resolution_type: code_fix +related_components: + - development_workflow + - documentation +tags: + - ci + - release-engineering + - consistency-test + - version-pin + - cascade-failure + - test-design + - workflow +--- + +# Release-time consistency tests cause cascade CI failures across all open PRs + +## Context + +A `tests/test_version_consistency.py::test_sync_cache_path_uses_skill_version` test was added to enforce that the version string embedded in `skills/last30days/scripts/sync.sh` (a hardcoded plugin-cache path segment) matched the version frontmatter in `skills/last30days/SKILL.md`. The intention was sound: the cache path had to stay in lockstep with the skill version or the sync would silently pull stale files. + +The test worked as designed until a release shipped. At that point it turned into a cascade-failure machine: + +1. A release PR bumps `SKILL.md` version (e.g., 3.2.0 → 3.2.1) **and** bumps the `sync.sh` pin. That PR's CI is green. +2. The release PR merges to `main`. +3. Every PR that was open at merge time was branched from pre-release `main`. Those PRs have `SKILL.md` 3.2.1 (inherited via merge-base with `main`) but their branch never touched `sync.sh`. +4. CI for those PRs runs the consistency test against the new `main` — `SKILL.md` says 3.2.1, `sync.sh` still says 3.2.0 — and fails. +5. All open PRs are now red simultaneously, with a failure that has nothing to do with their changes. + +This affected at least five PRs during the 2026-05-13 to 2026-05-15 window: PR #400 (caught during rebase, required a manual pin bump), PRs #390 and #392 (OpenClaw `SCRAPECREATORS_API_KEY` fix, both stalled for the same stale-pin reason), and at least two others. A follow-up hotfix PR (#397 — `fix(sync): bump cache target to 3.2.1 to match SKILL.md`) was required just to unblock the queue. + +The permanent fix was PR #405: delete `sync.sh` entirely (the install workflow made it redundant) and drop `test_sync_cache_path_uses_skill_version`. Once both were gone, no version-consistency cascade was possible. + +## Guidance + +### 1. Don't write consistency tests that read two files and assert one matches a substring derived from the other + +This pattern looks safe but is not: + +```python +def test_sync_cache_path_uses_skill_version(self) -> None: + sync_text = (SKILL_ROOT / "scripts" / "sync.sh").read_text(encoding="utf-8") + version = _skill_version() # reads SKILL.md + self.assertIn( + f'last30days-skill/last30days/{version}"', + sync_text, # asserts sync.sh contains that string + ) +``` + +It encodes the assumption that both files are always updated together, in the same commit, on the same branch. That assumption breaks the moment two files have independent lifecycle owners — a versioned manifest and a deployment script are archetypal examples. + +### 2. If the values genuinely need to stay in sync, derive one from the other at runtime + +Remove the hardcoded pin from `sync.sh` and compute it: + +```bash +# sync.sh — derive version from SKILL.md at runtime, no pin to maintain +SKILL_VERSION=$(grep -m1 '^version:' "$(dirname "$0")/../SKILL.md" \ + | sed 's/version:[[:space:]]*"\([^"]*\)"/\1/') +CACHE_PATH="last30days-skill/last30days/${SKILL_VERSION}" +``` + +Now there is only one source of truth (`SKILL.md`). The test that asserted they matched becomes vacuous and should be deleted. If `SKILL.md` is wrong, the sync itself will fail loudly — which is better feedback than a CI gate on a different PR. + +### 3. If two values must stay independent for legitimate reasons, update them together and make the test self-skip if either source is missing + +If separate versioning is genuinely required (e.g., SKILL.md versions for harness consumers, sync.sh versions a private artifact store with its own cadence), update both in the same PR — never staggered — and write the test to self-skip rather than error when either file is absent: + +```python +def test_sync_cache_path_uses_skill_version(self) -> None: + sync_sh = SKILL_ROOT / "scripts" / "sync.sh" + if not sync_sh.exists(): + self.skipTest("sync.sh not present; skipping pin consistency check") + sync_text = sync_sh.read_text(encoding="utf-8") + version = _skill_version() + self.assertIn( + f'last30days-skill/last30days/{version}"', + sync_text, + ) +``` + +Self-skipping means deleting the file is a non-event in CI — no cascading red, no hotfix PR to the queue. + +### 4. Run consistency tests against the merge-base diff, not main + +If you keep a two-file consistency test, scope it so it only fails when the PR itself modifies one of the two files but not the other. A GitHub Actions step can do this: + +```yaml +- name: Check sync.sh version pin consistency + run: | + BASE=$(git merge-base HEAD origin/main) + SKILL_CHANGED=$(git diff --name-only "$BASE" HEAD | grep -c 'SKILL\.md' || true) + SYNC_CHANGED=$(git diff --name-only "$BASE" HEAD | grep -c 'sync\.sh' || true) + if [ "$SKILL_CHANGED" -gt 0 ] && [ "$SYNC_CHANGED" -eq 0 ]; then + echo "SKILL.md version bumped but sync.sh pin was not updated" + exit 1 + fi +``` + +This only fires when your PR touched `SKILL.md` and left `sync.sh` alone — never because a release merged to `main` after you branched. + +### 5. Ask whether you actually need this test + +If the values are wrong, downstream tooling will fail loudly: the sync will fetch the wrong artifact, installs will break, or the harness will reject the version. A test that exists only to catch a human-bookkeeping error at release time adds cascade-fail risk without offering a meaningfully earlier signal. Weigh that cost before adding any two-file consistency gate. + +## Why This Matters + +The damage from a stale-pin consistency test is asymmetric. It: + +- Fails on every open PR simultaneously the moment a release lands on `main` — not just the PR that forgot to update the pin. +- Produces a failure message that points at a line in a test file with no obvious relationship to the PR's actual changes. +- Requires either a hotfix PR (touching a file the failing PRs have no business touching) or a manual rebase of every affected branch. +- Blocks work that has already been reviewed and approved. + +In this repo the effect was measurable: at least five PRs stalled across a two-day window, one hotfix PR was shipped just to unblock the queue, and multiple authors spent time debugging a failure completely unrelated to their changes. + +The broader principle is that tests which gate on *bookkeeping consistency between files* impose their maintenance cost on every contributor, every time, even when those contributors did nothing wrong. That cost compounds with team size and release cadence. + +## When to Apply + +Apply this guidance whenever you find yourself: + +- Writing a test that reads two files and asserts that a string in one matches a value derived from the other. +- Adding a CI step labeled "consistency check," "sync check," or "pin check" where the check compares a hardcoded value against a computed one from a separate file. +- Working in a repo where a versioned manifest (e.g., `SKILL.md`, `package.json`, `pyproject.toml`) and a deployment artifact (e.g., a shell script, a Dockerfile, a Helm values file) are both maintained by hand. +- Reviewing a PR that touches only one of two "paired" files and fails a consistency test for the other. + +It does *not* apply to tests that read a single source of truth and validate its internal structure (e.g., asserting that `SKILL.md`'s frontmatter version is double-quoted, or that `package.json`'s `version` field is a valid semver string). Those tests have one file and one assertion; they cannot cascade across branches. + +## Examples + +### Before — the pattern that caused the cascade + +Original `tests/test_version_consistency.py` (deleted in commit `9fb19ea`): + +```python +import re +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +SKILL_ROOT = ROOT / "skills" / "last30days" + + +def _skill_version() -> str: + text = (SKILL_ROOT / "SKILL.md").read_text(encoding="utf-8") + match = re.search(r'^version:\s*"([^"]+)"\s*$', text, re.MULTILINE) + if not match: + raise AssertionError("SKILL.md version frontmatter not found") + return match.group(1) + + +class TestVersionConsistency(unittest.TestCase): + def test_sync_cache_path_uses_skill_version(self) -> None: + sync_text = (SKILL_ROOT / "scripts" / "sync.sh").read_text(encoding="utf-8") + version = _skill_version() # source 1: SKILL.md frontmatter + self.assertIn( # assertion: sync.sh must contain + f'last30days-skill/last30days/{version}"', + sync_text, # source 2: hardcoded string in sync.sh + ) +``` + +`sync.sh` contained a line like: + +```bash +PLUGIN_CACHE="$HOME/.cache/last30days-skill/last30days/3.2.0" +``` + +When SKILL.md bumped to `3.2.1` in a release PR, `sync.sh` was updated in the same PR and CI stayed green. But every PR branched before that release still had `sync.sh` at `3.2.0`. Their CI failed immediately, with an assertion error pointing at the test, not at the release PR. + +### After — what we did: delete both + +PR #405 deleted `sync.sh` (the install workflow replaced it) and dropped `test_sync_cache_path_uses_skill_version` in the same change. No consistency gate, no pin to maintain, no cascade possible. + +### After — what we could have done instead: derive at runtime + +If `sync.sh` had still been needed, the right fix would have been to remove the hardcoded version from the script and derive it from `SKILL.md`: + +```bash +#!/usr/bin/env bash +# sync.sh — no hardcoded version; reads SKILL.md as single source of truth +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +SKILL_VERSION=$(grep -m1 '^version:' "${SCRIPT_DIR}/../SKILL.md" \ + | sed 's/version:[[:space:]]*"\([^"]*\)"/\1/') + +if [ -z "$SKILL_VERSION" ]; then + echo "error: could not parse version from SKILL.md" >&2 + exit 1 +fi + +PLUGIN_CACHE="$HOME/.cache/last30days-skill/last30days/${SKILL_VERSION}" +# ... rest of sync logic +``` + +With this in place, `test_sync_cache_path_uses_skill_version` has no reason to exist — there is nothing to assert. Delete it. If the version parsing breaks, `sync.sh` itself exits non-zero with a clear message. + +## Related + +- **PR #397** (merged) — `fix(sync): bump cache target to 3.2.1 to match SKILL.md`. The hotfix that unblocked the cascade temporarily by bumping the pin. +- **PR #400** (merged) — caught the same cascade during rebase; had to bump the pin to clear CI. +- **PR #390** (closed) and **PR #392** (rebased + merged) — OpenClaw `SCRAPECREATORS_API_KEY` fix; both blocked by the cascade until rebased onto post-#405 main. +- **PR #405** (merged) — the permanent fix: deleted `sync.sh` + `test_sync_cache_path_uses_skill_version` together. +- **PR #412** (merged) — adjacent work that consolidated SKILL.md version parsing into `lib/skill_meta.py`, reducing future drift risk by giving the version field one canonical reader.