diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 863b6ef7dc..6178c4740e 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2574,6 +2574,39 @@ def _get_manifest(self, pack_dir: Path) -> Optional["PresetManifest"]: self._manifest_cache[key] = None return self._manifest_cache[key] + def _manifest_declared_template( + self, pack_dir: Path, template_name: str, template_type: str + ) -> tuple[dict | None, Path | None]: + """Resolve a preset's manifest-declared template entry and usable file. + + Returns ``(entry, candidate)``: + - ``entry`` is the matching ``provides.templates`` mapping, or ``None`` if + the manifest is absent or does not list this ``(name, type)``. + - ``candidate`` is the declared ``file:`` resolved under ``pack_dir`` IFF + it is a regular file (``is_file()``); ``None`` otherwise — a missing, + empty, or non-file (e.g. directory) declaration yields ``(entry, None)``. + + The manifest is authoritative: when it declares a template (``entry`` is + not ``None``) but the file is unusable (``candidate`` is ``None``), + callers must NOT fall back to the convention lookup — that would mask a + typo or pick up an undeclared file. Shared by ``resolve()`` and + ``collect_all_layers()`` so their manifest-first resolution cannot + silently diverge again (the divergence this fix addressed). + """ + manifest = self._get_manifest(pack_dir) + if not manifest: + return None, None + for tmpl in manifest.templates: + if tmpl.get("name") == template_name and tmpl.get("type") == template_type: + file_path = tmpl.get("file") + if file_path: + manifest_candidate = pack_dir / file_path + return tmpl, ( + manifest_candidate if manifest_candidate.is_file() else None + ) + return tmpl, None + return None, None + def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: """Build unified list of registered and unregistered extensions sorted by priority. @@ -2676,6 +2709,27 @@ def resolve( registry = PresetRegistry(self.presets_dir) for pack_id, _metadata in registry.list_by_priority(): pack_dir = self.presets_dir / pack_id + # The preset manifest is authoritative: if it declares this + # template with an explicit ``file:``, resolve to that path — + # and do NOT fall back to convention when it's missing, to + # avoid masking typos or picking up an undeclared file. Only + # when the manifest is absent or doesn't list this template do + # we use the convention-based subdir lookup. Mirrors + # collect_all_layers()/resolve_content() so resolve() and + # resolve_with_source() agree with them instead of returning + # the core template (or a stray convention file). + entry, manifest_candidate = self._manifest_declared_template( + pack_dir, template_name, template_type + ) + if manifest_candidate is not None: + return manifest_candidate + if entry is not None: + # Manifest declares this template but the file is missing, + # non-file (e.g. a directory), or an empty/falsey ``file`` + # value. The manifest is authoritative, so skip this pack's + # convention fallback rather than mask a typo — mirrors + # collect_all_layers(). + continue for subdir in subdirs: if subdir: candidate = pack_dir / subdir / f"{template_name}{ext}" @@ -2943,31 +2997,22 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: pack_dir = self.presets_dir / pack_id # Read strategy and manifest file path from preset manifest strategy = "replace" - manifest_file_path = None manifest_has_strategy = False - manifest_found_entry = False - manifest = self._get_manifest(pack_dir) - if manifest: - for tmpl in manifest.templates: - if (tmpl.get("name") == template_name - and tmpl.get("type") == template_type): - strategy = tmpl.get("strategy", "replace") - manifest_has_strategy = "strategy" in tmpl - manifest_file_path = tmpl.get("file") - manifest_found_entry = True - break - # Use manifest file path if specified, otherwise convention-based - # lookup — but only when the manifest doesn't exist or doesn't - # list this template, so preset.yml stays authoritative. + entry, manifest_candidate = self._manifest_declared_template( + pack_dir, template_name, template_type + ) + if entry is not None: + strategy = entry.get("strategy", "replace") + manifest_has_strategy = "strategy" in entry + # Use the manifest's declared file when it's a usable regular file; + # only fall back to convention-based lookup when the manifest + # doesn't list this template at all, so preset.yml stays + # authoritative (a declared-but-unusable file skips convention — + # parity with resolve()). candidate = None - if manifest_file_path: - manifest_candidate = pack_dir / manifest_file_path - if manifest_candidate.exists(): - candidate = manifest_candidate - # Explicit file path that doesn't exist: skip convention - # fallback to avoid masking typos or picking up unintended files. - elif not manifest_found_entry: - # Manifest doesn't list this template — check convention paths + if manifest_candidate is not None: + candidate = manifest_candidate + elif entry is None: candidate = _find_in_subdirs(pack_dir) if candidate: # Legacy fallback: if manifest doesn't explicitly declare a diff --git a/tests/test_presets.py b/tests/test_presets.py index 54cb9dd5b3..19f726b3d1 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -884,6 +884,186 @@ def test_resolve_pack_takes_priority_over_core(self, project_dir, pack_dir): assert result is not None assert "Custom Spec Template" in result.read_text() + def _install_pack_with_manifest_file(self, project_dir, *, extra_file=False): + """Create a pack whose manifest declares a NON-convention file: path. + + Returns the pack dir under the project. The declared file lives at + custom/spec.md (not the convention templates/spec-template.md). + """ + presets_dir = project_dir / ".specify" / "presets" + pack_dir = presets_dir / "mypack" + (pack_dir / "custom").mkdir(parents=True) + (pack_dir / "custom" / "spec.md").write_text( + "# Manifest-declared Spec\n", encoding="utf-8" + ) + if extra_file: + # An undeclared convention-path file the manifest points away from. + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text( + "# Stray Convention Spec\n", encoding="utf-8" + ) + manifest = { + "schema_version": "1.0", + "preset": { + "id": "mypack", + "name": "My Pack", + "version": "1.0.0", + "description": "declares a non-convention file path", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "template", + "name": "spec-template", + "file": "custom/spec.md", + "strategy": "replace", + } + ] + }, + } + with open(pack_dir / "preset.yml", "w") as f: + yaml.dump(manifest, f) + PresetRegistry(presets_dir).add( + "mypack", {"version": "1.0.0", "priority": 10} + ) + return pack_dir + + def test_resolve_uses_manifest_declared_file_path(self, project_dir): + """resolve() must honor a manifest-declared non-convention file: path. + + Previously the tier-2 loop was convention-only, so it returned the + core template and resolve_with_source() misattributed source='core', + diverging from collect_all_layers()/resolve_content(). + """ + pack_dir = self._install_pack_with_manifest_file(project_dir) + resolver = PresetResolver(project_dir) + + result = resolver.resolve("spec-template") + assert result == pack_dir / "custom" / "spec.md" + assert "Manifest-declared Spec" in result.read_text() + + sourced = resolver.resolve_with_source("spec-template") + assert sourced is not None + assert "mypack" in sourced["source"] + # resolve() must agree with collect_all_layers()'s top layer. + layers = resolver.collect_all_layers("spec-template") + assert Path(layers[0]["path"]) == pack_dir / "custom" / "spec.md" + + def test_resolve_manifest_file_wins_over_undeclared_convention_file( + self, project_dir + ): + """A stray convention-path file must not shadow the manifest's file:.""" + pack_dir = self._install_pack_with_manifest_file( + project_dir, extra_file=True + ) + resolver = PresetResolver(project_dir) + result = resolver.resolve("spec-template") + assert result == pack_dir / "custom" / "spec.md" + assert "Manifest-declared Spec" in result.read_text() + + def test_resolve_skips_convention_when_manifest_file_missing(self, project_dir): + """When the manifest declares a file: that does not exist, resolve() + must NOT fall back to a convention file in the same pack (that would + mask a typo) — it skips the pack and resolves core instead.""" + presets_dir = project_dir / ".specify" / "presets" + pack_dir = presets_dir / "mypack" + # Manifest declares custom/spec.md (MISSING); a convention file exists + # in the pack and must NOT be used. + (pack_dir / "templates").mkdir(parents=True) + (pack_dir / "templates" / "spec-template.md").write_text( + "# Stray Convention Spec\n", encoding="utf-8" + ) + manifest = { + "schema_version": "1.0", + "preset": { + "id": "mypack", + "name": "My Pack", + "version": "1.0.0", + "description": "declares a missing file path", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "template", + "name": "spec-template", + "file": "custom/spec.md", + "strategy": "replace", + } + ] + }, + } + with open(pack_dir / "preset.yml", "w") as f: + yaml.dump(manifest, f) + PresetRegistry(presets_dir).add( + "mypack", {"version": "1.0.0", "priority": 10} + ) + + resolver = PresetResolver(project_dir) + result = resolver.resolve("spec-template") + assert result is not None + content = result.read_text() + assert "Stray Convention Spec" not in content # pack convention skipped + assert "Core Spec Template" in content # fell through to core + + def test_resolve_skips_convention_when_manifest_file_is_directory( + self, project_dir + ): + """When the manifest's file: path resolves to a DIRECTORY (not a regular + file), resolve()/collect_all_layers() must treat it as missing — exists() + would accept it and downstream read_text() on a directory would crash. + The pack is skipped (no convention fallback), so core wins.""" + presets_dir = project_dir / ".specify" / "presets" + pack_dir = presets_dir / "mypack" + # Declared file: custom/spec.md is created as a DIRECTORY. + (pack_dir / "custom" / "spec.md").mkdir(parents=True) + # A convention file also exists and must NOT be used. + (pack_dir / "templates").mkdir(parents=True) + (pack_dir / "templates" / "spec-template.md").write_text( + "# Stray Convention Spec\n", encoding="utf-8" + ) + manifest = { + "schema_version": "1.0", + "preset": { + "id": "mypack", + "name": "My Pack", + "version": "1.0.0", + "description": "declares a file: that is actually a directory", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "template", + "name": "spec-template", + "file": "custom/spec.md", + "strategy": "replace", + } + ] + }, + } + with open(pack_dir / "preset.yml", "w") as f: + yaml.dump(manifest, f) + PresetRegistry(presets_dir).add( + "mypack", {"version": "1.0.0", "priority": 10} + ) + + resolver = PresetResolver(project_dir) + result = resolver.resolve("spec-template") + assert result is not None + assert result.is_file() # never a directory + content = result.read_text() + assert "Stray Convention Spec" not in content # pack convention skipped + assert "Core Spec Template" in content # fell through to core + # collect_all_layers() must agree: the directory is not a layer. + layers = resolver.collect_all_layers("spec-template") + assert all(Path(layer["path"]).is_file() for layer in layers) + assert all( + Path(layer["path"]) != pack_dir / "custom" / "spec.md" + for layer in layers + ) + def test_resolve_override_takes_priority_over_pack(self, project_dir, pack_dir): """Test that overrides take priority over installed packs.""" # Install the pack