From 6137cdba43fcb6ff9afde6fdb4c98079b52f3aa2 Mon Sep 17 00:00:00 2001 From: Safi Date: Tue, 2 Jun 2026 21:08:11 +0100 Subject: [PATCH] harden _always_on against missing blocks and pin skillgen baselines R1: the 6 always-on blocks were read into module-level constants at import, so a missing/corrupt always_on/*.md crashed `import graphify.__main__` and bricked every CLI command, not just install. Make _always_on lazy + lru_cached, raising a clear "reinstall" error only on the install path that needs the block; a module __getattr__ keeps the legacy constant names importable for the tests. Verified: deleting a block no longer crashes `graphify --version`. skillgen baselines: the self-check guards pinned to the moving `origin/v8` ref, which stops pointing at the pre-split state once the split lands on v8 (the always-on-roundtrip guard then fails in CI and monolith-roundtrip goes vacuous). Pin _v8_baseline_ref, ALWAYS_ON_BASELINE_REF, and the two monolith roundtrip_refs to the immutable pre-split commit SHA, matching the stated "does not track HEAD" intent; update the 4 tests that asserted the old ref string. R2: add tests/test_wheel_packaging.py - build the wheel and assert every references bundle and always-on block ships in it, so a package-data glob miss can't pass the repo-tree guards yet hard-exit `graphify install` for real users. Co-Authored-By: Claude Opus 4.8 --- graphify/__main__.py | 67 +++++++++++++++++++++++---------- tests/test_skillgen.py | 8 ++-- tests/test_wheel_packaging.py | 71 +++++++++++++++++++++++++++++++++++ tools/skillgen/gen.py | 16 ++++++-- tools/skillgen/platforms.toml | 4 +- 5 files changed, 136 insertions(+), 30 deletions(-) create mode 100644 tests/test_wheel_packaging.py diff --git a/graphify/__main__.py b/graphify/__main__.py index 1f2affa..0fb6c77 100644 --- a/graphify/__main__.py +++ b/graphify/__main__.py @@ -1,6 +1,7 @@ """graphify CLI - `graphify install` sets up the Claude Code skill.""" from __future__ import annotations +import functools import json import os import platform @@ -21,6 +22,7 @@ except Exception: _GRAPHIFY_OUT = os.environ.get("GRAPHIFY_OUT", "graphify-out") +@functools.lru_cache(maxsize=None) def _always_on(basename: str) -> str: """Read a packaged always-on instruction block from graphify/always_on/. @@ -32,7 +34,38 @@ def _always_on(basename: str) -> str: bytes here must match the former triple-quoted constant exactly — the always-on-roundtrip validator proves that. """ - return (Path(__file__).parent / "always_on" / f"{basename}.md").read_text(encoding="utf-8") + path = Path(__file__).parent / "always_on" / f"{basename}.md" + try: + return path.read_text(encoding="utf-8") + except OSError as exc: + # Defer to use-time so a missing/corrupt packaged block can't crash module + # import (which would brick every CLI command, not just install). Reached + # only by an install/integration path that actually needs this block. + raise RuntimeError( + f"graphify install is incomplete: missing always-on block '{basename}' " + f"at {path}. Reinstall graphifyy (e.g. `uv tool install --reinstall graphifyy`)." + ) from exc + + +_ALWAYS_ON_ALIASES = { + "_CLAUDE_MD_SECTION": "claude-md", + "_AGENTS_MD_SECTION": "agents-md", + "_GEMINI_MD_SECTION": "gemini-md", + "_VSCODE_INSTRUCTIONS_SECTION": "vscode-instructions", + "_ANTIGRAVITY_RULES": "antigravity-rules", + "_KIRO_STEERING": "kiro-steering", +} + + +def __getattr__(name: str) -> str: + # PEP 562: lazily resolve the legacy always-on section constants for external + # importers (e.g. the install-string tests). In-module code calls _always_on() + # directly; nothing is read at import time, so a missing block can no longer + # brick the CLI on `import graphify.__main__` (#1121 follow-up). + base = _ALWAYS_ON_ALIASES.get(name) + if base is not None: + return _always_on(base) + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") def _default_graph_path() -> str: @@ -587,17 +620,14 @@ def _print_install_usage() -> None: # generated by tools/skillgen and guarded by `skillgen --check`. Reading them at # load keeps the install-string / issue-#580 contract byte-for-byte while letting # a human edit one fragment instead of a triple-quoted literal here. -_CLAUDE_MD_SECTION = _always_on("claude-md") _CLAUDE_MD_MARKER = "## graphify" # AGENTS.md section for Codex, OpenCode, and OpenClaw. # All three platforms read AGENTS.md in the project root for persistent instructions. -_AGENTS_MD_SECTION = _always_on("agents-md") _AGENTS_MD_MARKER = "## graphify" -_GEMINI_MD_SECTION = _always_on("gemini-md") _GEMINI_MD_MARKER = "## graphify" @@ -630,10 +660,10 @@ def gemini_install(project_dir: Path | None = None, *, project: bool = False) -> if target.exists(): content = target.read_text(encoding="utf-8") new_content = _replace_or_append_section( - content, _GEMINI_MD_MARKER, _GEMINI_MD_SECTION + content, _GEMINI_MD_MARKER, _always_on("gemini-md") ) else: - new_content = _GEMINI_MD_SECTION + new_content = _always_on("gemini-md") if target.exists() and new_content == target.read_text(encoding="utf-8"): print(f"graphify already configured in {target.resolve()} (no change)") @@ -714,7 +744,6 @@ def gemini_uninstall(project_dir: Path | None = None, *, project: bool = False) _VSCODE_INSTRUCTIONS_MARKER = "## graphify" -_VSCODE_INSTRUCTIONS_SECTION = _always_on("vscode-instructions") def vscode_install(project_dir: Path | None = None) -> None: @@ -753,7 +782,7 @@ def vscode_install(project_dir: Path | None = None) -> None: if instructions.exists(): content = instructions.read_text(encoding="utf-8") new_content = _replace_or_append_section( - content, _VSCODE_INSTRUCTIONS_MARKER, _VSCODE_INSTRUCTIONS_SECTION + content, _VSCODE_INSTRUCTIONS_MARKER, _always_on("vscode-instructions") ) if new_content == content: print(f" {instructions} -> already configured (no change)") @@ -761,7 +790,7 @@ def vscode_install(project_dir: Path | None = None) -> None: instructions.write_text(new_content, encoding="utf-8") print(f" {instructions} -> graphify section {'updated' if _VSCODE_INSTRUCTIONS_MARKER in content else 'added'}") else: - instructions.write_text(_VSCODE_INSTRUCTIONS_SECTION, encoding="utf-8") + instructions.write_text(_always_on("vscode-instructions"), encoding="utf-8") print(f" {instructions} -> created") print() @@ -813,7 +842,6 @@ def vscode_uninstall(project_dir: Path | None = None) -> None: _ANTIGRAVITY_RULES_PATH = Path(".agents") / "rules" / "graphify.md" _ANTIGRAVITY_WORKFLOW_PATH = Path(".agents") / "workflows" / "graphify.md" -_ANTIGRAVITY_RULES = _always_on("antigravity-rules") _ANTIGRAVITY_WORKFLOW = """\ --- @@ -829,7 +857,6 @@ If no path argument is given, use `.` (current directory). """ -_KIRO_STEERING = _always_on("kiro-steering") _KIRO_STEERING_MARKER = "graphify: A knowledge graph of this project" @@ -849,13 +876,13 @@ def _kiro_install(project_dir: Path) -> None: steering_dir = project_dir / ".kiro" / "steering" steering_dir.mkdir(parents=True, exist_ok=True) steering_dst = steering_dir / "graphify.md" - if steering_dst.exists() and steering_dst.read_text(encoding="utf-8") == _KIRO_STEERING: + if steering_dst.exists() and steering_dst.read_text(encoding="utf-8") == _always_on("kiro-steering"): print(f" .kiro/steering/graphify.md -> already configured (no change)") else: # File is wholly graphify-owned. Overwrite on upgrade so older # report-first wording does not silently linger (issue #580). action = "updated" if steering_dst.exists() else "written" - steering_dst.write_text(_KIRO_STEERING, encoding="utf-8") + steering_dst.write_text(_always_on("kiro-steering"), encoding="utf-8") print(f" .kiro/steering/graphify.md -> always-on steering {action}") print() @@ -904,13 +931,13 @@ def _antigravity_install(project_dir: Path) -> None: rules_path.parent.mkdir(parents=True, exist_ok=True) if rules_path.exists(): existing = rules_path.read_text(encoding="utf-8") - if _ANTIGRAVITY_RULES.strip() != existing.strip(): - rules_path.write_text(_ANTIGRAVITY_RULES, encoding="utf-8") + if _always_on("antigravity-rules").strip() != existing.strip(): + rules_path.write_text(_always_on("antigravity-rules"), encoding="utf-8") print(f"graphify rule updated at {rules_path.resolve()}") else: print(f"graphify rule already configured at {rules_path.resolve()} (no change)") else: - rules_path.write_text(_ANTIGRAVITY_RULES, encoding="utf-8") + rules_path.write_text(_always_on("antigravity-rules"), encoding="utf-8") print(f"graphify rule written to {rules_path.resolve()}") # 3. Write .agents/workflows/graphify.md @@ -1406,10 +1433,10 @@ def _agents_install(project_dir: Path, platform: str) -> None: if target.exists(): content = target.read_text(encoding="utf-8") new_content = _replace_or_append_section( - content, _AGENTS_MD_MARKER, _AGENTS_MD_SECTION + content, _AGENTS_MD_MARKER, _always_on("agents-md") ) else: - new_content = _AGENTS_MD_SECTION + new_content = _always_on("agents-md") if target.exists() and new_content == target.read_text(encoding="utf-8"): print(f"graphify already configured in {target.resolve()} (no change)") @@ -1636,10 +1663,10 @@ def claude_install(project_dir: Path | None = None) -> None: if target.exists(): content = target.read_text(encoding="utf-8") new_content = _replace_or_append_section( - content, _CLAUDE_MD_MARKER, _CLAUDE_MD_SECTION + content, _CLAUDE_MD_MARKER, _always_on("claude-md") ) else: - new_content = _CLAUDE_MD_SECTION + new_content = _always_on("claude-md") if target.exists() and new_content == target.read_text(encoding="utf-8"): print(f"graphify already configured in {target.resolve()} (no change)") diff --git a/tests/test_skillgen.py b/tests/test_skillgen.py index fcac56d..4749480 100644 --- a/tests/test_skillgen.py +++ b/tests/test_skillgen.py @@ -593,9 +593,9 @@ def test_audit_reads_each_host_against_its_own_v8_body(): This is the structural fix: a per-host body, so a drop on one host surfaces. """ - assert gen._v8_baseline_ref("claude") == "origin/v8:graphify/skill.md" - assert gen._v8_baseline_ref("trae") == "origin/v8:graphify/skill-trae.md" - assert gen._v8_baseline_ref("vscode") == "origin/v8:graphify/skill-vscode.md" + assert gen._v8_baseline_ref("claude") == "47042beb05d1f6dd2186c0c499ae2840ce604ead:graphify/skill.md" + assert gen._v8_baseline_ref("trae") == "47042beb05d1f6dd2186c0c499ae2840ce604ead:graphify/skill-trae.md" + assert gen._v8_baseline_ref("vscode") == "47042beb05d1f6dd2186c0c499ae2840ce604ead:graphify/skill-vscode.md" def test_audit_catches_an_induced_per_host_drop(): @@ -783,6 +783,6 @@ def test_amp_audit_coverage_passes_against_its_own_v8(): confirms every heading single-homes in amp's core + references. """ platforms = gen.load_platforms() - assert gen._v8_baseline_ref("amp") == "origin/v8:graphify/skill-amp.md" + assert gen._v8_baseline_ref("amp") == "47042beb05d1f6dd2186c0c499ae2840ce604ead:graphify/skill-amp.md" problems = gen.audit_coverage(platforms["amp"]) assert problems == [], "\n".join(problems) diff --git a/tests/test_wheel_packaging.py b/tests/test_wheel_packaging.py new file mode 100644 index 0000000..182925c --- /dev/null +++ b/tests/test_wheel_packaging.py @@ -0,0 +1,71 @@ +"""Packaging guard (#1121 follow-up): the 5 skillgen guards check the *repo tree*, +not the *built wheel*. A host whose references bundle or always-on block fails to +match the `package-data` globs would pass `--check`/`--audit-coverage` yet make +`graphify install` hard-exit with "not found in package" for real users. + +This builds the wheel once and asserts every committed skill artifact ships in it. +""" +from __future__ import annotations + +import subprocess +import sys +import zipfile +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[1] +PKG = REPO / "graphify" + + +def _has_build() -> bool: + try: + subprocess.run( + [sys.executable, "-m", "build", "--version"], + check=True, capture_output=True, + ) + return True + except (subprocess.CalledProcessError, FileNotFoundError): + return False + + +def _expected_artifacts() -> list[Path]: + """Every committed references/*.md (per host) + always_on/*.md block.""" + refs = sorted((PKG / "skills").glob("*/references/*.md")) + always = sorted((PKG / "always_on").glob("*.md")) + # Sanity: if these are empty the test wiring is broken, not the wheel. + assert refs, "no skills/*/references/*.md found in repo — packaging test mis-wired" + assert always, "no always_on/*.md found in repo — packaging test mis-wired" + return refs + always + + +@pytest.fixture(scope="module") +def wheel_namelist(tmp_path_factory) -> set[str]: + if not _has_build(): + pytest.skip("`python -m build` unavailable (dev extra not installed)") + out = tmp_path_factory.mktemp("wheel") + proc = subprocess.run( + [sys.executable, "-m", "build", "--wheel", "--no-isolation", + "--outdir", str(out), str(REPO)], + capture_output=True, text=True, + ) + if proc.returncode != 0: + pytest.skip(f"wheel build failed in this env:\n{proc.stderr[-800:]}") + wheels = list(out.glob("graphifyy-*.whl")) + assert wheels, "no wheel produced" + with zipfile.ZipFile(max(wheels, key=lambda p: p.stat().st_mtime)) as z: + return set(z.namelist()) + + +@pytest.mark.parametrize( + "artifact", + _expected_artifacts(), + ids=lambda p: str(p.relative_to(PKG)), +) +def test_skill_artifact_ships_in_wheel(artifact: Path, wheel_namelist: set[str]) -> None: + rel = "graphify/" + artifact.relative_to(PKG).as_posix() + assert rel in wheel_namelist, ( + f"{rel} is committed in the repo but NOT in the built wheel — " + f"`graphify install` would hard-exit for this host. Check the " + f"[tool.setuptools.package-data] globs in pyproject.toml." + ) diff --git a/tools/skillgen/gen.py b/tools/skillgen/gen.py index b473d67..b91505c 100644 --- a/tools/skillgen/gen.py +++ b/tools/skillgen/gen.py @@ -45,11 +45,19 @@ PLATFORMS_TOML = SKILLGEN_DIR / "platforms.toml" # against ITS OWN v8 body is the per-host guard: a drop that only hits one host # (e.g. trae losing its AGENTS.md integration section) is invisible when every # host is checked against claude's monolith, so the audit must be per-host. +# +# Baselines are pinned to the immutable pre-split commit SHA, NOT the moving +# `origin/v8` ref: once the split lands on v8, `origin/v8` no longer holds the +# original monolith bodies / inline constants, so a symbolic ref would compare +# the split against itself (vacuous) or fail to find the old constants. The SHA +# is an ancestor of origin/v8 and is fetched under the CI `fetch-depth: 0`. +_V8_BASELINE_SHA = "47042beb05d1f6dd2186c0c499ae2840ce604ead" + def _v8_baseline_ref(platform_key: str) -> str: - """The git ref for a split host's own v8 skill body.""" + """The git ref for a split host's own pre-split skill body.""" if platform_key == "claude": - return "origin/v8:graphify/skill.md" - return f"origin/v8:graphify/skill-{platform_key}.md" + return f"{_V8_BASELINE_SHA}:graphify/skill.md" + return f"{_V8_BASELINE_SHA}:graphify/skill-{platform_key}.md" # Immutable baseline for --always-on-roundtrip. The six always-on instruction # blocks used to be triple-quoted constants in graphify/__main__.py; they are now @@ -59,7 +67,7 @@ def _v8_baseline_ref(platform_key: str) -> str: # constant byte for byte. It deliberately does NOT track HEAD: once the extraction # lands, HEAD's constants are _always_on(...) calls, not the literals the # validator needs to compare against. -ALWAYS_ON_BASELINE_REF = "origin/v8:graphify/__main__.py" +ALWAYS_ON_BASELINE_REF = f"{_V8_BASELINE_SHA}:graphify/__main__.py" # The always-on instruction blocks: rendered-file basename -> the __main__.py # constant it must reproduce. Rendered to graphify/always_on/.md from diff --git a/tools/skillgen/platforms.toml b/tools/skillgen/platforms.toml index 72842de..bc1ccbd 100644 --- a/tools/skillgen/platforms.toml +++ b/tools/skillgen/platforms.toml @@ -193,10 +193,10 @@ extraction = "verbose" bucket = "monolith" skill_dst = "graphify/skill-aider.md" monolith = "aider" -roundtrip_ref = "origin/v8:graphify/skill-aider.md" +roundtrip_ref = "47042beb05d1f6dd2186c0c499ae2840ce604ead:graphify/skill-aider.md" [platform.devin] bucket = "monolith" skill_dst = "graphify/skill-devin.md" monolith = "devin" -roundtrip_ref = "origin/v8:graphify/skill-devin.md" +roundtrip_ref = "47042beb05d1f6dd2186c0c499ae2840ce604ead:graphify/skill-devin.md"