feat: add preset update command - #4441
Open
digimangos wants to merge 12 commits into
Open
Conversation
25 tasks
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in source provenance, staged-state handling, reconciliation, cleanup, and bulk failure isolation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 8
New issues introduced by this change (9)
| Severity | Finding |
|---|---|
 |
src/specify_cli/presets/__init__.py — The staged tree is validated, but its returned manifest and diff are discarded. If a --dev source… |
|
src/specify_cli/presets/__init__.py — For a changed command entry, item.get("new") or item.get("old") collects only the new aliases. If… |
|
src/specify_cli/presets/__init__.py — This detects only manifest-declared constitution layers. Presets also support convention-only… |
|
src/specify_cli/presets/_commands.py — If write_bytes(), archive detection, or suffix renaming fails, this helper raises without… |
|
src/specify_cli/presets/_commands.py — Bulk auto-resolution checks only whether the ID exists in the current catalog, not whether this… |
|
src/specify_cli/presets/_commands.py — Bundled presets intentionally store None in catalog_archives, so cancelling a bulk update… |
|
src/specify_cli/presets/_commands.py — This resolves every source-less update by catalog ID, regardless of how the installed preset was… |
|
src/specify_cli/presets/_commands.py — The bulk apply loop catches only preset-domain exceptions. Filesystem failures from staging or… |
 |
