feat(extensions): accept provides.templates and provides.scripts in manifest - #4012
Conversation
…anifest Extensions could only formally declare commands under `provides` (plus config/hooks/events); templates and scripts shipped by an extension were picked up purely by filename convention, with no id, description, or metadata. Add optional `provides.templates` and `provides.scripts` sections to the extension manifest schema, mirroring the preset template shape minus an authorable `strategy` (extension artifacts always resolve as replace, so a present `strategy` key is now a validation error rather than a silently accepted no-op). ExtensionManifest gains `templates`/`scripts` properties so tooling can enumerate an extension's declared artifacts directly from the manifest. An extension may now satisfy the "must provide something" rule with only a template or script, not just a command/hook/event. Addresses the manifest-schema portion of github#4010; resolver authoritative-vs-convention precedence for these new sections is left for a follow-up.
collect_all_layers only consulted ExtensionManifest for command resolution, leaving provides.templates/.scripts purely decorative -- a declared entry whose file didn't sit at the conventional path was validated but never resolved. Extend the existing manifest-fallback branch to cover template_type "template" and "script" the same way it already does "command": convention lookup first, manifest lookup as fallback so undeclared on-disk files keep resolving unchanged.
There was a problem hiding this comment.
Pull request overview
Adds declarative extension template/script metadata and resolver support.
Changes:
- Validates
provides.templatesandprovides.scripts. - Exposes artifacts through
ExtensionManifest. - Adds resolver tests and API documentation.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Adds schema validation and properties. |
src/specify_cli/presets/__init__.py |
Resolves declared extension artifacts. |
tests/test_extensions.py |
Tests manifest validation. |
tests/test_presets.py |
Tests manifest-based resolution. |
extensions/EXTENSION-API-REFERENCE.md |
Documents the new schema. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
…ntion Copilot review on github#4012 found the manifest-declared template/script lookup was gated on convention lookup missing first, so a stale conventional file could shadow a declared entry at a non-conventional path, and resolve() never consulted the manifest at all (only collect_all_layers() did). Add a shared _extension_manifest_declared_template() helper and check it before convention-based lookup in both resolve() and collect_all_layers(), mirroring the preset manifest precedence. Also update EXTENSION-DEVELOPMENT-GUIDE.md, which still claimed provides only supports commands and required a command or hook.
|
Addressed the Copilot feedback in 0070d25:
Full suite (6621 passed, 9 skipped) and |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/specify_cli/presets/init.py:5176
- The implementation now makes manifest declarations authoritative, but the PR description still says convention lookup runs first and the manifest is consulted only on a miss. That describes the pre-fix behavior and contradicts both these lines and the new precedence tests; please update the PR description so reviewers and release-note consumers see the actual manifest-first contract.
# The extension manifest is authoritative, same as preset manifests
# above: check it before convention-based lookup so a declared entry
# at a non-conventional path wins over a stale conventional file.
entry, manifest_candidate = self._extension_manifest_declared_template(
ext_dir, template_name, template_type
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Please address test & lint errors |
… path _extension_manifest_declared_template() resolved ext_dir/rel_path before returning it, which follows symlinks in ext_dir's ancestors (e.g. macOS's symlinked tmp dir) and diverges from the unresolved paths convention-based lookup returns for the same directory. Resolve only for the traversal containment check; return the unresolved candidate. Fixes the 4 CI test failures across all OS/Python matrix jobs on github#4012.
|
Fixed in 467c0f7: `_extension_manifest_declared_template()` was calling `.resolve()` on the returned candidate path, which follows symlinks in `ext_dir`'s ancestors (e.g. the symlinked tmp dir on macOS CI runners) and diverges from the unresolved paths convention-based lookup returns for the same directory — that's what the 4 macOS/ubuntu/windows pytest failures were. Now resolution is only used for the path-traversal containment check; the returned path stays unresolved, matching convention lookup. Reproduced the exact failures locally by pointing |
There was a problem hiding this comment.
Review details
Suppressed comments (4)
extensions/EXTENSION-API-REFERENCE.md:141
- Issue #4010 explicitly includes updating
AGENTS.mdwith the new sections and replace-only rule in its acceptance criteria, but this PR only updates the extension guides. Add the corresponding contributor guidance to complete the documented scope.
#### `provides.templates[].strategy` / `provides.scripts[].strategy`
- Not an authorable field. Extension-contributed templates and scripts are
always resolved as `replace`; a manifest that includes a `strategy` key on
one of these entries is rejected with a `ValidationError`. Composable
strategies (`wrap`/`prepend`/`append`) are preset-only.
extensions/EXTENSION-DEVELOPMENT-GUIDE.md:180
hooksandeventsare top-level manifest fields, notprovidessub-fields (the validator reads them fromself.data). This wording can lead authors to indent them underprovides, where they will not satisfy validation. Distinguish the threeprovidesfields from the top-level alternatives.
**Optional sub-fields** (at least one of `commands`, `templates`, `scripts`, `hooks`, or `events` is required):
src/specify_cli/presets/init.py:5184
- The PR summary says convention lookup runs first and the manifest is consulted only on a miss, but this code intentionally makes the manifest authoritative before convention lookup. That matches issue #4010 and the new tests, so the implementation appears correct; update the PR description to state the actual manifest-first precedence.
# The extension manifest is authoritative, same as preset manifests
# above: check it before convention-based lookup so a declared entry
# at a non-conventional path wins over a stale conventional file.
entry, manifest_candidate = self._extension_manifest_declared_template(
ext_dir, template_name, template_type
)
if manifest_candidate is not None:
return manifest_candidate
if entry is not None:
src/specify_cli/extensions/init.py:583
- Duplicate names within
provides.templatesorprovides.scriptsare currently accepted. The resolver returns the first matching entry (presets/__init__.py:5018-5037), making later declarations unreachable while enumeration APIs still expose them. Reject duplicate names within each section and add regression coverage.
for entry in entries:
if not isinstance(entry, dict):
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Closes #4010 |
|
Edit: this PR merged (684b3d8) before 1ac3c3c was pushed, so that commit did not make it into Addressed the remaining Copilot feedback in 1ac3c3c:
Full suite (6623 passed, 9 skipped) and Re: the AGENTS.md acceptance-criterion item — I intentionally left that file alone. It documents only the AI-agent integration subsystem (adding Claude/Gemini/Copilot/etc. support); none of the other subsystems (presets, the extension system itself) are covered there either. The extension manifest schema is documented in |
Summary
Extensions could only formally declare commands under
provides(plusconfig/hooks/events) — seeExtensionManifest(src/specify_cli/extensions/__init__.py). Templates and scripts an extension shipped were picked up purely by filename convention during preset/template resolution, with noid,name, ordescription, and forced toreplacewith no way to make that intent explicit.This PR implements #4010 end to end — manifest schema, validation, and resolver wiring:
provides.templatesandprovides.scriptsare now optional, validated sections ofextension.yml, mirroring the shapePresetManifestalready uses for its own templates (name/file/description), minus an authorablestrategy— extension-provided artifacts always resolve asreplace, and a manifest that includes astrategykey on one of these entries now raises a clearValidationErrorinstead of silently accepting (and ignoring) it.provides.scripts[].runtimesaccepts an optional list restricted tobash/powershell/python— declared metadata, not inferred from the file extension.ExtensionManifestgainstemplatesandscriptsproperties (parity with the existingcommands/configproperties) so tooling (e.g. a setup wizard,extension info) can enumerate an extension's declared artifacts directly from the manifest.PresetResolver.collect_all_layers,resolve(), andresolve_with_source()(src/specify_cli/presets/__init__.py) now consultExtensionManifest.templates/.scriptsfortemplate_type in {"template", "script"}the same way they already did for.commands, via the shared_extension_manifest_declared_template()helper. The manifest is authoritative and is checked before convention-based lookup: a declared entry whosefiledoesn't sit at the conventional path (templates/<name>.md/scripts/<name>.shdirectly under the extension root) still resolves, a declared entry wins over a stale conventional file at the same name, and a declared-but-missing file does not fall back to convention. Undeclared on-disk files keep resolving via convention exactly as before (no regression).provides.templates/provides.scriptsentries must have unique names within their section — a duplicate is rejected with aValidationError, since the resolver returns the first entry matching a name and a later duplicate would otherwise be silently unreachable.schema_version: "1.0"; no version bump needed.extensions/EXTENSION-API-REFERENCE.mdmanifest schema section and Python API property list updated. The doc's "Always resolve as 'replace'" claim now matches actual resolver behavior for templates/scripts, not just commands.extensions/EXTENSION-DEVELOPMENT-GUIDE.md'sprovidessection now distinguishes its own sub-fields (commands/templates/scripts) fromhooks/events, which are top-level manifest fields, not nested underprovides.Testing
New test class
TestExtensionManifestTemplatesAndScriptsintests/test_extensions.pycovers: valid declarations, templates/scripts-only extensions (no commands/hooks/events), section-type validation, per-entry shape/path-safety/name-format validation, rejectedstrategykey, rejected duplicate names within a section,runtimesvalidation (type + allowed values), anddescriptiontype-checking.Three new tests in
tests/test_presets.py::TestWrapStrategycover resolver precedence per the issue's acceptance criteria:test_extension_template_resolves_via_manifest_when_filename_differsandtest_extension_script_resolves_via_manifest_when_filename_differsassert a manifest-declared template/script resolves when its file lives away from the naming convention;test_extension_template_convention_lookup_unaffected_when_undeclaredasserts an undeclared on-disk template still resolves via the pre-existing convention (no regression).Confirmed all new tests fail against the pre-fix source (
git checkout HEAD~1 -- src/specify_cli/extensions/__init__.py src/specify_cli/presets/__init__.py) — 19/20 manifest tests withAttributeError/DID NOT RAISE, and the two new resolver-precedence tests with an empty layer list — then pass again after restoring the fix. The duplicate-name rejection added after review (git checkout HEAD -- src/specify_cli/extensions/__init__.pyagainst the pre-fix version of that file) was confirmed to fail withDID NOT RAISE ValidationErroron both parametrized cases before the fix, and pass after.AI Disclosure
Implemented autonomously by Claude Code (model: Claude Sonnet 5) under human direction: read the issue and related open PRs first to confirm no overlap, implemented the manifest schema/validation/properties and resolver wiring, wrote failing-first regression tests, and verified the full test suite and
rufflocally before pushing.