fix(presets): resolve() honors manifest-declared file: for installed presets#3351
Open
jawwad-ali wants to merge 2 commits into
Open
fix(presets): resolve() honors manifest-declared file: for installed presets#3351jawwad-ali wants to merge 2 commits into
jawwad-ali wants to merge 2 commits into
Conversation
…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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes PresetResolver.resolve() so that installed preset packs (tier-2) honor the preset manifest’s declared file: path for a template, aligning resolve()/resolve_with_source() behavior with collect_all_layers() and resolve_content().
Changes:
- Update
PresetResolver.resolve()tier-2 installed-presets lookup to checkpreset.ymland prefer manifest-declaredfile:paths, skipping convention fallback when a declared file is missing. - Add tests ensuring
resolve()uses the manifest-declared file path and that undeclared convention-path files don’t shadow the manifest.
Show a summary per file
| File | Description |
|---|---|
| tests/test_presets.py | Adds regression tests for manifest-declared file: resolution and precedence over stray convention files. |
| src/specify_cli/presets/init.py | Updates tier-2 preset resolution to consult preset.yml and resolve to manifest-declared file paths before convention-based lookup. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
PresetResolver.resolve()'s tier-2 (installed presets) loop is convention-only — it checks<pack>/templates/<name>.mdand<pack>/<name>.md, ignoring a preset manifest that declares the template with an explicitfile:path:So when a preset's
preset.ymldeclares e.g.{type: template, name: spec-template, file: custom/spec.md},resolve()returns the core template andresolve_with_source()misattributessource='core'— whilecollect_all_layers()andresolve_content()in the same class correctly honor the declared file. It can also return a straytemplates/spec-template.mdthe manifest deliberately points away from.Reproduced on main @
bba473c: with a pack declaringfile: custom/spec.md,resolve('spec-template')returned the core path, not the pack's.Fix
Mirror
collect_all_layers()'s manifest-first logic in the tier-2 loop: read the manifest, and if it lists the template with afile:, resolve topack_dir/filewhen it exists (and skip the convention fallback when it's missing, to avoid masking typos — exactly ascollect_all_layersdoes); only fall back to the convention subdir walk when the manifest is absent or doesn't list the template.resolve_with_source()is fixed for free since it delegates toresolve().Testing
Two new tests in
TestPresetResolver(both fail-before / pass-after, source-stash verified):test_resolve_uses_manifest_declared_file_path: a pack declaringfile: custom/spec.md→resolve()returns that file,resolve_with_source()['source']names the pack, and it equalscollect_all_layers()[0]['path'];test_resolve_manifest_file_wins_over_undeclared_convention_file: an undeclaredtemplates/spec-template.mdin the pack does not shadow the manifest'sfile:.Full
tests/test_presets.py: 328 passed (no regressions — convention-following presets unaffected).uvx ruff checkclean.AI Disclosure
Found and fixed with Claude Code (Claude Fable 5) under my direction. AI identified the tier-2 convention-only divergence from
collect_all_layers; I reproduced the wrong resolution, mirrored the authoritative logic, verified fail-before/pass-after and the full suite, and reviewed the diff.