feat(presets): let a preset declare a required extension - #4250
feat(presets): let a preset declare a required extension#4250Yash-Chindam wants to merge 3 commits into
Conversation
A preset whose command overrides call into an extension is inert without it, but the overrides fall through to the core workflow, so nothing errors -- the feature just silently does less than the user expects. Until now the only place that dependency could be stated was the README, which fails exactly the user who did not read it. Add an optional requires.extensions to preset.yml, accepting either a bare extension id or a mapping with an optional version specifier and an optional required flag. Validation mirrors the requires.speckit_version strictness from github#3980: a non-list, a member that is neither string nor mapping, a missing or malformed id, a non-string or unparseable version, and a non-boolean required each raise PresetValidationError rather than surfacing later as a bare TypeError from re.match or SpecifierSet. On `specify preset add`, warn once for each unsatisfied dependency, naming the extension and the command that installs it. The check runs at the single point where the --dev, --from, and catalog paths converge, so all three behave the same. It warns rather than fails: these presets are written to degrade safely, and three catalog entries already declare the dependency, so failing would break installs that work today. The field is optional, so every existing preset stays valid and silent. Closes github#4231 Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
There was a problem hiding this comment.
Pull request overview
Adds manifest-declared extension dependencies for presets and install-time warnings when requirements are unmet.
Changes:
- Validates and normalizes
requires.extensions. - Checks installed extension versions and emits actionable warnings.
- Documents the schema and adds dependency tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/presets/__init__.py |
Adds dependency validation and resolution. |
src/specify_cli/presets/_commands.py |
Displays install-time warnings. |
tests/test_presets.py |
Tests validation and dependency checks. |
presets/PUBLISHING.md |
Documents dependency declarations. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| f"{_escape_markup(dep['installed'])} does not satisfy " | ||
| f"{_escape_markup(dep['version'])}" | ||
| ) | ||
| console.print(f" Install with: specify extension add {extension_id}") |
| for dep in declared: | ||
| metadata = registry.get(dep["id"]) | ||
| if metadata is None: | ||
| unmet.append({**dep, "installed": None, "reason": "missing"}) |
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
…sabled Addresses review feedback on github#4250. `specify extension add <id>` refuses an already-installed extension without --force, so suggesting it for a version mismatch handed the user a command that could only fail. Suggest `extension update` for a version mismatch and `extension enable` for a disabled one, keeping `add` for a genuinely missing extension. A disabled extension was also treated as satisfied, because the registry entry exists. Resolution skips disabled extensions, so the preset stays exactly as inert as if the extension were absent, with no warning to explain it. Report it as a distinct "disabled" reason, ahead of any version check -- enabling is the prerequisite, and the version may be fine once it is. Also correct the closing line, which said the extensions "will do nothing until they are present" -- inaccurate for a disabled extension, which is present. Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
|
Both findings addressed in 853e67d. I verified each against the code before changing anything, and both were correct. Remediation command didn't match the reason. Confirmed: The remedy now follows the reason — Disabled extensions were treated as satisfied. Also confirmed, and the consequence is worse than a missed warning: End-to-end, with the extension installed but disabled: Running that suggestion enables the extension, and re-installing the preset is then silent. One thing I changed beyond the two comments: the closing line read "will do nothing until they are present", which is wrong for a disabled extension that is present. It now reads "until this is resolved". Two new tests cover the disabled case and its ordering against a version mismatch, and the multi-dependency test now mixes present, absent, disabled, and optional. The four CI workflows are still showing Disclosure: this comment and 853e67d were written by Claude Code (model: Claude Opus 5), acting autonomously on behalf of @Yash-Chindam. The commit carries an |
| f"{_escape_markup(dep['installed'])} does not satisfy " | ||
| f"{_escape_markup(dep['version'])}" | ||
| ) | ||
| remedy = f"specify extension update {extension_id}" |
|
Please address Copilot feedback |
Assisted-by: ChatGPT (model: GPT-5, supervised)
|
The remaining Copilot finding is addressed in 972cb57. I checked the version-constraint behavior before changing the remediation text. Version remediation could over-promise. Confirmed: the manifest accepts general PEP 440 constraints such as Regression coverage. Added a focused test that verifies an upper-bound mismatch does not suggest The focused preset dependency tests pass (3 passed), and Ruff reports no lint errors. The full preset-file run was also attempted; its unrelated failures are the existing Windows symlink-privilege cases and Typer compatibility failures documented in the PR, with the new test passing. Disclosure: this comment and 972cb57 were written by ChatGPT (model: GPT-5), acting under the direction of @Yash-Chindam. The commit carries an |
Description
Closes #4231.
A preset whose command overrides call into an extension is inert without it — but the overrides fall through to the core workflow, so nothing errors. The feature just silently does less than the user expects, with nothing in the install output pointing at the cause. Until now the only place that dependency could be stated was the README, which fails exactly the user who did not read it.
Three community presets already declare
requires.extensionsincatalog.community.json(aide-in-place,inventory-alignment,mde), and the preset submission template already collects the field — butpreset.ymlhad no supported key for it and nothing read it. This adds the manifest field and the install-time check.What changed
Schema.
preset.ymlaccepts an optionalrequires.extensions, taking either a bare id or a mapping:Validation. Follows the
requires.speckit_versionstrictness established by #3980, for the same reason: an unvalidated value reachesre.matchorSpecifierSetlater and surfaces as a bareTypeErrorthat no caller handles as a malformed manifest. A non-list, a member that is neither string nor mapping, a missing or non-string or badly-shapedid, a non-string/blank/unparseableversion, and a non-booleanrequiredeach raisePresetValidationErrornaming the offending index.Install-time check.
PresetManager.find_unmet_extension_dependencies()reports rather than raises.specify preset addwarns once per unsatisfied dependency:The check sits at the single point where the
--dev,--from, and catalog paths converge, so all three behave identically rather than drifting.Design decisions
These follow the positions I set out in the issue against the assessment's carried-forward questions. Each is easy to change if you'd rather go the other way.
requiredflag leaves the door open for an opt-in hard-fail later.version, so the incremental cost is the comparison itself. Deferring it would ship a field that validates but is silently ignored — the same shape as the problem this issue is about.extension.ymlleft alone. It has the identical limitation, but no extension in the catalog declares a dependency on another extension, so there's no demonstrated need. Happy to mirror it here or in a follow-up.--devor--from <url>installs, so the manifest is the only copy present on every path. Documented inPUBLISHING.mdrather than mechanically reconciled; validating the catalog field against the packaged manifest seems better as its own change against theadd-community-presetworkflow.The field is optional, so every existing preset stays valid and silent.
Worth flagging
The warning will fire for nobody on day one. The three presets carrying
requires.extensionsdo so only in their catalog entries, not in their packagedpreset.yml. I maintaininventory-alignmentand will add it there; the other two need their authors. Not a blocker, but the feature starts with no live coverage.Testing
Automated
tests/test_presets.py: 624 passed, 8 failed. All 8 failures areWinError 1314symlink-privilege failures from my Windows environment and reproduce identically onmainwith this branch stashed.tests/test_extensions.py+tests/test_extension_registration.py: 540 passed, 2 failed, same symlink cause.22 new tests:
requires.extensionsabsent stays valid and reports no dependenciesid, non-string/blank/unparseableversion, non-booleanrequired)required: falsenever reportedManual
Per the mapping rules,
src/specify_cli/*.py→ test the affected CLI command. This changesspecify preset add; it is not an init/scaffolding change, so no slash command is affected.Agent: n/a (CLI change) | OS/Shell: Windows 11 / Git Bash, Python 3.11.14
specify preset add --devspecify preset add --devspecify preset add --dev>=0.1.0against installed0.1.0→ no warning.specify preset add --dev>=9.0.0against installed0.1.0→ warning showing both versions.specify preset add --devspecify preset add <id>lean) via thepreset_idbranch → installs clean, no warning, no regression.specify preset add --from <url>test_preset_add_from_url_reads_in_bounded_chunks, which executes the new call site on the--frompath.All three install branches are therefore exercised, which is what the shared call site is meant to guarantee. Re-verified after rebasing onto
mainat 1.0.1.dev0.uv run specify --helpuv sync && uv run pytestAI Disclosure
Filed by @Yash-Chindam. This change was written by Claude Code (model: Claude Opus 5), acting autonomously on my behalf — code generation, tests, and this description, not just comments. The commit carries an
Assisted-by: Claude Code (model: Claude Opus 5, autonomous)trailer.Worth recording one thing it caught in its own work: the first version of
find_unmet_extension_dependencies()broketest_preset_add_from_url_reads_in_bounded_chunks, which passes a duck-typedSimpleNamespacemanifest with norequires_extensionsattribute. That is a real robustness gap rather than a test artifact — the method is public and reachable with a hand-built manifest, the same casecheck_compatibility()already guards against — so the fix went into the code, not the test.Every check in the Testing section was executed by Claude Code on my machine at my direction and the results reviewed by me; I am not claiming I re-ran each command by hand. I will disclose agent involvement again in each review-round comment rather than relying on this section to cover them.