Skip to content

feat(presets): let a preset declare a required extension - #4250

Merged
mnriem merged 10 commits into
github:mainfrom
Yash-Chindam:feat/4231-preset-extension-dependency
Sep 1, 2026
Merged

feat(presets): let a preset declare a required extension#4250
mnriem merged 10 commits into
github:mainfrom
Yash-Chindam:feat/4231-preset-extension-dependency

Conversation

@Yash-Chindam

@Yash-Chindam Yash-Chindam commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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.extensions in catalog.community.json (aide-in-place, inventory-alignment, mde), and the preset submission template already collects the field — but preset.yml had no supported key for it and nothing read it. This adds the manifest field and the install-time check.

What changed

Schema. preset.yml accepts an optional requires.extensions, taking either a bare id or a mapping:

requires:
  speckit_version: ">=0.9.0"
  extensions:
    - "companion-extension"        # required, any version
    - id: "other-extension"
      version: ">=1.2.0"           # optional PEP 440 specifier
      required: false              # optional, defaults to true

Validation. Follows the requires.speckit_version strictness established by #3980, for the same reason: an unvalidated value reaches re.match or SpecifierSet later and surfaces as a bare TypeError that 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-shaped id, a non-string/blank/unparseable version, and a non-boolean required each raise PresetValidationError naming the offending index.

Install-time check. PresetManager.find_unmet_extension_dependencies() reports rather than raises. specify preset add warns once per unsatisfied dependency:

!  This preset depends on extensions that are not satisfied:
    speckit-inventory 0.1.0 does not satisfy >=9.0.0
      Install with: specify extension add speckit-inventory

The preset is installed and safe to use; the parts that rely on these
extensions will do nothing until they are present.

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.

  • Warn, not fail. These presets are written to degrade safely, and three catalog entries already declare the dependency — hard-failing would break installs that work today. The required flag leaves the door open for an opt-in hard-fail later.
  • Version constraints included in v1. The mapping form has to be parsed and validated anyway to accept 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.
  • Preset side only; extension.yml left 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.
  • Manifest authoritative, catalog a mirror. The catalog isn't consulted for --dev or --from <url> installs, so the manifest is the only copy present on every path. Documented in PUBLISHING.md rather than mechanically reconciled; validating the catalog field against the packaged manifest seems better as its own change against the add-community-preset workflow.

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.extensions do so only in their catalog entries, not in their packaged preset.yml. I maintain inventory-alignment and 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 are WinError 1314 symlink-privilege failures from my Windows environment and reproduce identically on main with this branch stashed. tests/test_extensions.py + tests/test_extension_registration.py: 540 passed, 2 failed, same symlink cause.

22 new tests:

  • requires.extensions absent stays valid and reports no dependencies
  • both declaration forms normalize to the same shape
  • 12 parametrized malformed-input cases (not-a-list, bad member type, missing/non-string/bad-pattern id, non-string/blank/unparseable version, non-boolean required)
  • dependency missing / installed / version satisfied / version unsatisfied
  • required: false never reported
  • registry entry with an unusable version is not invented into a mismatch
  • multiple dependencies evaluated independently

Manual

