github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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/<name>.md and <name>.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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) -> (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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Ali jawwad
2026-07-14 01:40:36 +05:00
committed by GitHub
parent 993083405e
commit fc1a3fd76c
2 changed files with 248 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

91
src/specify_cli/presets/__init__.py
View File

@@ -2588,6 +2588,39 @@ class PresetResolver:
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 @@ class PresetResolver:
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 @@ class PresetResolver:
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

180
tests/test_presets.py
View File

@@ -884,6 +884,186 @@ class TestPresetResolver:
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