From 552b5e509632b29844d940a463deae215b3ea220 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 5 Jul 2026 19:42:36 +0500 Subject: [PATCH 1/5] fix(presets): resolve() honors manifest-declared file: for installed presets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PresetResolver.resolve()'s tier-2 (installed presets) loop was convention-only: it looked for templates/.md and .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 --- src/specify_cli/presets/__init__.py | 28 +++++++++++ tests/test_presets.py | 78 +++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 863b6ef7dc..515809c238 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2676,6 +2676,34 @@ 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). + manifest_file_path = None + 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): + manifest_file_path = tmpl.get("file") + manifest_found_entry = True + break + if manifest_file_path: + manifest_candidate = pack_dir / manifest_file_path + if manifest_candidate.exists(): + return manifest_candidate + # Declared file missing: skip this pack's convention fallback. + continue + if manifest_found_entry: + # Manifest lists the template without a file: nothing here. + continue for subdir in subdirs: if subdir: candidate = pack_dir / subdir / f"{template_name}{ext}" diff --git a/tests/test_presets.py b/tests/test_presets.py index 54cb9dd5b3..8478067ac7 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -884,6 +884,84 @@ 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_override_takes_priority_over_pack(self, project_dir, pack_dir): """Test that overrides take priority over installed packs.""" # Install the pack From 24901fe2ce21f14b079822794a7aab4e02df76d0 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 22:18:32 +0500 Subject: [PATCH 2/5] 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 --- src/specify_cli/presets/__init__.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 515809c238..ccf63d33b7 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2702,7 +2702,11 @@ def resolve( # Declared file missing: skip this pack's convention fallback. continue if manifest_found_entry: - # Manifest lists the template without a file: nothing here. + # Manifest lists this template but with an empty/falsey + # ``file`` value (``file`` is a required key per + # PresetManifest._validate(), so this is a non-usable path, + # not a truly absent one). Don't fall through to convention + # — mirrors collect_all_layers(). continue for subdir in subdirs: if subdir: From 4e75cf586e4e99c8f665a3b3d35be9abb3006d13 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 9 Jul 2026 19:36:12 +0500 Subject: [PATCH 3/5] 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 --- tests/test_presets.py | 45 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/test_presets.py b/tests/test_presets.py index 8478067ac7..898eb1c85d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -962,6 +962,51 @@ def test_resolve_manifest_file_wins_over_undeclared_convention_file( 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_override_takes_priority_over_pack(self, project_dir, pack_dir): """Test that overrides take priority over installed packs.""" # Install the pack From 74a8b07ad93b00cf5221766d84882631cae2b679 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 10 Jul 2026 21:33:08 +0500 Subject: [PATCH 4/5] 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) --- src/specify_cli/presets/__init__.py | 15 ++++++-- tests/test_presets.py | 57 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index ccf63d33b7..892a5c1cd5 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2697,9 +2697,13 @@ def resolve( break if manifest_file_path: manifest_candidate = pack_dir / manifest_file_path - if manifest_candidate.exists(): + # is_file() (not exists()) so a manifest ``file:`` that points + # at a directory is treated as missing rather than returned to + # callers that will read_text() it and crash. + if manifest_candidate.is_file(): return manifest_candidate - # Declared file missing: skip this pack's convention fallback. + # Declared file missing/non-file: skip this pack's convention + # fallback. continue if manifest_found_entry: # Manifest lists this template but with an empty/falsey @@ -2994,9 +2998,12 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: candidate = None if manifest_file_path: manifest_candidate = pack_dir / manifest_file_path - if manifest_candidate.exists(): + # is_file() (not exists()): a directory at the declared path is + # not a usable layer, so treat it as missing (parity with + # resolve()). + if manifest_candidate.is_file(): candidate = manifest_candidate - # Explicit file path that doesn't exist: skip convention + # Explicit file path that isn't a regular file: 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 diff --git a/tests/test_presets.py b/tests/test_presets.py index 898eb1c85d..19f726b3d1 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -1007,6 +1007,63 @@ def test_resolve_skips_convention_when_manifest_file_missing(self, project_dir): 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 From a0344d09b503fb19e2897414eb6be9db68af77fa Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 13 Jul 2026 22:16:39 +0500 Subject: [PATCH 5/5] refactor(presets): extract shared _manifest_declared_template for resolve()/collect_all_layers() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) -> (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) --- src/specify_cli/presets/__init__.py | 110 +++++++++++++++------------- 1 file changed, 58 insertions(+), 52 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 892a5c1cd5..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. @@ -2685,32 +2718,17 @@ def resolve( # 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). - manifest_file_path = None - 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): - manifest_file_path = tmpl.get("file") - manifest_found_entry = True - break - if manifest_file_path: - manifest_candidate = pack_dir / manifest_file_path - # is_file() (not exists()) so a manifest ``file:`` that points - # at a directory is treated as missing rather than returned to - # callers that will read_text() it and crash. - if manifest_candidate.is_file(): - return manifest_candidate - # Declared file missing/non-file: skip this pack's convention - # fallback. - continue - if manifest_found_entry: - # Manifest lists this template but with an empty/falsey - # ``file`` value (``file`` is a required key per - # PresetManifest._validate(), so this is a non-usable path, - # not a truly absent one). Don't fall through to convention - # — mirrors collect_all_layers(). + 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: @@ -2979,34 +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 - # is_file() (not exists()): a directory at the declared path is - # not a usable layer, so treat it as missing (parity with - # resolve()). - if manifest_candidate.is_file(): - candidate = manifest_candidate - # Explicit file path that isn't a regular file: 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