Per the mapping rules, src/specify_cli/*.py → test the affected CLI command. This changes specify 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

Command tested Notes
specify preset add --dev Dependency missing → warning naming the extension and install command. Preset still installs.
specify preset add --dev Extension installed, no constraint → no warning.
specify preset add --dev >=0.1.0 against installed 0.1.0 → no warning.
specify preset add --dev >=9.0.0 against installed 0.1.0 → warning showing both versions.
specify preset add --dev Preset declaring nothing → unchanged output.
specify preset add <id> Bundled preset (lean) via the preset_id branch → installs clean, no warning, no regression.
specify preset add --from <url> Covered by the existing test_preset_add_from_url_reads_in_bounded_chunks, which executes the new call site on the --from path.

All three install branches are therefore exercised, which is what the shared call site is meant to guarantee. Re-verified after rebasing onto main at 1.0.1.dev0.

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable)

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

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() broke test_preset_add_from_url_reads_in_bounded_chunks, which passes a duck-typed SimpleNamespace manifest with no requires_extensions attribute. That is a real robustness gap rather than a test artifact — the method is public and reachable with a hand-built manifest, the same case check_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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/specify_cli/presets/_commands.py Outdated
Comment thread src/specify_cli/presets/__init__.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Aug 21, 2026
…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)
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

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: install_from_directory raises Extension '<id>' is already installed without --force, so specify extension add <id> could only fail for a version mismatch. Reproduced it directly:

Error: Extension 'speckit-inventory' is already installed. Use 'specify
extension remove speckit-inventory' first, or retry with --force to overwrite.

The remedy now follows the reason — add for missing, update for a version mismatch, enable for a disabled one — and the label changed from "Install with:" to "Fix with:", since two of the three are no longer installs.

Disabled extensions were treated as satisfied. Also confirmed, and the consequence is worse than a missed warning: _collect_extension_layers skips disabled extensions (presets/__init__.py:5337, matching extensions/__init__.py:1000), so the preset is exactly as inert as if the extension were absent, while the registry entry made the check report success. Now reported as a distinct disabled reason, ordered ahead of the version check — enabling is the prerequisite, and the version may well be fine once it is.

End-to-end, with the extension installed but disabled:

!  This preset depends on extensions that are not satisfied:
    speckit-inventory is installed but disabled
      Fix with: specify extension enable speckit-inventory

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. tests/test_presets.py: 626 passed, 8 failed — all 8 are WinError 1314 symlink-privilege failures from my Windows environment that reproduce on main with this branch stashed.

The four CI workflows are still showing action_required, so they have not run yet — could you approve them when you get a chance?

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 Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/presets/_commands.py Outdated
@mnriem

mnriem commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

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 <2, ==1.2, exclusions, and possible downgrades, while extension update only moves forward to the catalog release. The warning now tells users to install a release satisfying the declared constraint, without claiming that extension update will resolve every valid constraint.

Regression coverage. Added a focused test that verifies an upper-bound mismatch does not suggest specify extension update and instead reports the required constraint explicitly.

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 Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Dependency detection can falsely warn for active unregistered extensions and silently accept unverifiable versions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/specify_cli/presets/init.py:995

  • This registry-only lookup reports a dependency as missing even when the extension is active through the supported unregistered-directory fallback. PresetResolver._get_all_extensions_by_priority() explicitly includes safe extension directories absent from .registry at implicit priority 10 (src/specify_cli/presets/__init__.py:5359-5364), with coverage in tests/test_presets.py:2038-2060. Check that fallback (and its manifest version when available) before emitting a missing warning; otherwise a working preset/extension pair produces a false remediation warning.
            metadata = registry.get(dep["id"])
            if metadata is None:
                unmet.append({**dep, "installed": None, "reason": "missing"})

src/specify_cli/presets/init.py:1022

  • A version-constrained dependency with a missing or non-string registry version is treated as satisfied here, although its version cannot be shown to meet the constraint. ExtensionRegistry.get() accepts any mapping entry, so corrupted or legacy metadata can take this path and suppress the exact warning this check is meant to provide. Report an unmet/unknown-version reason and render an appropriate remediation instead of failing open.
            if installed_version is None:
                # A registry entry without a usable version cannot be compared.
                # Treat it as satisfied rather than inventing a failure, since
                # the extension is demonstrably installed and enabled.
                continue
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread presets/PUBLISHING.md Outdated
@Yash-Chindam
Yash-Chindam force-pushed the feat/4231-preset-extension-dependency branch from 972cb57 to db9a90d Compare September 1, 2026 03:59
Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 1, 2026
…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)
Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 1, 2026
Addresses review feedback on github#4250.

The guide said a mapping was for "a version floor", but the field accepts any
PEP 440 specifier, so upper bounds, exact pins, and exclusions were all
undocumented. Say "version constraint", show a bounded range in the example,
and state the accepted forms explicitly.

Two adjacent claims had also drifted from the behaviour and are corrected in
the same pass. The guide promised the warning would "name the command that
fixes it", which stopped being true for a version mismatch once that case
began stating the constraint instead of naming a command that cannot satisfy
every specifier. And the notes listed only missing and version-unsatisfied
dependencies as warned about, never mentioning disabled ones.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

Addressed in db9a90d, and the branch is rebased onto current main.

"Version floor" understated the field. Correct — version is passed to SpecifierSet, so it accepts any PEP 440 specifier. I verified <2, ==1.2.0, !=1.3.0, >=1.2.0 and the bounded range >=1.2.0,<2 all validate and round-trip through requires_extensions. The guide now says "version constraint", the example shows a bounded range, and the accepted forms are stated explicitly.

Two adjacent claims in the same section had also drifted from the behaviour, so I corrected them in the same pass rather than leave them for a later round:

  • It promised the warning would "name the command that fixes it". That stopped being true for a version mismatch once that branch began stating the constraint instead of naming a command, per the previous round.
  • The notes listed only missing and version-unsatisfied dependencies as producing a warning, and never mentioned disabled ones, which became a third reason earlier in this review.

Rebase. main had moved 39 commits, four of which touch presets/ (#4088, #4341, #4094, #3843). Rebased cleanly with no conflicts and re-verified afterwards rather than assuming: tests/test_presets.py is 634 passed / 8 failed, the 8 being the WinError 1314 symlink-privilege cases from my Windows environment that reproduce on main unmodified. End-to-end, a bounded range still reports correctly against an installed extension:

speckit-inventory 0.1.0 does not satisfy >=0.0.1,<0.0.9
  Fix with: install a release of speckit-inventory satisfying >=0.0.1,<0.0.9

Note the PR still has no CI results — the four workflows have never run, so nothing here has been checked on Linux or macOS. Whenever you get a chance to approve the workflow runs, that would be useful before merge.

Disclosure: this comment and db9a90d were written by Claude Code (model: Claude Opus 5), acting autonomously on behalf of @Yash-Chindam. The commit carries an Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unusable version strings and stale registry entries can produce incorrect dependency results.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/specify_cli/presets/__init__.py Outdated
Comment thread src/specify_cli/presets/__init__.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 1, 2026
…s uncomparable

Addresses review feedback on github#4250.

A registry entry is not proof the extension can contribute. When the entry
survives after .specify/extensions/<id> is removed, PresetResolver skips the
extension outright -- both template lookup and layer collection guard on
is_dir() -- so the preset is as inert as if it were never installed, while the
surviving entry read as satisfied. Report that state as a distinct "stale"
reason, ahead of the disabled and version checks, and remediate it with a
forced reinstall rather than a plain add.

The uncomparable-version guard also only covered non-string values. An
unparseable string such as "unknown" passed it and reached version_satisfies(),
which catches InvalidVersion and returns False -- reporting a mismatch against
a version that was never actually evaluated, and contradicting the documented
behaviour that unusable versions are not invented into mismatches. Check
parseability before comparing so only real comparisons reach the warning.

The test helper now creates the extension directory alongside the registry
entry, matching what the installer does, with an opt-out for the stale case.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

Both findings addressed in 47a5bb6. I confirmed each against the code first; both were real.

Stale registry entries read as satisfied. Confirmed — PresetResolver guards on ext_dir.is_dir() in both template lookup and layer collection, so once .specify/extensions/<id> is gone the extension contributes nothing, while the surviving registry entry made the check report success. That is the same silent-inertness this feature exists to surface, one layer down.

Now reported as a distinct stale reason, checked ahead of disabled and the version comparison, since restoring the files is the prerequisite for either of those mattering:

speckit-inventory is registered but its files are missing
  Fix with: specify extension add speckit-inventory --force

--force rather than a plain add, because the registry entry is still present and a plain add would be refused as already installed.

Unparseable versions were reported as mismatches. Also confirmed. The guard only covered non-string values, so a string like "unknown" passed it and reached version_satisfies(), which catches InvalidVersion and returns False — indistinguishable from a genuine mismatch, and contradicting the documented behaviour that unusable versions are not invented into failures. Parseability is now checked before comparing, via a small _is_comparable_version() helper. Verified end-to-end that "unknown" against >=9.0.0 produces no warning, while a real 0.1.0 against >=9.0.0 still does.

Test-helper correction worth flagging. The existing helper only wrote the registry entry, never the extension directory — so every prior dependency test was, unknowingly, exercising the stale state. It now creates the directory as the real installer does, with an opt-out used by the stale tests. Without that, the new check would have silently passed for the wrong reason.

Added tests: stale detection, its precedence over disabled and version, its remediation text, and a parametrized uncomparable-version case covering None, a non-string, "unknown", "", and "latest".

tests/test_presets.py: 641 passed, 8 failed — the same WinError 1314 symlink-privilege cases from my Windows environment, which reproduce on unmodified main.

The PR still has no CI results; the workflows have never run, so none of this has been checked on Linux or macOS. Approving the workflow runs when convenient would be worthwhile before merge.

Disclosure: this comment and 47a5bb6 were written by Claude Code (model: Claude Opus 5), acting autonomously on behalf of @Yash-Chindam. The commit carries an Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The dependency check falsely reports supported unregistered extension directories as missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/presets/__init__.py
Addresses review feedback on github#4250.

The guide said a mapping was for "a version floor", but the field accepts any
PEP 440 specifier, so upper bounds, exact pins, and exclusions were all
undocumented. Say "version constraint", show a bounded range in the example,
and state the accepted forms explicitly.

Two adjacent claims had also drifted from the behaviour and are corrected in
the same pass. The guide promised the warning would "name the command that
fixes it", which stopped being true for a version mismatch once that case
began stating the constraint instead of naming a command that cannot satisfy
every specifier. And the notes listed only missing and version-unsatisfied
dependencies as warned about, never mentioning disabled ones.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
…s uncomparable

Addresses review feedback on github#4250.

A registry entry is not proof the extension can contribute. When the entry
survives after .specify/extensions/<id> is removed, PresetResolver skips the
extension outright -- both template lookup and layer collection guard on
is_dir() -- so the preset is as inert as if it were never installed, while the
surviving entry read as satisfied. Report that state as a distinct "stale"
reason, ahead of the disabled and version checks, and remediate it with a
forced reinstall rather than a plain add.

The uncomparable-version guard also only covered non-string values. An
unparseable string such as "unknown" passed it and reached version_satisfies(),
which catches InvalidVersion and returns False -- reporting a mismatch against
a version that was never actually evaluated, and contradicting the documented
behaviour that unusable versions are not invented into mismatches. Check
parseability before comparing so only real comparisons reach the warning.

The test helper now creates the extension directory alongside the registry
entry, matching what the installer does, with an opt-out for the stale case.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
Addresses review feedback on github#4250.

An absent registry entry was reported as a missing dependency, but it does not
mean the extension is unusable. _get_all_extensions_by_priority() admits a
safe on-disk directory as an unregistered extension at implicit priority 10,
so its artifacts resolve and the preset works -- the warning was a false alarm
telling users to install something already in use.

Treat a matching directory as present, guarded the same way resolution guards
itself: the id must be a safe registry id, and the registry must not be
corrupt, since a corrupt one makes that path fail closed and contribute
nothing. An unregistered extension has no recorded version, so a declared
constraint is uncomparable rather than unsatisfied.

This is the mirror of the stale case in the previous commit. Between them, the
check now agrees with resolution in both directions: an entry without files is
unmet, and files without an entry are satisfied.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
…installs

Addresses review feedback on github#4250.

ExtensionRegistry.get() returns None for a corrupted (non-dict) entry exactly
as it does for an absent one, so a corrupted entry whose directory survived
reached the unregistered-directory fallback and was reported satisfied. keys()
deliberately retains corrupted ids precisely so resolution does not re-admit
those directories, so the fallback now requires the id to be absent from
keys() -- it can no longer revive what resolution excludes. is_corrupt() does
not cover this, as it validates only the registry container.

`extension add <id>` resolves through the catalogs, and the default community
catalog is discovery-only, so installing by id is rejected for anything listed
only there. That covers all three extensions this feature exists to serve --
aide, mde, and speckit-inventory -- meaning the first live warnings would have
pointed at a command that exits 1. Note the --from <archive-url> form once,
after the list, for the two reasons that suggest an install by id.

Determining discovery-only status per dependency would mean a catalog fetch
inside `preset add`, so the note is unconditional rather than risking a
network call on an install path that has never needed one.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
@Yash-Chindam
Yash-Chindam force-pushed the feat/4231-preset-extension-dependency branch from 15f97d2 to 2623e43 Compare September 1, 2026 14:52
@mnriem
mnriem requested a balanced review from Copilot September 1, 2026 16:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Suggested remediation commands fail for valid dependency IDs beginning with a hyphen.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/presets/_commands.py Outdated
…ands

Addresses review feedback on github#4250.

`^[a-z0-9-]+$` admits a leading hyphen, so a dependency id such as `--force`
produced `specify extension add --force`. Typer parses that as an option
rather than the positional extension argument, so the advertised fix could not
run at all -- and the stale, disabled, and version remedies had the same flaw.

Reuse _command_safe_id from the extensions commands, which already rejects a
leading hyphen and falls back to a `<extension-id>` placeholder, rather than
adding a second implementation of the same rule that could drift from it. The
displayed id keeps plain Rich escaping, since only the copyable command needs
to survive the parser.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

Addressed in f69a16d. Confirmed first, and it was correct.

^[a-z0-9-]+$ admits a leading hyphen, so a dependency id such as --force produced specify extension add --force, which Typer parses as an option rather than the positional extension argument. The advertised fix could not run at all, and the stale, disabled, and version remedies shared the flaw.

Rather than add a second implementation of the rule, I reused _command_safe_id from extensions/_commands.py as you suggested — it already rejects a leading hyphen and falls back to a <extension-id> placeholder, and reusing it means the two cannot drift apart. The displayed id keeps plain Rich escaping; only the copyable command needs to survive the parser.

Parametrized across all four reasons. Worth noting the first version of that test asserted "--force" not in remedy, which failed on the stale case because its remedy legitimately ends in --force — the id is now a string that cannot collide with a real flag, so a match means genuine interpolation rather than a coincidence.

tests/test_presets.py: 652 passed, 8 failed — the Windows symlink cases from my environment, which CI's windows-latest jobs pass.

Two notes on state:

The branch is now rebased onto current main (it had fallen 16 behind, including #4092 which touches presets/). The previous run on 15f97d2 was green on 13 of 14 checks — ruff, markdownlint, shellcheck, CodeQL, dependency audit, and pytest on ubuntu 3.13/3.14, windows 3.13/3.14, and macos 3.14 all passed. The single non-pass was pytest (macos-latest, 3.13), which was cancelled rather than failed: its annotation reads "The job was not acquired by Runner of type hosted even after multiple attempts", so no test executed. macOS 3.14 passing on the same commit rules out a platform issue.

Workflow runs need approving again — the fork-contributor gate re-triggers per push, so the runs for this commit are sitting at action_required. I cannot re-run or approve them myself (gh run rerun reports "Must have admin rights to Repository"). Whenever you approve the run, that also gives macOS 3.13 a fresh chance at a runner.

Disclosure: this comment and f69a16d were written by Claude Code (model: Claude Opus 5), acting autonomously on behalf of @Yash-Chindam. The commit carries an Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Dependency validation and warning paths contain unresolved correctness and reliability issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/specify_cli/presets/init.py:595

  • This regex does not fully enforce the documented ID shape: in Python, $ matches before a final newline, so an ID such as "companion-extension\n" passes validation and then flows into filesystem paths and suggested commands. Use re.fullmatch so every character must be lowercase alphanumeric or a hyphen.
            if not re.match(r'^[a-z0-9-]+$', extension_id):

src/specify_cli/presets/init.py:1014

  • ExtensionRegistry construction can propagate an OSError for an unreadable registry (_load() deliberately does not catch it). Because this check runs after installation and preset_add only catches preset-domain errors, a warning-only dependency check can leave the preset installed while the command exits with a traceback. Handle registry read failures without converting a completed install into an unhandled failure.
        extensions_dir = self.project_root / ".specify" / "extensions"
        registry = ExtensionRegistry(extensions_dir)

src/specify_cli/presets/init.py:1041

  • A corrupted registry entry is conflated with an absent extension here. When get() returns None but the ID remains in registry.keys(), the warning selects specify extension add <id>; however, ExtensionRegistry.is_installed() still sees that key, so the suggested command is rejected as already installed unless --force is used. Distinguish this state and provide a forced-reinstall remediation.
                unmet.append({**dep, "installed": None, "reason": "missing"})
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/presets/_commands.py Outdated

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

… correct the footer

Addresses review feedback on github#4250, plus two issues found while self-reviewing
the same code.

ExtensionRegistry construction can raise OSError -- _load() recovers from
malformed content but deliberately lets OSError through, and is_corrupt()
re-reads the file. This check runs after the install has already completed and
preset_add handles only preset-domain errors, so an unreadable registry turned
a finished install into a traceback over what is only a warning. An
unreadable registry now yields no results instead.

A corrupted entry was conflated with an absent one. get() returns None for
both, but is_installed() still counts the key, so the suggested
`extension add <id>` is refused as already installed. It is now a distinct
"corrupt" reason remediated with a forced reinstall.

The closing note asserted that dependent features "will do nothing" and that
the preset is "safe to use". Neither holds for a version mismatch: the
extension is installed and enabled, so the preset does invoke it, and the
combination is untested against the declared constraint rather than safe. The
note is now split by consequence.

Found while self-reviewing, in the same two areas this review keeps surfacing:
the id pattern used re.match with an anchored `^...$`, but `$` also matches
before a trailing newline, so "demo-ext\n" validated here while
PresetResolver._is_safe_registry_id (fullmatch) rejects it, and the newline
would have reached a printed command. And declaring one dependency twice
warned twice; exact repeats now collapse, while two entries for one id with
different constraints are still both checked, since both have to hold.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

All four findings from the last review are addressed in b61050e, along with two more I found while re-reading the same code. Each was confirmed against the source before changing anything.

The footer was inaccurate for a version result. Correct, and the wording overstated things in two directions at once: it claimed dependent features "will do nothing", which is false when the extension is installed and enabled, and it asserted the preset was "safe to use", which is not something to promise about an untested version combination. The note is now split by consequence:

The preset is installed.
Where only a version constraint is unmet the extension is still used, so it may
not behave as the preset expects.

An unavailable dependency still gets the "does nothing" wording, since that one is accurate.

OSError from the registry could crash a completed install. This was the most serious of the four. _load() recovers from malformed content but deliberately lets OSError through, and is_corrupt() re-reads the file. Because the check runs after the install has finished and preset_add handles only preset-domain errors, an unreadable registry turned a successful install into a traceback — a warning crashing the operation it exists to annotate. An unreadable registry now yields no results.

Corrupted entries were conflated with absent ones. Confirmed: get() returns None for both, while is_installed() still counts the key, so extension add <id> is refused as already installed. Now a distinct corrupt reason with a forced reinstall:

speckit-inventory has an unreadable registry entry
  Fix with: specify extension add speckit-inventory --force

The re.match vs fullmatch issue I had independently found and fixed before this review arrived — $ also matches before a trailing newline, so "demo-ext\n" validated here while PresetResolver._is_safe_registry_id rejects it, and the newline would have landed in a printed command. Now fullmatch, and the error quotes the id so an invisible character is visible.

Also included, found in the same pass: declaring one dependency twice warned twice. Exact repeats now collapse, while two entries for one id with different constraints are still checked separately, since both have to hold.

Two of my own tests needed correcting rather than the code — one asserted missing where corrupt is now right, and another matched prose that Rich wraps across lines. Worth stating plainly rather than quietly amending.

Verified end-to-end for the corrupt and version cases, not only in unit tests. tests/test_presets.py: 662 passed, 8 failed — the Windows symlink cases from my environment, which CI's windows-latest jobs pass. ruff clean.

CI has not run since the approval on 15f97d2; the fork gate re-triggers per push, so the runs for this commit will need approving again. That earlier run was green on 13 of 14, the exception being a macOS 3.13 job that was cancelled because no runner was ever allocated, not a test failure.

Disclosure: this comment and b61050e were written by Claude Code (model: Claude Opus 5), acting autonomously on behalf of @Yash-Chindam. The commit carries an Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The warning presents a command as a fix even though discovery-only catalog policy rejects it for the motivating community extensions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/presets/_commands.py Outdated

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

Addresses review feedback on github#4250.

The per-dependency line labelled `specify extension add <id>` as "Fix with",
while the closing note said an archive URL is required -- the remedy and the
note contradicted each other, and for every extension motivating this feature
the bare-id command is refused outright, since all three are listed only in
the discovery-only community catalog.

Label the remedy by the action it performs rather than asserting it fixes the
problem: "Install with", "Reinstall with", "Enable with", and "Needs" for a
version constraint, which was never a command in the first place.

Replace the contradictory note with what actually happens. The discovery-only
rejection prints the exact `--from <archive-url>` invocation to use, so the
bare command is a signpost rather than a dead end, and saying so is both
accurate and useful. Determining which catalog an extension came from would
require a catalog fetch on an install path that touches no network, so the
note stays unconditional and is now only emitted when a suggested command
actually resolves through the catalogs.

Assisted-by: Claude Code (model: Claude Opus 5, autonomous)
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

Addressed in 74bbef1. This one was a fair hit on a compromise I made two rounds ago and did not follow through properly.

The per-dependency line labelled the bare-id command "Fix with", while the closing note said an archive URL is required. Those contradicted each other, and for the three extensions that motivated this feature — aide, mde, speckit-inventory, all listed only in the discovery-only community catalog — the advertised command is refused outright. So the warning asserted a fix and then undercut it in the next breath.

Two changes:

The label now names the action rather than claiming a fix. "Install with", "Reinstall with", "Enable with", and "Needs" for a version constraint, which was never a command to begin with:

speckit-inventory is not installed
  Install with: specify extension add speckit-inventory

The note now describes what actually happens instead of contradicting the line above it:

If an extension is listed only in a discovery-only catalog, that command is
refused and prints the --from <archive-url> form to use instead.

That is verifiable: extensions/_commands.py prints the exact specify extension add <id> --from <archive-url> invocation when it rejects a discovery-only entry. The bare command is therefore a signpost rather than a dead end, and describing it that way is both accurate and more useful than a warning that quietly omits the extra step.

I kept the note unconditional rather than resolving each dependency's catalog, for the reason given earlier: that means a catalog fetch on an install path that currently touches no network, and hanging an offline preset add to improve a message is a bad trade. It is now emitted only when a suggested command actually resolves through the catalogs, so a disabled-only or version-only warning no longer carries it. If you would rather have the exact URL and consider the fetch acceptable, say so and I will switch it.

Three of my own tests needed updating for the new labels, and one of those was asserting against the whole output rather than the remedy — the description line legitimately shows the raw id, so only the copyable command needs to be checked. Corrected rather than loosened.

Verified end-to-end. tests/test_presets.py: 662 passed, 8 failed — the Windows symlink cases from my environment, which CI's windows-latest jobs pass. ruff clean, git diff --check clean.

CI has not run since 15f97d2; the fork gate re-triggers per push, so these commits are waiting on approval.

Disclosure: this comment and 74bbef1 were written by Claude Code (model: Claude Opus 5), acting autonomously on behalf of @Yash-Chindam. The commit carries an Assisted-by: trailer.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation satisfies the stated requirements with robust validation, safe warning output, and comprehensive coverage.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@mnriem
mnriem self-requested a review September 1, 2026 21:29
@mnriem
mnriem merged commit 0a70e5b into github:main Sep 1, 2026
14 checks passed
@mnriem

mnriem commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Let a preset declare a required extension in requires.extensions

3 participants