fix(presets): resolve() honors manifest-declared file: for installed presets (#3351)

jawwad-ali · claude · web-flow · commit fc1a3fd76c8b · 2026-07-13T15:40:36.000-05:00
* fix(presets): resolve() honors manifest-declared file: for installed presets

PresetResolver.resolve()'s tier-2 (installed presets) loop was
convention-only: it looked for templates/&lt;name&gt;.md and &lt;name&gt;.md,
ignoring a preset manifest that declares the template with an explicit,
non-convention file: path. So resolve() returned the core template (and
resolve_with_source() misattributed source='core') while
collect_all_layers()/resolve_content() correctly used the preset's
declared file — a divergence inside the same class. It could also return
a stray convention-path file the manifest deliberately points away from.
Mirror collect_all_layers()'s manifest-first logic: use the declared
file: when present (skip convention fallback if it's missing, to avoid
masking typos), and fall back to the convention walk only when the
manifest is absent or doesn't list the template.

Co-Authored-By: Claude Fable 5 &lt;noreply@anthropic.com&gt;

* docs(presets): clarify the empty/falsey manifest-file branch comment

Per review: 'file' is a required key for every template entry
(PresetManifest._validate()), so the manifest-found branch is reached
for an empty/falsey/non-usable 'file' value, not a truly absent one.
Reword the comment to say so. Comment-only.

Co-Authored-By: Claude Fable 5 &lt;noreply@anthropic.com&gt;

* fix(presets): resolve() returns only real files; test missing-file skip

Per review:
- Use is_file() (not exists()) when honoring a manifest-declared file: so a
  manifest pointing at a directory is treated as missing rather than
  returned to a caller that will read_text() it. Applied in both resolve()
  and collect_all_layers() so the two stay consistent.
- Add a regression test for the skip-convention-fallback-when-declared-file-
  missing behavior: manifest declares a missing custom/spec.md while the pack
  has a convention templates/spec-template.md; resolve() must skip the pack
  and fall through to core, not pick up the stray convention file.

Co-Authored-By: Claude Fable 5 &lt;noreply@anthropic.com&gt;

* fix(presets): resolve()/collect_all_layers() require a regular file for manifest file:

A manifest-declared file: path is honored via exists(), which also accepts
a directory. If a preset points file: at a directory, resolve() returned it
and downstream read_text() crashes. Use is_file() in both resolve() and
collect_all_layers() so a non-file (directory) is treated as missing and the
convention fallback is skipped (pack yields to core), matching the existing
missing-file behavior.

Adds a directory-at-file: test (fails on exists(), passes on is_file()) that
also asserts collect_all_layers() never returns the directory as a layer.

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;

* refactor(presets): extract shared _manifest_declared_template for resolve()/collect_all_layers()

Both methods reimplemented the manifest-entry lookup + authoritative-fallback
rules independently — the exact duplication that let them diverge and caused the
bug this PR fixes. Extract a single _manifest_declared_template(pack_dir, name,
type) -&gt; (entry, candidate) helper (candidate is the declared file only when it
is_file(); a declared-but-unusable file returns (entry, None) so callers skip the
convention fallback). resolve() and collect_all_layers() now both call it, so
their manifest-first resolution cannot silently diverge again.

Pure refactor, behavior-preserving: full test_presets.py (331) still passes,
including the directory-at-file:, missing-file, and manifest-file-wins cases.

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;

---------

Co-authored-by: Claude Fable 5 &lt;noreply@anthropic.com&gt;
diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py
@@ -2588,6 +2588,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.
 
@@ -2690,6 +2723,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}"
@@ -2957,31 +3011,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
@@ -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
