diff --git a/src/fips_agents_cli/tools/patching.py b/src/fips_agents_cli/tools/patching.py index 7f69861..b4a9458 100644 --- a/src/fips_agents_cli/tools/patching.py +++ b/src/fips_agents_cli/tools/patching.py @@ -10,11 +10,21 @@ import click from rich.console import Console from rich.syntax import Syntax +from ruamel.yaml import YAML from fips_agents_cli.tools.git import clone_template console = Console() +# Filename a template repo can ship at its comparison root to declare its own +# patch categories, overriding the hardcoded MCP_/AGENT_ constants below. +TEMPLATE_MANIFEST_FILENAME = ".fips-template.yaml" + +# Schema version the CLI knows how to read. Manifests with a different value +# are treated as unsupported — the CLI falls back to the built-in constants +# and warns the user. +SUPPORTED_MANIFEST_SCHEMA_VERSION = 1 + # File categories for MCP server projects MCP_FILE_CATEGORIES = { "generators": { @@ -262,6 +272,117 @@ def _clone_template_for_patch(template_info: dict[str, Any], temp_path: Path) -> return template_root +def _load_template_manifest(template_root: Path) -> dict[str, Any] | None: + """Read ``.fips-template.yaml`` from the comparison root, if present. + + The manifest lets a template repo declare its own patch categories + rather than relying on the CLI's hardcoded MCP_/AGENT_ constants. + See issue #45 for the schema and rollout plan. + + Returns the parsed dict on success, or ``None`` when: + - the file does not exist (legacy template — fall back silently); + - the file is malformed YAML or not a mapping (warn, fall back); + - ``schema_version`` is missing or unsupported (warn, fall back). + """ + manifest_path = template_root / TEMPLATE_MANIFEST_FILENAME + if not manifest_path.exists(): + return None + + try: + yaml = YAML(typ="safe") + with open(manifest_path) as f: + data = yaml.load(f) + except Exception as e: + console.print( + f"[yellow]⚠[/yellow] Could not parse {TEMPLATE_MANIFEST_FILENAME} " + f"({e}); falling back to built-in patch categories." + ) + return None + + if not isinstance(data, dict): + console.print( + f"[yellow]⚠[/yellow] {TEMPLATE_MANIFEST_FILENAME} is not a mapping; " + "falling back to built-in patch categories." + ) + return None + + schema_version = data.get("schema_version") + if schema_version != SUPPORTED_MANIFEST_SCHEMA_VERSION: + console.print( + f"[yellow]⚠[/yellow] {TEMPLATE_MANIFEST_FILENAME} schema_version " + f"{schema_version!r} is not supported by this CLI " + f"(want {SUPPORTED_MANIFEST_SCHEMA_VERSION}); falling back to " + "built-in patch categories." + ) + return None + + return data + + +def _categories_from_manifest( + manifest: dict[str, Any], +) -> tuple[dict[str, dict[str, Any]], list[str]] | None: + """Translate a parsed manifest into the (categories, never_patch) shape + the rest of the patcher already understands. + + Returns ``None`` when the manifest is structurally wrong — caller should + warn and fall back to the constants. Recoverable defaults (missing + ``description``, missing ``ask_before_patch``) are filled in here. + """ + patch_block = manifest.get("patch") + if not isinstance(patch_block, dict): + return None + + raw_categories = patch_block.get("categories") + if not isinstance(raw_categories, dict): + return None + + raw_never_patch = patch_block.get("never_patch", []) + if not isinstance(raw_never_patch, list): + return None + + categories: dict[str, dict[str, Any]] = {} + for name, config in raw_categories.items(): + if not isinstance(name, str) or not isinstance(config, dict): + return None + patterns = config.get("patterns") + if not isinstance(patterns, list) or not all(isinstance(p, str) for p in patterns): + return None + categories[name] = { + "description": str(config.get("description", name)), + "patterns": list(patterns), + "ask_before_patch": bool(config.get("ask_before_patch", False)), + } + + never_patch = [str(p) for p in raw_never_patch] + return categories, never_patch + + +def _resolve_categories( + template_root: Path, template_info: dict[str, Any] +) -> tuple[dict[str, dict[str, Any]], list[str]]: + """Return ``(categories, never_patch)`` for the cloned template. + + Prefers the template's ``.fips-template.yaml`` manifest when present + and well-formed. Falls back to the hardcoded constants keyed by + ``template.type`` for any other case (file missing, malformed, + unsupported schema version). + """ + manifest = _load_template_manifest(template_root) + if manifest is not None: + resolved = _categories_from_manifest(manifest) + if resolved is not None: + return resolved + console.print( + f"[yellow]⚠[/yellow] {TEMPLATE_MANIFEST_FILENAME} present but " + "missing required fields; falling back to built-in patch " + "categories." + ) + + project_type = get_project_type(template_info) + return get_categories_for_type(project_type) + + def check_for_updates(project_path: Path, template_info: dict[str, Any]) -> dict[str, Any]: """ Check what files have changed in template since project creation. @@ -274,8 +395,6 @@ def check_for_updates(project_path: Path, template_info: dict[str, Any]) -> dict dict: Dictionary of categories with changed files """ template_url = template_info["template"]["url"] - project_type = get_project_type(template_info) - file_categories, _ = get_categories_for_type(project_type) console.print(f"[cyan]Fetching latest template from {template_url}...[/cyan]") @@ -283,6 +402,7 @@ def check_for_updates(project_path: Path, template_info: dict[str, Any]) -> dict with tempfile.TemporaryDirectory() as temp_dir: temp_path = Path(temp_dir) template_root = _clone_template_for_patch(template_info, temp_path) + file_categories, _ = _resolve_categories(template_root, template_info) updates = {} @@ -334,21 +454,23 @@ def patch_category( tuple: (success, message) """ project_type = get_project_type(template_info) - file_categories, never_patch = get_categories_for_type(project_type) + template_url = template_info["template"]["url"] - if category not in file_categories: - available = ", ".join(file_categories.keys()) or "(none)" + # Pre-clone fast-fail: if `category` is not in the project type's + # built-in category set, refuse before paying for a clone. The + # manifest can override patterns/never_patch for known categories + # but cannot introduce new category names — those would have no + # Click subcommand registered for them anyway. + builtin_categories, _ = get_categories_for_type(project_type) + if category not in builtin_categories: + available = ", ".join(builtin_categories.keys()) or "(none)" return ( False, f"Category '{category}' is not valid for {project_type} projects. " f"Available: {available}", ) - config = file_categories[category] - template_url = template_info["template"]["url"] - console.print(f"\n[bold cyan]Patching Category: {category}[/bold cyan]") - console.print(f"[dim]{config['description']}[/dim]\n") with tempfile.TemporaryDirectory() as temp_dir: temp_path = Path(temp_dir) @@ -356,6 +478,23 @@ def patch_category( template_root = _clone_template_for_patch(template_info, temp_path) console.print("[green]✓[/green] Template fetched\n") + # Resolve categories from manifest (when present) or hardcoded constants. + # Done post-clone so a template's .fips-template.yaml can override the + # patterns / ask_before_patch / never_patch for built-in categories. + file_categories, never_patch = _resolve_categories(template_root, template_info) + + if category not in file_categories: + # Manifest dropped a built-in category. Rare but possible. + available = ", ".join(file_categories.keys()) or "(none)" + return ( + False, + f"Category '{category}' is not declared by this template. " + f"Available: {available}", + ) + + config = file_categories[category] + console.print(f"[dim]{config['description']}[/dim]\n") + files_patched = 0 files_skipped = 0 diff --git a/tests/test_patch.py b/tests/test_patch.py index e8203c5..fbd736b 100644 --- a/tests/test_patch.py +++ b/tests/test_patch.py @@ -199,6 +199,205 @@ def test_walks_up_to_parent(self, tmp_path, monkeypatch): assert root == tmp_path +# --------------------------------------------------------------------------- +# Unit tests — .fips-template.yaml manifest loader (issue #45) +# --------------------------------------------------------------------------- + + +def _write_manifest(template_root: Path, body: str) -> Path: + """Drop a `.fips-template.yaml` file into a template root for testing.""" + manifest = template_root / ".fips-template.yaml" + manifest.write_text(body) + return manifest + + +class TestLoadTemplateManifest: + def test_returns_none_when_manifest_absent(self, tmp_path): + # Legacy template — no manifest, no warning, falls through silently + assert patching._load_template_manifest(tmp_path) is None + + def test_loads_well_formed_manifest(self, tmp_path): + _write_manifest( + tmp_path, + "schema_version: 1\npatch:\n categories: {}\n never_patch: []\n", + ) + manifest = patching._load_template_manifest(tmp_path) + assert manifest is not None + assert manifest["schema_version"] == 1 + + def test_returns_none_on_malformed_yaml(self, tmp_path, capsys): + _write_manifest(tmp_path, "this is: : not valid: yaml: at: all:\n - [bad") + assert patching._load_template_manifest(tmp_path) is None + # The user is told why we fell back — no silent surprise + captured = capsys.readouterr() + assert "Could not parse" in captured.out or "fall" in captured.out.lower() + + def test_returns_none_when_root_is_not_mapping(self, tmp_path, capsys): + _write_manifest(tmp_path, "- just a list\n- nothing useful\n") + assert patching._load_template_manifest(tmp_path) is None + + def test_returns_none_for_unsupported_schema_version(self, tmp_path, capsys): + _write_manifest(tmp_path, "schema_version: 99\npatch: {}\n") + assert patching._load_template_manifest(tmp_path) is None + captured = capsys.readouterr() + assert "schema_version" in captured.out + + def test_returns_none_when_schema_version_missing(self, tmp_path): + # Explicit contract — no schema_version means the file is not for us + _write_manifest(tmp_path, "patch:\n categories: {}\n") + assert patching._load_template_manifest(tmp_path) is None + + +class TestCategoriesFromManifest: + def test_extracts_categories_and_never_patch(self): + manifest = { + "schema_version": 1, + "patch": { + "categories": { + "chart": { + "description": "Helm chart templates", + "patterns": ["chart/templates/**/*"], + "ask_before_patch": True, + }, + "claude": { + "description": "Claude Code commands", + "patterns": [".claude/commands/**/*"], + "ask_before_patch": False, + }, + }, + "never_patch": ["src/agent.py", "agent.yaml"], + }, + } + result = patching._categories_from_manifest(manifest) + assert result is not None + cats, never = result + assert set(cats.keys()) == {"chart", "claude"} + assert cats["chart"]["patterns"] == ["chart/templates/**/*"] + assert cats["chart"]["ask_before_patch"] is True + assert cats["claude"]["ask_before_patch"] is False + assert never == ["src/agent.py", "agent.yaml"] + + def test_fills_in_default_description(self): + # description is optional; defaults to the category name + manifest = { + "patch": { + "categories": { + "build": {"patterns": ["Makefile"]}, + }, + "never_patch": [], + }, + } + result = patching._categories_from_manifest(manifest) + assert result is not None + cats, _ = result + assert cats["build"]["description"] == "build" + + def test_defaults_ask_before_patch_to_false(self): + manifest = { + "patch": { + "categories": { + "build": {"patterns": ["Makefile"]}, + }, + "never_patch": [], + }, + } + cats, _ = patching._categories_from_manifest(manifest) + assert cats["build"]["ask_before_patch"] is False + + def test_defaults_never_patch_to_empty(self): + manifest = {"patch": {"categories": {}}} + result = patching._categories_from_manifest(manifest) + assert result is not None + _, never = result + assert never == [] + + def test_returns_none_when_patch_block_missing(self): + assert patching._categories_from_manifest({"schema_version": 1}) is None + + def test_returns_none_when_categories_not_a_mapping(self): + manifest = {"patch": {"categories": ["chart", "build"]}} + assert patching._categories_from_manifest(manifest) is None + + def test_returns_none_when_patterns_missing(self): + manifest = { + "patch": { + "categories": {"build": {"description": "no patterns"}}, + }, + } + assert patching._categories_from_manifest(manifest) is None + + def test_returns_none_when_pattern_is_not_a_string(self): + manifest = { + "patch": { + "categories": {"build": {"patterns": ["Makefile", 42]}}, + }, + } + assert patching._categories_from_manifest(manifest) is None + + +class TestResolveCategories: + """`_resolve_categories` is the integration point: prefer the manifest + when valid, otherwise fall back to the constants keyed by project type. + """ + + def test_falls_back_to_constants_when_manifest_absent(self, tmp_path): + info = {"template": {"type": "agent"}} + cats, never = patching._resolve_categories(tmp_path, info) + assert cats is patching.AGENT_FILE_CATEGORIES + assert never is patching.AGENT_NEVER_PATCH + + def test_manifest_overrides_constants(self, tmp_path): + _write_manifest( + tmp_path, + """\ +schema_version: 1 +patch: + categories: + only-this: + description: A custom category + patterns: + - foo/bar.txt + ask_before_patch: true + never_patch: + - keep/me.yaml +""", + ) + info = {"template": {"type": "agent"}} + cats, never = patching._resolve_categories(tmp_path, info) + assert set(cats.keys()) == {"only-this"} + assert cats["only-this"]["description"] == "A custom category" + assert never == ["keep/me.yaml"] + + def test_falls_back_when_manifest_is_malformed(self, tmp_path, capsys): + # Manifest exists but is missing required fields + _write_manifest(tmp_path, "schema_version: 1\npatch:\n not_categories: 42\n") + info = {"template": {"type": "mcp-server"}} + cats, never = patching._resolve_categories(tmp_path, info) + assert cats is patching.MCP_FILE_CATEGORIES + assert never is patching.MCP_NEVER_PATCH + captured = capsys.readouterr() + assert "missing required fields" in captured.out + + def test_manifest_works_for_unsupported_project_type(self, tmp_path): + # `gateway` raises in get_categories_for_type, so before #45 it + # was unpatchable. With a manifest the template can opt in. + _write_manifest( + tmp_path, + """\ +schema_version: 1 +patch: + categories: + chart: + patterns: [chart/templates/**/*] + never_patch: [] +""", + ) + info = {"template": {"type": "gateway"}} + cats, never = patching._resolve_categories(tmp_path, info) + assert "chart" in cats + assert never == [] + + # --------------------------------------------------------------------------- # Unit tests — _clone_template_for_patch handles subdirs # --------------------------------------------------------------------------- @@ -402,3 +601,40 @@ def fake_clone(url, target_path, branch=None): # Must enumerate the available agent categories for cat in AGENT_FILE_CATEGORIES: assert cat in result.output + + def test_template_manifest_overrides_constants_in_check( + self, agent_project, monkeypatch, cli_runner + ): + # When a template ships .fips-template.yaml, those categories + # are what `patch check` reports — not the hardcoded constants. + def fake_clone(url, target_path, branch=None): + sub = target_path / "templates" / "agent-loop" + _make_fake_agent_template(sub) + # Drift a file the manifest's custom category covers + (sub / "Makefile").write_text("# UPDATED via manifest category\n") + (sub / ".fips-template.yaml").write_text("""\ +schema_version: 1 +patch: + categories: + template-managed: + description: Template-managed scaffolding files + patterns: + - Makefile + - Containerfile + ask_before_patch: true + never_patch: + - chart/values.yaml +""") + return "manifestcommit" + + monkeypatch.setattr(patching, "clone_template", fake_clone) + monkeypatch.chdir(agent_project) + + result = cli_runner.invoke(cli, ["patch", "check"]) + assert result.exit_code == 0, result.output + assert "Available Updates" in result.output + # Manifest's category name surfaces + assert "template-managed" in result.output + # Built-in agent categories that are NOT in the manifest must NOT show + assert "chart" not in result.output + assert "claude" not in result.output