docs/reference/presets.md — The linked requirements call for a documented recovery path if the process crashes between… |
What changed in this PR
Adds single and bulk preset updates from catalog, URL, and local sources.
Changes:
- Adds staged validation, manifest diffs, atomic swaps, rollback, and reconciliation.
- Adds dry-run and bulk-update support.
- Adds regression tests and documentation.
| File | Description |
|---|---|
tests/test_presets.py |
Adds preset-update regression coverage. |
src/specify_cli/presets/_commands.py |
Implements update commands, source resolution, and downloads. |
src/specify_cli/presets/__init__.py |
Implements update, swap, and reconciliation logic. |
docs/reference/presets.md |
Documents preset updates and options. |
Suppressed comments (3)
src/specify_cli/presets/init.py:4300
- Removed commands are unregistered for every recorded agent above, but this merge drops affected names only for the currently active agent. Inactive agents therefore remain falsely owned in this preset's registry; a later preset removal can delete a lower-priority command that reconciliation restored there. Remove
removed_command_namesfrom every agent while retaining changed names for historical agents.
retained = (
[name for name in names if name not in command_names]
if agent == active_agent
else list(names)
)
src/specify_cli/presets/init.py:4354
- The same stale-ownership problem occurs for skills: removed skills are deleted for every recorded agent, but inactive-agent entries are copied unchanged into the updated preset's metadata. A later removal can then delete a restored skill owned by another layer. Filter
removed_skill_namesglobally and filter the remaining affected names only for the active agent.
[
name
for name in names
if name not in affected_skill_names
]
src/specify_cli/presets/init.py:4406
- The warning states that reconciliation failed but gives no recovery action, even though the linked acceptance criterion requires an actionable warning after a successful swap. Explain that generated command/skill/constitution files may be stale and provide the supported command or source-based update procedure for repairing them.
warnings.warn(
f"Preset '{target_id}' was swapped, but post-update "
f"reconciliation failed: {exc}",
stacklevel=2,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Author
|
Posted on behalf of @digimangos by GitHub Copilot (model: gpt-5.6-luna). Addressed the valid review findings in commit Validation: 7,715 tests passed, 16 skipped; package build passed. The repository-wide Ruff baseline remains unchanged. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical version validation and multiple source, dry-run, cleanup, and reconciliation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (5)
| Severity | Finding |
|---|---|
 |
src/specify_cli/presets/_commands.py — This compares only the catalogue metadata version; the downloaded/bundled manifest is never… |
|
src/specify_cli/presets/__init__.py — Including aliases here sends each alias through _reconcile_composed_commands, but… |
|
src/specify_cli/presets/__init__.py — These conditions explicitly include symlinks, but shutil.rmtree() refuses to operate on a… |
|
src/specify_cli/presets/__init__.py — Legacy flat-list skill provenance is not normalized here. _register_skills may migrate it in the… |
|
src/specify_cli/presets/__init__.py — Changed commands and skills have already been written by _register_commands and… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — This resolves every source-less update by catalog ID, regardless of how the installed preset was… View comment |
|
src/specify_cli/presets/_commands.py — Bulk auto-resolution checks only whether the ID exists in the current catalog, not whether this… View comment |
Issues resolved since last review (7)
| Severity | Finding |
|---|---|
|
docs/reference/presets.md — The linked requirements call for a documented recovery path if the process crashes between… View resolved comment |
|
src/specify_cli/presets/_commands.py — The bulk apply loop catches only preset-domain exceptions. Filesystem failures from staging or… View resolved comment |
|
src/specify_cli/presets/_commands.py — Bundled presets intentionally store None in catalog_archives, so cancelling a bulk update… View resolved comment |
|
src/specify_cli/presets/_commands.py — If write_bytes(), archive detection, or suffix renaming fails, this helper raises without… View resolved comment |
|
src/specify_cli/presets/__init__.py — This detects only manifest-declared constitution layers. Presets also support convention-only… View resolved comment |
|
src/specify_cli/presets/__init__.py — For a changed command entry, item.get("new") or item.get("old") collects only the new aliases. If… View resolved comment |
|
src/specify_cli/presets/__init__.py — The staged tree is validated, but its returned manifest and diff are discarded. If a --dev source… View resolved comment |
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/_commands.py:567
- During a dry run,
download_pack()defaults to.specify/presets/.cache/downloads, and catalogue lookup can also refresh files under.cache; unlinking the archive later still leaves cache files/directories behind. This contradicts the linked acceptance criterion that--dry-runmodify no file or directory. Use a non-project temporary download/cache location or a no-write catalogue mode for dry runs.
This issue also appears on line 680 of the same file.
src/specify_cli/presets/_commands.py:722
- For dry runs this reports
constitution reconciledwhenever there is no unchanged manifest entry namedconstitution-template. Ordinary presets with no constitution layer—and unchanged convention-only constitution layers—therefore get a false reconciliation status. Determine whether the old and new resolved constitution layers/content differ; absence of a constitution layer should report unchanged.
docs/reference/presets.md:51
- The command does not enforce reuse of the same local directory or archive URL: it accepts any explicit same-ID source, and automatic catalogue resolution is by ID because no source provenance is stored. This wording therefore gives users an incorrect constraint. Describe the actual rule: presets absent from an install-allowed catalogue require an explicit
--devor--fromsource.
Catalogue updates are resolved by preset ID and only use catalogues that allow
installation. A preset installed from a local directory or an archive URL must
be updated with the same `--dev <path>` or `--from <url>` source.
src/specify_cli/presets/_commands.py:533
- Bulk auto-update resolves every installed preset solely by matching its ID in the current catalogue. Installation currently records
source: localfor every source (src/specify_cli/presets/__init__.py:3980-3983), so a preset originally installed via--devor--fromcan be silently replaced by unrelated catalogue content when its ID happens to match. This contradicts #4427’s catalogue-only eligibility requirement; persist and consult source provenance, or require an explicit source when verified catalogue provenance is unavailable.
pack_info = catalog.get_pack_info(item_id)
src/specify_cli/presets/_commands.py:647
- The single-preset path also treats any matching catalogue ID as a re-resolvable source, without checking how the installed preset was obtained. A local/URL installation sharing that ID is therefore replaced from the catalogue instead of producing the documented “supply --from/--dev explicitly” error. Gate this lookup on durable catalogue-source provenance.
pack_info = catalog_candidates.get(item_id) or catalog.get_pack_info(item_id)
src/specify_cli/presets/_commands.py:680
- The single-preset catalogue path also writes through
download_pack()during--dry-run, leaving project cache files/directories despite promising no changes. Route both catalogue refreshes and downloaded archives to a non-project/no-write path whendry_runis set.
archive_path = catalog.download_pack(item_id)
src/specify_cli/presets/_commands.py:662
- The single-update path likewise trusts only
pack_info["version"];update_from_directory/archivevalidates ID and compatibility but does not ensure the staged manifest equals the advertised version or remains newer than the installed version. A mismatched catalogue payload can silently downgrade the installation. Enforce the expected catalogue version during staged validation.
catalog_version = pkg_version.Version(str(pack_info["version"]))
except (KeyError, TypeError, ValueError) as exc:
raise PresetError(
f"catalog entry for preset '{item_id}' has an invalid version"
) from exc
Author
|
Posted on behalf of @digimangos by GitHub Copilot (model: gpt-5.6-luna). Follow-up review fixes are in commit Validation: targeted preset tests 691 passed; full suite 7,715 passed, 16 skipped; package build passed. The existing repository-wide Ruff baseline remains unchanged. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple moderate correctness issues remain in dry-run validation, caching, and artifact reconciliation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 8
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — The dry-run return bypasses the expected_version check below. If a catalogue advertises v2 but… |
|
src/specify_cli/presets/__init__.py — When --priority changes the registry priority, this set still contains only… |
|
src/specify_cli/presets/_commands.py — This deletes the shared dry-run cache while the bulk preflight loop still needs it. Every later… |
|
src/specify_cli/presets/_commands.py — The dry-run constitution status only inspects manifest entries, but convention-only… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Changed commands and skills have already been written by _register_commands and… View comment |
|
src/specify_cli/presets/__init__.py — Including aliases here sends each alias through _reconcile_composed_commands, but… View comment |
|
src/specify_cli/presets/_commands.py — This resolves every source-less update by catalog ID, regardless of how the installed preset was… View comment |
|
src/specify_cli/presets/_commands.py — Bulk auto-resolution checks only whether the ID exists in the current catalog, not whether this… View comment |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Legacy flat-list skill provenance is not normalized here. _register_skills may migrate it in the… View resolved comment |
|
src/specify_cli/presets/__init__.py — These conditions explicitly include symlinks, but shutil.rmtree() refuses to operate on a… View resolved comment |
|
src/specify_cli/presets/_commands.py — This compares only the catalogue metadata version; the downloaded/bundled manifest is never… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:4402
- For a disabled target, reconciliation excludes that preset via
list_by_priority(), so these calls can overwrite the newly registered command/skill with a lower layer. This conflicts with the existing CLI contract that disabled presets keep registered commands/skills active until removal (_commands.py:1145) and means updating a disabled preset may not update its generated artifacts. Reconcile with the updated target included for this case, or consistently change the disable semantics.
docs/reference/presets.md:51
- This source requirement does not match the implementation or the clarified acceptance criteria: automatic updates look up any installed preset by ID, regardless of whether it was originally installed via
--devor--from. Describe the actual fallback condition—an explicit source is required only when the ID cannot be resolved from an install-enabled catalogue.
Updates one installed preset, or all installed presets when no ID is given.
Catalogue updates are resolved by preset ID and only use catalogues that allow
installation. A preset installed from a local directory or an archive URL must
be updated with the same `--dev <path>` or `--from <url>` source.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple moderate reconciliation, reporting, and bulk-isolation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 6
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — include_disabled_id changes layer resolution, but the ownership list later still comes from… |
|
src/specify_cli/presets/__init__.py — The affected-name set is expanded with every alias before priority changes add the remaining… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The dry-run constitution status only inspects manifest entries, but convention-only… View comment |
|
src/specify_cli/presets/__init__.py — Changed commands and skills have already been written by _register_commands and… View comment |
|
src/specify_cli/presets/_commands.py — This resolves every source-less update by catalog ID, regardless of how the installed preset was… View comment |
|
src/specify_cli/presets/_commands.py — Bulk auto-resolution checks only whether the ID exists in the current catalog, not whether this… View comment |
Issues resolved since last review (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — This deletes the shared dry-run cache while the bulk preflight loop still needs it. Every later… View resolved comment |
|
src/specify_cli/presets/__init__.py — When --priority changes the registry priority, this set still contains only… View resolved comment |
|
src/specify_cli/presets/__init__.py — The dry-run return bypasses the expected_version check below. If a catalogue advertises v2 but… View resolved comment |
|
src/specify_cli/presets/__init__.py — Including aliases here sends each alias through _reconcile_composed_commands, but… View resolved comment |
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/_commands.py:724
- A successful update never runs the existing unmet-extension dependency warning. If the new manifest adds or tightens an extension requirement, the preset is installed but its dependent behavior can silently become inert or unsupported;
preset addcalls this warning for the same resulting state. Invoke it after non-dry-run updates.
docs/reference/presets.md:51 - This states that all
--dev/--frominstallations must retain that source, but update resolution intentionally mirrors extension update and looks up by preset ID without persisted provenance. Therefore a locally installed preset whose ID exists in an allowed catalogue can be updated from that catalogue. Describe the actual “catalogue entry resolvable by ID” rule instead.
src/specify_cli/presets/init.py:2519
- The resolver includes the disabled preset, but
presets_by_prioritybelow excludes it. When that preset owns the winning skill layer, reconciliation classifies it as a non-preset source and restores core/extension content, undoing the skill written for the updated disabled preset. Derive the owner list from the same resolver.
resolver = PresetResolver(
self.project_root, include_disabled_id=include_disabled_id
)
src/specify_cli/presets/_commands.py:738
- This dry-run prediction treats only a manifest-declared
constitution-templatediff as a reconciliation. Convention-only constitution files are supported and tested below, so changingtemplates/constitution-template.mdwithout declaring it reports “constitution unchanged” even though the real update rewrites it; a priority-only change can likewise change the winner. Compute the projected resolved constitution (including the requested priority), or avoid claiming it is unchanged.
constitution_unchanged = (
not any(
entry["identity"] == ("constitution-template", "template")
for category in ("added", "removed", "changed")
for entry in diff[category]
src/specify_cli/presets/_commands.py:745
- The normal success summary omits changed commands, so an update that only changes command content, strategy, or aliases reports
+0 commands, -0 commandsdespite having reconciled a command. Include the changed-command count so the advertised manifest diff is accurate outside--dry-run.
f"(+{added_commands} commands, -{removed_commands} commands, "
src/specify_cli/presets/_commands.py:602
- Bulk preflight does not catch
OSError, even though_diff_preset_template_files()reads referenced files and can raise it. A single unreadable preset therefore aborts the entire bulk command before the remaining IDs are processed, contrary to the per-item isolation contract; the execution loop already handles this exception per item.
except (PresetValidationError, PresetError) as exc:
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved moderate issues affect provenance, validation, failure handling, bulk isolation, and dry-run accuracy.
Review tier: Balanced
Findings: 4
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The dry-run constitution status only inspects manifest entries, but convention-only… View comment |
|
src/specify_cli/presets/__init__.py — Changed commands and skills have already been written by _register_commands and… View comment |
|
src/specify_cli/presets/_commands.py — This resolves every source-less update by catalog ID, regardless of how the installed preset was… View comment |
|
src/specify_cli/presets/_commands.py — Bulk auto-resolution checks only whether the ID exists in the current catalog, not whether this… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — The affected-name set is expanded with every alias before priority changes add the remaining… View resolved comment |
|
src/specify_cli/presets/__init__.py — include_disabled_id changes layer resolution, but the ownership list later still comes from… View resolved comment |
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:4288
PresetManifestdoes not validate thealiasescontainer, and the registrar explicitly acceptsaliases: nullas no aliases. This iteration runs after the directory and registry swap and outside the reconciliation warning handler, so such an incoming manifest raisesTypeErrorafter the update has already committed, producing a traceback without the promised actionable warning. Validate or normalize aliases during pre-swap validation and use the normalized list throughout reconciliation.
This issue also appears on line 4306 of the same file.
src/specify_cli/presets/_commands.py:769
- A compatibility failure during a single-preset update is recorded as
skipped, so the final failure check does not raise and the CLI exits successfully even for an explicitly supplied incompatible--dev/--fromsource. Keep the bulk skip behavior, but treat this as a failure for single updates, consistent withpreset add.
src/specify_cli/presets/init.py:4314
- The reconciliation set is captured before aliases from changed entries are added below. Registration or unregistration can therefore overwrite/delete an alias output, while only the primary name is re-resolved; if another preset, extension, or core command should win that alias, the generated file remains stale or missing. Build the reconciliation set after collecting affected aliases and include the expanded primary set.
reconcile_command_names = set(primary_command_names)
for item in diff["added"] + diff["removed"] + diff["changed"]:
for template in (item.get("old"), item.get("new")):
if template and template.get("type") == "command":
command_names.update(
alias
for alias in template.get("aliases", [])
if isinstance(alias, str)
)
src/specify_cli/presets/_commands.py:661
- No-source updates select a catalog entry solely by preset ID, without verifying that the installed preset came from that catalog. All current install paths record
source: "local"(src/specify_cli/presets/init.py:3993-4000), so a preset installed via--devor--fromcan be silently replaced by an unrelated catalog entry with the same ID. This contradicts docs/reference/presets.md:49-51 and the linked issue's catalog-eligibility requirement. Persist catalog provenance during installation and require it here; otherwise require an explicit source.
pack_info = catalog_candidates.get(item_id) or catalog.get_pack_info(item_id)
src/specify_cli/presets/_commands.py:741
- Dry-run constitution status is inferred only from manifest diff entries. A convention-only
templates/constitution-template.mdis intentionally supported by this update path, but it has no manifest identity, so changing its content still printsconstitution unchangedeven though the real update reconciles it. Compare the old and staged conventional constitution layers (or include them in the diff) before reporting this status.
constitution_unchanged = (
not any(
entry["identity"] == ("constitution-template", "template")
for category in ("added", "removed", "changed")
for entry in diff[category]
)
if dry_run
else constitution_before == constitution_after
src/specify_cli/presets/_commands.py:602
- Bulk preflight does not catch
OSError, although the execution loop does. An unreadable installed template or filesystem error from dry-run validation therefore aborts the entire bulk command instead of recording this preset as failed and continuing with the remaining presets, violating per-preset failure isolation.
except (PresetValidationError, PresetError) as exc:
src/specify_cli/presets/_commands.py:724
- Successful updates omit the unmet-extension dependency warning that every
preset addpath emits atsrc/specify_cli/presets/_commands.py:426-430. If a new preset version introducesrequires.extensions, its overrides can silently become inert after update. Invoke_warn_unmet_extension_dependencies(manager, manifest)after a successful non-dry-run update.
action = "would update" if dry_run else "updated"
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in alias validation and reconciliation, version handling, dry-run accuracy, and update reporting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 5
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Alias validation stops at the value type, so an incoming command alias such as ../escape passes… |
|
src/specify_cli/presets/__init__.py — Aliases from every command in both manifests are added here, even when their template is unchanged.… |
|
src/specify_cli/presets/__init__.py — The final stack reconciliation drops aliases because it uses only primary_command_names.… |
|
src/specify_cli/presets/_commands.py — For a dry run with --priority, this reports the constitution as unchanged solely from the… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Changed commands and skills have already been written by _register_commands and… View comment |
Issues resolved since last review (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — include_disabled_id changes layer resolution, but the ownership list later still comes from… View resolved comment |
|
src/specify_cli/presets/_commands.py — The dry-run constitution status only inspects manifest entries, but convention-only… View resolved comment |
|
src/specify_cli/presets/_commands.py — This resolves every source-less update by catalog ID, regardless of how the installed preset was… View resolved comment |
|
src/specify_cli/presets/_commands.py — Bulk auto-resolution checks only whether the ID exists in the current catalog, not whether this… View resolved comment |
Suppressed comments (8)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:4230
- This validates the catalogue version by raw string equality even though update eligibility uses PEP 440 versions. Equivalent versions such as catalogue
2.0and manifest2.0.0compare equal under PEP 440 but are rejected here. Compare parsedVersionvalues so valid catalogue releases are not blocked.
This issue also appears on line 4260 of the same file.
src/specify_cli/presets/_commands.py:730
- The normal success summary counts only added and removed commands, so a content-, strategy-, or alias-only command update reports
+0 commands, -0 commandseven though the manifest diff contains changed commands. Include a changed-command count in the success line so the advertised diff summary accurately reports these updates.
docs/reference/presets.md:58
- The option table omits the public
--allflag even though the command exposes it and the surrounding text discusses bulk updates. Add it so users can discover the explicit bulk form from the reference.
resolved through an installation-enabled catalogue, provide an explicit
`--dev <path>` or `--from <url>` source.
This means a preset installed from a catalogue can later be updated from the
catalogue entry currently associated with its ID, while a development or
one-off archive installation remains updateable only when an explicit source
docs/reference/presets.md:51
- This source-provenance claim does not match the implementation or the clarified acceptance criteria: automatic updates use catalogue lookup by ID only, and every install path currently records
source: local. Therefore a preset installed via--devor--fromwhose ID exists in a catalogue can be updated from that catalogue without repeating its original source. Document ID-based eligibility instead.
Catalogue updates are resolved by preset ID alone. Spec Kit searches the active
catalogues for a matching entry and only uses entries from catalogues that
allow installation. It does not retain or replay the original URL or local
src/specify_cli/presets/init.py:4260
- The staged catalogue-version check repeats the raw string comparison, so PEP 440-equivalent versions (for example
2.0and2.0.0) still fail after staging even if the earlier check is normalized. Compare parsed versions here as well.
if expected_version is not None and staged_manifest.version != expected_version:
src/specify_cli/presets/init.py:160
- The new no-op write path is not covered by a regression test. Add a test that materializes identical resolved content and verifies both the constitution and its provenance sidecar are not rewritten; content equality alone would not catch the unnecessary sidecar write this change is intended to prevent.
if memory_constitution.exists() and memory_constitution.read_bytes() == content:
return "unchanged"
src/specify_cli/presets/_commands.py:528
- The new bulk path has no CLI regression coverage: the added tests invoke only a single
--dev --dry-runupdate. Please cover bare/--allconfirmation, cancellation, per-item failure isolation, and disabled-state preservation; otherwise the core half of this command can regress without detection.
if bulk:
actionable_ids = []
catalog_archives = {}
for item_id in ids:
safe_id = _escape_markup(str(item_id))
tests/test_presets.py:14985
- The new bulk-update branch has no regression coverage: the added CLI test exercises only a single
--dev --dry-run. Please cover bulk preflight/confirmation, continuing after one item fails, disabled-preset preservation, and aggregate exit status; these are distinct paths from the tested manager-level single update.
def test_cli_single_dry_run_does_not_prompt(
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate alias-safety, reconciliation, single-pass behavior, and bulk-test coverage issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 5
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The bulk-update path is not exercised by the added tests: current CLI coverage only invokes a… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — For a dry run with --priority, this reports the constitution as unchanged solely from the… View comment |
|
src/specify_cli/presets/__init__.py — The final stack reconciliation drops aliases because it uses only primary_command_names.… View comment |
|
src/specify_cli/presets/__init__.py — Aliases from every command in both manifests are added here, even when their template is unchanged.… View comment |
|
src/specify_cli/presets/__init__.py — Alias validation stops at the value type, so an incoming command alias such as ../escape passes… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Changed commands and skills have already been written by _register_commands and… View resolved comment |
Suppressed comments (5)
src/specify_cli/presets/init.py:4312
command_namesis expanded with every alias in both manifests, including aliases owned by unchanged commands. Later, the active agent drops every name in this set fromregistered_commands, while_register_commands()re-adds only templates selected byprimary_command_names; unrelated aliases therefore lose registry ownership even though their files remain, so later removal cannot unregister them. Keep this set limited to affected primaries here—the changed-entry loop below already adds aliases belonging to affected templates.
command_names.update(
alias
for manifest in (old_manifest, new_manifest)
for item in manifest.templates
if item.get("type") == "command"
src/specify_cli/presets/init.py:468
- This accepts traversal/anchored aliases such as
../victimorC:foo.CommandRegistrar.register_commands()rejects those later viarelative_extension_path_violation()(src/specify_cli/agents.py:694-710), but an update reaches that check only after the preset directory and registry version have been swapped, leaving stale generated artifacts behind with only a reconciliation warning. Validate each alias with the shared path-safety helper here so malformed updates fail before the swap, matching extension alias validation atsrc/specify_cli/extensions/__init__.py:1121-1125.
if not isinstance(aliases, list) or not all(
isinstance(alias, str) for alias in aliases
):
raise PresetValidationError(
"Invalid template aliases: expected a list of strings"
src/specify_cli/presets/init.py:4365
- Removed aliases are unregistered here but never added to
reconcile_command_names, which currently contains only primary identities. If a lower-priority preset, extension, or core command provides that alias name, updating the preset deletes the generated command/skill and does not restore the surviving layer. Add removed names to the reconciliation set before the reconciliation calls.
removed_command_names = old_command_names - new_command_names
src/specify_cli/presets/init.py:4408
- For a priority-only update,
primary_command_namesis expanded aftercommand_nameswas created, so this loop produces an empty managed-skill set. Historical skill directories are then passed to_reconcile_skills()with no names and retain content from the old precedence order. Derive these names from the final reconciliation set instead.
for name in command_names:
src/specify_cli/presets/init.py:4388
- This directly writes each changed command before the later full-stack reconciliation writes the same command again;
_register_skills()and_reconcile_skills()repeat the same two-pass pattern. Besides violating #4427's retained single-pass reconciliation requirement, a lower-priority updated preset is briefly published before the resolver restores the actual winner. Use the single full-stack reconciliation pass to perform registration and derive tracking from its returned writes.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unsafe concurrent staging and multiple reconciliation correctness issues must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — These fixed per-preset staging and backup paths are unsafe across concurrent update processes. One… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The bulk-update path is not exercised by the added tests: current CLI coverage only invokes a… View comment |
|
src/specify_cli/presets/_commands.py — For a dry run with --priority, this reports the constitution as unchanged solely from the… View comment |
Issues resolved since last review (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — The final stack reconciliation drops aliases because it uses only primary_command_names.… View resolved comment |
|
src/specify_cli/presets/__init__.py — Aliases from every command in both manifests are added here, even when their template is unchanged.… View resolved comment |
|
src/specify_cli/presets/__init__.py — Alias validation stops at the value type, so an incoming command alias such as ../escape passes… View resolved comment |
|
src/specify_cli/presets/__init__.py — include_disabled_id changes layer resolution, but the ownership list later still comes from… View resolved comment |
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:4266
- The staged copy retains the source's generated
.composeddirectory. If--devpoints at (or copies) an installed preset and a command changes from wrap/prepend/append toreplace,_register_skills()still prefers the stale.composed/<name>.mdover the new command file (lines 3134-3138), leaving the skill on the old content. Remove generated composition artifacts from staging; composition reconciliation will regenerate those still needed.
docs/reference/presets.md:68
- The command exposes an explicit
--alloption, but this option table omits it even though the following paragraphs discuss bulk updates. Add it so the documented interface includes every supported update mode.
| `--dry-run` | Show the manifest diff without changing anything |
src/specify_cli/presets/init.py:4392
- This directly writes every changed command, and the later
_reconcile_composed_commands()call writes the same affected names again; skills follow the same duplicate_register_skills()/_reconcile_skills()pattern. For a lower-priority preset, the first pass can even publish the non-winning content before the second pass corrects it. This contradicts #4427's requirement that command/skill reconciliation run exactly once and the PR's claim to avoid unnecessary sidecar writes; make the single full-stack reconciliation pass perform registration and ownership updates.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
src/specify_cli/presets/init.py:4322
- Priority-only updates add every primary command to reconciliation, but omit their aliases because the alias loop below only visits manifest-diff entries. If this preset has
Aaliased asBand another preset provides primaryB, reprioritizing this preset downward first writes its raw aliasB, then reconciles onlyA; the other preset's winningBis never restored. Add aliases from all old/new command templates whenever priority changes.
if preserved_priority != original_priority:
src/specify_cli/presets/_commands.py:524
- The new bulk-update path has no CLI regression coverage: the added tests invoke only a missing ID and one single-preset
--dev --dry-runcase. Please cover--allconfirmation, already-current and incompatible skips, per-preset failure isolation/exit status, and preserved disabled/priority state so this core path cannot regress unnoticed.
if bulk:
src/specify_cli/presets/_commands.py:742
- The dry-run constitution status is inferred only from this preset's file diff, not the projected resolved state. For example, changing a constitution template while
constitution-syncis absent or the constitution is hand-edited reports “constitution reconciled” even though the real update writes nothing; conversely, a priority-only change can switch the winning constitution while this reports “unchanged.” Compute the status against the projected post-update stack without writing, or describe it as a manifest-level change rather than a reconciliation result.
constitution_unchanged = (
not diff.get("_constitution_changed", False)
and not any(
entry["identity"] == ("constitution-template", "template")
for category in ("added", "removed", "changed")
for entry in diff[category]
)
if dry_run
else constitution_before == constitution_after
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unresolved critical recursive-copy issue and moderate reconciliation defects must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — When --dev points at the project root (a case explicitly supported by the new regression test),… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Alias matching only selects the containing template; the rendering loop below still derives… View comment |
|
src/specify_cli/presets/__init__.py — This lock does not serialize against the existing preset mutators. preset set-priority, enable,… View comment |
Issues resolved since last review (6)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — --dev can legitimately point at the project root (for example, --dev . when that directory… View resolved comment |
|
src/specify_cli/presets/__init__.py — This registers changed commands against the currently active integration even when the preserved… View resolved comment |
|
src/specify_cli/presets/__init__.py — Legacy installations store registered_skills as a flat list, but this call omits… View resolved comment |
|
src/specify_cli/presets/_commands.py — The dry-run predicate does not mirror the actual reconciliation path: the update below calls… View resolved comment |
|
src/specify_cli/presets/__init__.py — Priority changes reconcile inactive command agents stack-wide, but inactive skill directories are… View resolved comment |
|
src/specify_cli/presets/__init__.py — Do not exempt priority changes from the post-lock downgrade check. A single catalogue update can… View resolved comment |
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/_commands.py:632
- When every preset is already current (or all preflights fail), this early return runs before the
--priority is ignorednotice below. That contradicts the documented behavior and leavespreset update --all --priority Nappearing to accept the option silently. Emit the bulk-priority notice immediately after preflight, before this no-actionable-items branch.
src/specify_cli/presets/init.py:6201
- Alias-aware resolution is not carried through the reconciliation ownership paths.
_reconcile_composed_commandsstill identifies a preset template only withtmpl["name"] == cmd_name(lines 2193 and 2305), so reconciling an alias falls through to generic/shared registration and does not merge the written artifact into the preset'sregistered_commands. For example, adding an alias while another agent is inactive can write that alias into the historical agent directory without tracking it, leaving it behind when the preset is removed. Update those owner/filter checks to match aliases as well as the primary name.
tmpl.get("name") == template_name
or (
isinstance(aliases, list)
and template_name in aliases
)
src/specify_cli/presets/init.py:4531
- Changed commands are written here by
_register_commands, then written again by_reconcile_composed_commandsbelow; skills follow the same two-pass pattern. This violates #4427's single-pass reconciliation requirement and creates an avoidable intermediate state where a reprioritized preset's losing content is installed before the resolver overwrites it. Drive generated-file writes through one reconciliation pass and update ownership metadata from that pass instead.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
src/specify_cli/presets/_commands.py:475
- The help text says the positional argument can be
all, but the implementation treats only an omitted ID or--allas bulk mode.specify preset update alltherefore attempts to update a preset literally namedall. Remove “(or all)” from this help text, or implement the advertised positional alias.
preset_id: str = typer.Argument(None, help="Preset ID to update (or all)"),
When a development preset source is the project root, avoid copying .specify/presets into the staged preset so update staging does not include registry, cache, or other installed preset state. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The critical registry concurrency issue must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This lock does not serialize the update against other preset mutations. preset disable, enable,… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — When --dev points at the project root (a case explicitly supported by the new regression test),… View resolved comment |
|
src/specify_cli/presets/__init__.py — Alias matching only selects the containing template; the rendering loop below still derives… View resolved comment |
|
src/specify_cli/presets/__init__.py — This lock does not serialize against the existing preset mutators. preset set-priority, enable,… View resolved comment |
Suppressed comments (8)
src/specify_cli/presets/init.py:6202
- Treating an alias as an ordinary layer also applies the primary entry's composition strategy to the alias's own stack. For a command such as primary
speckit.foo, aliasspeckit.f, strategyappend, and a base that exists only forspeckit.foo,resolve_content("speckit.f")sees one append layer and returnsNone;_reconcile_composed_commandsthen unregisters the alias as stale. Alias lookup needs to resolve composition through the primary identity (or reconciliation must explicitly map aliases to the primary's composed result) rather than composing each alias independently.
aliases = tmpl.get("aliases", [])
if (
tmpl.get("type") == template_type
and (
tmpl.get("name") == template_name
or (
isinstance(aliases, list)
and template_name in aliases
)
)
src/specify_cli/presets/init.py:4779
- These cleanup calls execute after a successful swap and registry update, but
remove_swap_pathpropagatesOSError. A locked/read-only file can therefore make the CLI report the update as failed even though the new preset is already installed, contradicting the command's resulting state. Catch cleanup failures here and warn with the retained recovery path instead of turning a committed update into a failure.
finally:
remove_swap_path(backup_dir)
remove_swap_path(staging_dir)
src/specify_cli/presets/init.py:4441
- The directory and registry have already committed before this cleanup runs. If removing the backup or staging tree raises (for example, because a file is locked on Windows), the disabled-preset update is reported as failed even though the new version is live and recorded in the registry. Post-commit cleanup should be best-effort and emit a recovery warning; only pre-commit cleanup should remain fatal.
remove_swap_path(backup_dir)
remove_swap_path(staging_dir)
return new_manifest, diff
src/specify_cli/presets/init.py:4637
- Sorting these logical names can leave an alias with the wrong winner.
_reconcile_composed_commandsre-emits all aliases when it processes a primary template (lines 2192-2197); for a primary such asspeckit.specifyand aliasspeckit.spec, the alias is reconciled first and then overwritten by the primary's layer, even when a higher-priority preset or project override winsspeckit.spec. Reconcile each logical name without re-emitting its siblings, or run a final winner-aware alias pass.
self._reconcile_composed_commands(
sorted(reconcile_command_names),
extra_agents=historical_agents,
)
src/specify_cli/presets/init.py:4531
- This directly writes the changed commands, and the later
_reconcile_composed_commandscall writes the same affected names again; skills follow the same duplicate_register_skills/_reconcile_skillspattern. The linked acceptance criteria require one reconciliation pass over the added/removed/changed union, so this currently causes duplicate writes and transiently installs a non-winning layer. Consolidate registration and full-stack reconciliation into a single winner-aware pass.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
src/specify_cli/presets/init.py:4370
- For the supported case where
source_diris the project root, this ignore callback excludes only.specify/presets. The transaction has already created.specify/.workflow-install.lock, so that lock—and the rest of the project's.specifystate such as memory and extension data—is copied into the installed preset. Exclude the project-owned.specifytree when staging from the project root, or copy only the preset payload.
def ignore_staging_state(directory: str, names: List[str]) -> Set[str]:
directory_path = Path(directory)
return {
name
for name in names
if directory_path / name in (self.presets_dir, staging_dir, backup_dir)
}
src/specify_cli/presets/init.py:4441
- Returning here leaves every generated command/skill at the pre-update version.
preset_enableonly flipsenabledand reconciles the constitution (presets/_commands.py:1173-1177), so after updating a disabled preset, newly added commands never appear and changed commands remain stale even after re-enabling it. The enable path needs to reconcile the now-current manifest (or this branch must record deferred reconciliation work).
if not enabled:
# Disabled presets keep their existing generated artifacts and
# provenance until removal. Updating only replaces the source and
# metadata while retaining enough ownership data for a later
# removal to clean up artifacts left on disk.
remove_swap_path(backup_dir)
remove_swap_path(staging_dir)
return new_manifest, diff
src/specify_cli/presets/_commands.py:67
- This message is misleading for a public HTTPS redirect to an HTTPS loopback address:
is_safe_download_redirectrejects that transition, but the message says the target merely needs HTTPS. State the actual policy so users can diagnose the rejection.
def validate_redirect(old_url, new_url):
if not is_safe_download_redirect(old_url, new_url):
raise error_type(
"redirect target must use HTTPS or remain on localhost"
)
Exclude the project .specify directory when a development preset update stages from the project root, avoiding recursive preset state and Windows lock-file copy failures. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three moderate reconciliation and bundled-version issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — A disabled update returns after replacing the source while deliberately retaining the old generated… |
|
src/specify_cli/presets/_commands.py — Bundled updates assume that the copy shipped with the running Spec Kit has exactly the version… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This lock does not serialize the update against other preset mutations. preset disable, enable,… View comment |
Suppressed comments (1)
src/specify_cli/presets/init.py:4537
- The update performs two registration passes for the same affected names: this call writes the new preset's command files directly, and the same block later calls
_reconcile_composed_commands; likewise_register_skillsis followed by_reconcile_skills. For a lower-priority preset this briefly writes the wrong winner, and if the second pass fails the warning leaves that wrong output in place. It also contradicts #4427's single-pass reconciliation requirement. Route the added/removed/changed union through one stack-aware reconciliation pass and derive the ownership metadata from that result instead of registering the incoming preset first.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
Refresh generated commands and skills when a preset is re-enabled after a disabled update, removing stale artefacts before re-registering the active agent output. Use the locally bundled preset version for bundled updates and fail clearly when the installed spec-kit package lags behind the catalogue. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three moderate enable-path issues can leave registry state or generated artifacts inconsistent or stale.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — Validate the installed manifest before setting enabled. As written, a missing or corrupt… |
|
src/specify_cli/presets/_commands.py — Legacy registries store registered_skills as a flat list, but _normalize_registered_skills()… |
|
src/specify_cli/presets/_commands.py — resolve_active_agent_for_registration() returns the non-string MISSING_INIT_OPTIONS_FILE… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This lock does not serialize the update against other preset mutations. preset disable, enable,… View comment |
|
src/specify_cli/presets/__init__.py — A disabled update returns after replacing the source while deliberately retaining the old generated… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — Bundled updates assume that the copy shipped with the running Spec Kit has exactly the version… View resolved comment |
- Validate the installed preset.yml before flipping enabled=True, so a missing/corrupt manifest fails closed with the preset left disabled instead of enabling first and erroring after mutating the registry. - Fix legacy flat-list registered_skills handling: use _infer_legacy_skill_provenance when the raw registry value is a list, so stale-skill cleanup on enable correctly identifies per-agent ownership instead of silently doing nothing. - Fall back to registering commands/skills for every detected agent when resolve_active_agent_for_registration returns MISSING_INIT_OPTIONS_FILE (legacy pre-init-options projects), instead of silently skipping all rescaffolding, matching install-time behaviour elsewhere in the codebase. Adds regression tests for all three scenarios under TestPresetEnableDisable. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Six moderate correctness and transactional-safety issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The preset is persisted as enabled before the newly added unregister/register/reconcile sequence,… |
|
src/specify_cli/presets/_commands.py — This cleanup restores only extension/core content; it does not resolve a surviving lower-priority… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This lock does not serialize the update against other preset mutations. preset disable, enable,… View comment |
|
src/specify_cli/presets/__init__.py — A disabled update returns after replacing the source while deliberately retaining the old generated… View comment |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — resolve_active_agent_for_registration() returns the non-string MISSING_INIT_OPTIONS_FILE… View resolved comment |
|
src/specify_cli/presets/_commands.py — Legacy registries store registered_skills as a flat list, but _normalize_registered_skills()… View resolved comment |
|
src/specify_cli/presets/_commands.py — Validate the installed manifest before setting enabled. As written, a missing or corrupt… View resolved comment |
Suppressed comments (4)
src/specify_cli/presets/init.py:4566
- In a legacy project without
init-options.json,resolve_active_agent_for_registration()returns the truthyMISSING_INIT_OPTIONS_FILEsentinel. Passingactive_agent or ""here lets_infer_legacy_skill_provenance()use that object as a dictionary key for any unmatched legacy skill. The resulting non-string key reachesregistry.update()andjson.dump(), which raises after the live preset has already been swapped and can leave.registrytruncated. Normalize the sentinel to an empty string, aspreset_enable()already does.
registered_skills_before = self._infer_legacy_skill_provenance(
[name for name in raw_registered_skills_before if isinstance(name, str)],
target_id,
active_agent or "",
)
src/specify_cli/presets/init.py:4445
- This cleanup runs after the directory swap and registry update have committed. If removing the backup fails (for example, because a file is locked),
update_from_directoryraises and the CLI reports the preset as failed even though the new version is installed; the enabled path has the same problem in its final cleanup. Treat post-commit backup/staging cleanup as best-effort and emit an actionable warning while retaining the recovery directory instead of converting a successful update into a failure.
remove_swap_path(backup_dir)
src/specify_cli/presets/init.py:4618
- Changed commands and skills have already been written by
_register_commandsand_register_skillsabove, and these reconciliation calls write those same affected names again. That contradicts #4427's requirement that command/skill registration reconciliation run exactly once per update and creates an avoidable intermediate artifact state. Use the full-stack reconciliation pass as the sole writer and derive ownership metadata from its results.
if reconcile_command_names:
historical_agents = {
src/specify_cli/presets/_commands.py:730
- This equality check happens before resolving a bundled preset's local manifest version. Consequently, an installed v2 preset is reported up to date when the catalog still says v2 even if the current Spec Kit package bundles v3; the bulk preflight has the same ordering. Resolve bundled availability first and compare the installed version against the local bundled version so newer bundled content is not skipped.
if catalog_version == installed_version and effective_priority is None:
console.print(
f"[dim]• {safe_id}: Up to date, skipped "
f"(v{installed_version})[/dim]"
)
outcomes.append("skipped")
continue
- Snapshot the pre-enable registry entry and wrap the reconciliation body in try/except: any failure restores the registry to its prior disabled state instead of leaving the preset marked enabled with only partially refreshed artifacts. - Reorder preset_enable to unregister/reconcile stale skills before stale commands, mirroring PresetManager.remove(). Native skill-only agents (claude, codex) track the same physical SKILL.md file under both registered_commands and registered_skills; unregistering commands first (no core-fallback there) could delete that file before the skills pass ever got a chance to restore/reconcile it. - Filter stale commands already covered by the skills pass out of the _unregister_commands call for native skill-only agents, avoiding a double-delete of the same file. - Pass _unregister_skills's returned affected directories through to _reconcile_skills, so a surviving lower-priority preset's override is reconciled back in instead of left showing core/extension content. - Add _command_name_for_skill_name (inverse of _skill_names_for_command) and use it to derive command names for stale skills, so skill-only reconciliation still runs even when no commands went stale. Adds regression tests for registry rollback on reconciliation failure and for surviving lower-priority preset reconciliation across historical and active agents. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate defects remain in bundled-version selection and command/skill reconciliation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This is not an inverse for namespaced commands: _skill_names_for_command("speckit.foo.bar")… |
|
src/specify_cli/presets/__init__.py — Removed commands are unregistered before their skill registrations are restored. For native skill… |
Issues resolved since last review (4)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — This cleanup restores only extension/core content; it does not resolve a surviving lower-priority… View resolved comment |
|
src/specify_cli/presets/_commands.py — The preset is persisted as enabled before the newly added unregister/register/reconcile sequence,… View resolved comment |
|
src/specify_cli/presets/__init__.py — A disabled update returns after replacing the source while deliberately retaining the old generated… View resolved comment |
|
src/specify_cli/presets/__init__.py — This lock does not serialize the update against other preset mutations. preset disable, enable,… View resolved comment |
Suppressed comments (3)
src/specify_cli/presets/_commands.py:1412
- Only the active agent is refreshed here; the reconciliation below targets historical agents only for names that became stale. If a preset was rendered for Claude, switched to Codex, then updated while disabled with the same command name but new content, enabling it refreshes Codex while Claude keeps the old body indefinitely. Reconcile all current command/skill names across the preset's recorded historical agents, not just removed names.
elif isinstance(resolved_agent, str):
manager.register_enabled_presets_for_agent(resolved_agent)
reconcile_command_names = sorted(
{name for names in stale_commands.values() for name in names}
src/specify_cli/presets/_commands.py:579
- The bulk path decides a bundled preset is current before inspecting the bundled manifest. Thus, with installed v2, catalogue v2, and a locally shipped bundled v3, it skips v3 even though this command otherwise treats the newer local bundled version as authoritative. Resolve the bundled version first, then compare the effective available version with the installed version.
catalog_version = pkg_version.Version(str(pack_info["version"]))
if catalog_version <= installed_version and effective_priority is None:
console.print(
f"[dim]• {safe_id}: Up to date, skipped "
f"(v{installed_version})[/dim]"
src/specify_cli/presets/_commands.py:728
- This early version skip also prevents single-item bundled updates from seeing a newer local bundled copy. For installed v2/catalogue v2/local bundled v3, the command reports “Up to date” and never reaches
_bundled_update_source, despite the supported local-newer behavior. Compare against the bundled manifest's effective version before returning.
if catalog_version == installed_version and effective_priority is None:
console.print(
f"[dim]• {safe_id}: Up to date, skipped "
f"(v{installed_version})[/dim]"
)
_commands.py doesn't import typing.Dict/List; with `from __future__ import annotations` these are lazily-evaluated string annotations so ruff's F821 caught the undefined names rather than a runtime failure. Switch to builtin dict/list generics (PEP 585) instead of adding an unused-elsewhere typing import. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Moderate issues remain in bundled-version selection, registry handling, and command/skill reconciliation.
Review tier: Balanced
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Removed commands are unregistered before their skill registrations are restored. For native skill… View comment |
|
src/specify_cli/presets/__init__.py — This is not an inverse for namespaced commands: _skill_names_for_command("speckit.foo.bar")… View comment |
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/_commands.py:676
- A corrupt registry value that is not a mapping causes
"version" not in metadatato raiseTypeError, which is not caught by the single-update handler and leaks a traceback instead of the intended “registry entry is missing or corrupt” failure. The bulk preflight happens to catch this separately, so guard the type here explicitly.
src/specify_cli/presets/_commands.py:1401 - When
registered_skillsis a legacy flat list and none of its skills are stale, the earlier migration remains only in the localregistered_skillsvariable. Herecurrent_metadata["registered_skills"]is still a list, so the subsequentmerged_skills.get(...)raisesAttributeErrorand enabling an otherwise unchanged legacy preset fails. Seed the merge from the already inferred mapping when the persisted value is a list.
src/specify_cli/presets/init.py:2954
- This is not an inverse for namespaced commands:
_skill_names_for_command("speckit.fakeext.cmd")producesspeckit-fakeext-cmd, but this maps it back tospeckit.fakeext-cmd. When enable cleans a stale skill that has noregistered_commandsentry, historical skill directories rely on this recovered name; reconciliation therefore cannot find a surviving lower-priority provider of the original namespaced command. Match stale skill names against known manifest command identities instead of reversing this lossy dot-to-hyphen encoding.
if skill_name.startswith("speckit-"):
short_name = skill_name[len("speckit-"):]
elif skill_name.startswith("speckit."):
short_name = skill_name[len("speckit."):]
else:
src/specify_cli/presets/init.py:4550
- Removed commands are unregistered before their corresponding skills. For an active native
*/SKILL.mdagent, both registry maps refer to the same file, so_unregister_commandsdeletes the skill directory;_unregister_skillsthen cannot restore it, and the later active-directory reconciliation sees no managed skill names after the registry entry is cleared. Updating an enabled preset that drops a core-overriding command therefore leaves the core skill missing. Mirrorremove()/preset_enable: unregister skills first, capture affected directories, and exclude covered native-skill commands from this pass.
if stale_commands:
self._unregister_commands(stale_commands)
src/specify_cli/presets/init.py:4553
- This directly writes every changed template from the updated preset before
_reconcile_composed_commandswrites the winning layer again. When the updated preset is lower priority, the first write is known to be the wrong winner; every successful update performs duplicate writes, and a later reconciliation failure leaves that lower-priority content active. This also contradicts #4427's single-pass reconciliation requirement. Drive the active-agent write only through the winner-aware reconciliation pass and use its results to update registration metadata.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
src/specify_cli/presets/_commands.py:710
- The single-preset path has the same premature catalog-version check: an installed v2 preset is skipped when the catalog remains at v2 even if the locally bundled manifest is v3. Determine the effective bundled version before deciding the installation is current, otherwise users cannot pick up newer bundled content after upgrading Spec Kit.
if catalog_version < installed_version:
src/specify_cli/presets/_commands.py:576
- Bulk updates compare the catalog version before resolving a bundled preset's actual local version. If the catalog says v2, the installed preset is already v2, and this Spec Kit release bundles v3, this branch reports “Up to date” and never reaches
_bundled_update_source, despite the later logic intentionally preferring the newer bundled copy. Resolve bundled metadata first and compareinstalled_versionagainst that effective version.
catalog_version = pkg_version.Version(str(pack_info["version"]))
if catalog_version <= installed_version and effective_priority is None:
…n preset enable Copilot's review flagged that recovering a command name from a skill directory name by naive reversal (`_command_name_for_skill_name`) is mathematically lossy for namespaced commands: "speckit.git.feature" and "speckit.git-feature" both encode to "speckit-git-feature", so reversing the encoding can't tell them apart. Replace the reversal with forward-matching: build the set of command names this preset is known to use (current manifest + everything ever recorded in its own registered_commands), and for each stale skill, find which known command name's _skill_names_for_command(...) output actually produces it. This is unambiguous because it goes in the transform's defined (forward) direction, and is scoped to only this preset's own command universe. Removed the now-dead, buggy _command_name_for_skill_name helper. Added a regression test (namespaced speckit.git.feature command, with the affected agent in a historical/inactive-agent scenario) covering this exact case end-to-end. A related reordering change to update_from_directory (unregistering stale skills before stale commands) was evaluated but reverted: traced step-by-step, update_from_directory's own _reconcile_skills call unconditionally rebuilds every affected command name's skill content after both unregister calls run, regardless of their order, so no reachable regression exists there -- reordering would have added complexity without fixing anything. Assisted-by: GitHub Copilot (model: auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two moderate reconciliation and post-commit cleanup issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 3
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — For a command-backed integration in skills mode (for example Copilot), registered_commands is… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Removed commands are unregistered before their skill registrations are restored. For native skill… View comment |
|
src/specify_cli/presets/__init__.py — This is not an inverse for namespaced commands: _skill_names_for_command("speckit.foo.bar")… View comment |
Suppressed comments (1)
src/specify_cli/presets/init.py:4785
- If deleting the backup or staging directory fails after the swap and registry update have succeeded, this
finallyblock propagatesOSError. The CLI then reports the preset as “failed,” although the new version is already installed and deliberately cannot be rolled back at this stage. Treat post-commit cleanup as best-effort and emit an actionable warning so the outcome accurately reports the successful update and the leftover recovery directory.
finally:
remove_swap_path(backup_dir)
remove_swap_path(staging_dir)
For a command-backed integration running in explicit skills-opt-in mode (e.g. Copilot with ai_skills on), _register_commands's guard never populates registered_commands for it, and after a disabled update the stale command is also absent from the preset's own current manifest. The prior forward-match fix (8d2210c) only searched this preset's own current manifest plus its own registered_commands, so it could not recover the command name in this case: _reconcile_skills received an empty command list and no-op'd, and the skill directory was deleted outright (no core/extension fallback to restore). Broaden the candidate search in preset_enable to also scan every other installed preset's registered_commands and re-parsed preset.yml (including aliases), since the surviving lower-priority preset that should win the skill back always still lists the command in its own current manifest. Added a regression test that discriminates the fix using two command-backed, skills-opt-in agents (one historical, one active) so neither agent's registered_commands can mask the bug via the active-agent full-rescaffold path. Confirmed the test fails (skill file deleted) against the narrower fix and passes with this change. Assisted-by: GitHub Copilot (model: auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Three unresolved moderate correctness issues must be addressed before approval.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Removed commands are unregistered before their skill registrations are restored. For native skill… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — For a command-backed integration in skills mode (for example Copilot), registered_commands is… View resolved comment |
|
src/specify_cli/presets/__init__.py — This is not an inverse for namespaced commands: _skill_names_for_command("speckit.foo.bar")… View resolved comment |
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/_commands.py:784
- In bulk dry-run mode, every call here leaves the manager and registry unchanged, so each preset's prospective constitution is calculated against the original stack rather than the cumulative stack that the real bulk update will produce. For example, if a higher-priority update removes its constitution layer and a lower-priority update changes its layer, the lower update is reported as
constitution unchangedhere even though the real sequential run reconciles it after the higher layer is removed. Accumulate the preflighted sources as resolver overrides (in execution order) when computing bulk dry-run status so the preview matches the final stack.
src/specify_cli/presets/_commands.py:1387
- In a legacy project without
init-options.json, enabling this preset registers only this preset across every detected agent. Because registration writes directly, enabling a lower-priority preset can overwrite an already-enabled higher-priority preset for every shared command/skill; the later reconciliation only covers stale (removed) names, not the current names just written. Reconcilecurrent_command_namesagainst the full enabled stack after this branch, or re-register all enabled presets in reverse priority order as the active-agent path does.
if resolved_agent is MISSING_INIT_OPTIONS_FILE:
# Legacy pre-init-options project: there is no single "active
# agent" to target, so mirror install-time behaviour and
# register/re-register this preset's commands and skills for
# every agent directory actually present on disk, merging the
# fresh result into the stored registry state.
fresh_commands = manager._register_commands(manifest, pack_dir)
src/specify_cli/presets/_commands.py:1492
- Forward matching is still ambiguous for the collision described above. If the installed stack contains both
speckit.git.featureandspeckit.git-feature, both map tospeckit-git-feature; iterating a set and breaking on the first match chooses an arbitrary logical command, so enable can repopulate the stale skill with the unrelated command's content. The new regression test only includes one of the colliding names. Preserve a non-lossy command-to-skill identity in registry provenance (or reject such collisions) instead of recovering it from the encoded directory name.
for command_name in all_known_command_names:
modern, legacy = manager._skill_names_for_command(command_name)
if skill_name in (modern, legacy):
stale_skill_command_names.add(command_name)
break
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.
Summary
Description
Implements
specify preset updatefor single and bulk preset updates. To align functionality withspecify extension updatewith the addition of supporting--from, and--devupdate sources based on ID.Testing
uv run specify --helpuv sync && uv run pytestAI Disclosure
Copilot Desktop App, Copilot CLI, detailed implementation plan, (too many) rounds with copilot code review agent, simulated real world smoke testing.
Closes #4427.