From aa60dfc4cf1eed67dd4dfcfb23ba6bdb66bcf5f0 Mon Sep 17 00:00:00 2001 From: Jorge Castro Date: Thu, 6 Aug 2026 20:36:53 -0400 Subject: [PATCH] fix(ci): stop skill-index staleness check from failing on date drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check-skill-catalog pre-commit hook (and validate.yml's required 'validate' check) compares scripts/generate_skill_index.py --check output byte-for-byte against the committed docs/skills/index.json and index.md. The catalog's generated_at field was always stamped with date.today() at build time, so the check failed whenever CI ran on a later calendar day than the last regeneration on main — regardless of whether any skill doc actually changed. This produced false-negative 'stale index' failures on 9 of 19 open PRs, none of which touched docs/skills content, training reviewers to ignore a required status check. Fix: only advance generated_at when the actual catalog content (schema_version + skills) differs from what's committed. --write becomes idempotent when nothing changed (no gratuitous date bump), and --check now tolerates a stale generated_at as long as the underlying skill data matches. A genuine drift (a skill doc edited without regenerating the index) still fails, since skills content itself differs — the real protection is preserved. Rejected alternatives: - Diffing against the PR merge result instead of PR head: doesn't fix the root cause, since the merge tree's freshly regenerated catalog would still get stamped with 'today' and be compared against a committed file stamped on a prior day. - Scoping the check to changed files: would blind it to a PR that edits a skill doc without regenerating the index, weakening the real protection this check exists for. - Dropping the committed generated file: much larger blast radius (index.md is referenced from docs), not needed once the byte-for-byte date comparison is fixed. - Auto-commit bot step: adds CI complexity for a problem solvable with a three-line comparison fix. Assisted-by: Claude Sonnet 5 via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/generate_skill_index.py | 29 +++++++++++++++++ tests/test_skill_docs.py | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/scripts/generate_skill_index.py b/scripts/generate_skill_index.py index e63501a4..69df952c 100755 --- a/scripts/generate_skill_index.py +++ b/scripts/generate_skill_index.py @@ -94,6 +94,34 @@ def build_catalog() -> dict: } +def load_existing_catalog() -> dict | None: + """Return the currently committed catalog, or None if it doesn't exist/parse.""" + if not INDEX_PATH.exists(): + return None + try: + return json.loads(INDEX_PATH.read_text()) + except json.JSONDecodeError: + return None + + +def pin_unchanged_generated_at(catalog: dict, existing: dict | None) -> None: + """Reuse the committed `generated_at` when the actual catalog content + (schema_version + skills) hasn't changed. + + `generated_at` is a "last regenerated" timestamp, not build-time metadata: + it should only advance when a skill doc actually changes. Without this, + every run stamps today's date, which makes `--check` fail on pure + calendar drift (e.g. a PR branch cut before today, or CI running a day + after the last regeneration) even though no skill content is stale. + """ + if ( + existing is not None + and existing.get("schema_version") == catalog["schema_version"] + and existing.get("skills") == catalog["skills"] + ): + catalog["generated_at"] = existing.get("generated_at", catalog["generated_at"]) + + def validate_catalog(catalog: dict) -> None: schema = json.loads(SCHEMA_PATH.read_text()) validator = Draft202012Validator(schema) @@ -137,6 +165,7 @@ def main() -> int: try: catalog = build_catalog() + pin_unchanged_generated_at(catalog, load_existing_catalog()) validate_catalog(catalog) except ValueError as e: print(f"error: {e}", file=sys.stderr) diff --git a/tests/test_skill_docs.py b/tests/test_skill_docs.py index ed3d4aad..b1cfcee2 100644 --- a/tests/test_skill_docs.py +++ b/tests/test_skill_docs.py @@ -251,6 +251,62 @@ def test_generate_skill_index_round_trip(tmp_path: Path, monkeypatch: pytest.Mon assert mod.main() == 0 +def test_generate_skill_index_check_tolerates_stale_generated_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A `--check` run must not fail purely because `generated_at` no longer + matches today's date. Real-world PR CI runs on a different calendar day + than the last regeneration on main, and that alone is not staleness. + """ + repo_root = make_skill_tree(tmp_path) + mod = load_generate_skill_index() + patch_skill_index_paths(mod, repo_root) + + monkeypatch.setattr(sys, "argv", ["generate_skill_index.py", "--write"]) + assert mod.main() == 0 + + index_path = repo_root / "docs" / "skills" / "index.json" + md_path = repo_root / "docs" / "skills" / "index.md" + + # Simulate calendar drift: back-date the committed files without + # touching any skill content. + data = json.loads(index_path.read_text()) + data["generated_at"] = "2020-01-01" + index_path.write_text(json.dumps(data, indent=2) + "\n") + md_text = md_path.read_text().replace( + f"Generated: {mod.date.today().isoformat()}", "Generated: 2020-01-01" + ) + md_path.write_text(md_text) + + monkeypatch.setattr(sys, "argv", ["generate_skill_index.py", "--check"]) + assert mod.main() == 0 + + +def test_generate_skill_index_check_still_fails_on_real_drift( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """A genuine content change without regeneration must still fail + `--check` — the date-drift tolerance must not weaken this protection. + """ + repo_root = make_skill_tree(tmp_path) + mod = load_generate_skill_index() + patch_skill_index_paths(mod, repo_root) + + monkeypatch.setattr(sys, "argv", ["generate_skill_index.py", "--write"]) + assert mod.main() == 0 + + # Modify a skill doc's front matter without regenerating the index. + zeta_path = repo_root / "docs" / "skills" / "zeta.md" + zeta_path.write_text( + zeta_path.read_text().replace("Zeta purpose", "Totally different purpose") + ) + + monkeypatch.setattr(sys, "argv", ["generate_skill_index.py", "--check"]) + assert mod.main() == 1 + captured = capsys.readouterr() + assert "index.json is stale" in captured.err + + def test_generate_skill_index_rejects_missing_entry_point( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: