feat: add preset update command - #4441
Draft
digimangos wants to merge 39 commits into
Draft
Conversation
Implement staged atomic preset updates with bulk and dry-run support, preserving registry state and reconciling manifests and constitutions safely. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the installed preset untouched and clean up partial staging when update validation or staging fails, and make missing preset errors direct users to preset add. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add the --all alias for bulk updates while preserving source-option validation and single-update semantics. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Scope command and skill registration to added, removed, and changed manifest identities, preserve unaffected registry tracking, and report constitution status from the resolved content hash. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Detect referenced template content changes in update diffs and isolate malformed catalog versions and archive cleanup. Add regression coverage for content-only changes. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add regression coverage for manifest rejection, type changes, rollback, missing IDs, and single-update dry runs. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Revalidate the copied staging directory before the atomic swap and cover rollback when staged validation fails. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reconcile historical command and skill targets during updates and reuse release-asset download handling for explicit sources. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Use the installed bundled preset directory when catalogue metadata has no download URL. Preserve archive handling for externally downloaded presets. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Preserve recorded skill ownership until removed command skills are unregistered during preset reconciliation. Assisted-by: GitHub Copilot (model: GPT-5.6 Luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
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.
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
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
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
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
Four moderate correctness and concurrency 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 — Archive classification no longer honors the response's final URL. The previous preset add --from… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This discards each historical agent's recorded names and passes every affected skill name… View comment |
|
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 |
Suppressed comments (3)
src/specify_cli/presets/init.py:4376
- Current aliases are added to the reconciliation set, but
PresetResolver._manifest_declared_template()only matches a template's primaryname(src/specify_cli/presets/__init__.py:5979-5988). If an alias shares a name with a lower preset, extension, or core command,_register_commands()first writes the updated alias and this reconciliation then resolves and overwrites it with the lower provider. Make aliases resolver-visible, or distinguish live aliases from removed aliases so only removed aliases fall through to a lower layer.
alias
for alias in template.get("aliases", [])
src/specify_cli/presets/init.py:4314
- The directory renames and registry write are not serialized. Two updaters can both validate the same old installation, then one can move the other's newly installed directory before either finishes registry update/reconciliation; both may report success while generated artifacts reflect interleaved diffs. The
FileNotFoundErrorbranch catches only the brief missing-directory window. Hold a per-project or per-preset transaction lock and recheck the installed version/hash under it before this rename;_workflow_install_transactioninsrc/specify_cli/workflows/_commands.py:685-718provides an existing cross-platform pattern.
os.replace(current_dir, backup_dir)
src/specify_cli/presets/_commands.py:758
- The dry-run constitution status does not apply the conditions used by the real reconciliation. A command-only preset with
--priorityis reported as "constitution reconciliation pending" even thoughupdate_from_directory()never calls it, while a changed constitution template is reported as planned even when_constitution_is_generated()will preserve a hand-edited file. Compute the planned outcome using the same constitution-layer and hand-edit guards so dry-run output reflects the writes an actual update would perform.
constitution_status = (
"constitution reconciliation pending"
if dry_run and priority is not None and not constitution_diff
else "constitution change planned"
if dry_run and constitution_diff
Preserve the final URL returned after redirects and classify downloaded archives from their detected bytes using the canonical suffix. This prevents a .zip request that redirects to a .tgz asset from being validated with the wrong filename format. Add a regression test covering the redirected archive case. Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Serialize preset updates with the existing project transaction lock, make resolver lookups alias-aware, preserve per-agent historical skill ownership during reconciliation, and make dry-run constitution output respect the real sync and hand-edit guards. Add regression coverage for alias resolution and command-only priority changes during dry-run updates. Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Author
|
Review round update, posted on behalf of @digimangos by GitHub Copilot (model: gpt-5.6-luna). Addressed the remaining findings:
Validation: 7,731 tests passed, 16 skipped, and |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in alias validation, skill reconciliation, dry-run reporting, and priority-only updates.
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 (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — Aliases now participate in preset resolution, but this validation does not reject an alias that… |
|
src/specify_cli/presets/_commands.py — This current-version check also bypasses an explicitly requested single-preset --priority. Thus… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This discards each historical agent's recorded names and passes every affected skill name… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — Archive classification no longer honors the response's final URL. The previous preset add --from… View resolved comment |
|
src/specify_cli/presets/_commands.py — The bulk-update path is not exercised by the added tests: current CLI coverage only invokes a… View resolved comment |
Suppressed comments (2)
src/specify_cli/presets/init.py:4575
- Historical skill directories are collected only from the preset being updated. If another preset currently owns an affected skill under an inactive agent, and this update changes priority so the updated preset becomes the winner,
_reconcile_skills()updates only the active directory and leaves that inactive agent on the old winner. The command path above already scans every other registry entry for this case; perform the equivalent stack-wideregistered_skillsscan and pass those agent directories and affected names here.
extra_skill_dirs = {}
if isinstance(registered_skills_before, dict):
for agent, names in registered_skills_before.items():
if agent == active_agent:
continue
src/specify_cli/presets/_commands.py:769
- This dry-run status reports a planned constitution change whenever this preset's constitution bytes differ, even if that layer is disabled or remains below the current winner. The real reconciliation resolves the final enabled priority stack and
_materialize_constitution_template()then skips an identical result, so the same update is reported as “change planned” in dry-run and “unchanged” when executed. Compute the prospective winning composition (including the proposed priority and enabled state) and compare its bytes with the current constitution before reporting the status.
constitution_status = (
"constitution change planned"
if constitution_can_reconcile
and (constitution_diff or priority is not None)
else "constitution unchanged"
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Author
|
Posted on behalf of @digimangos by GitHub Copilot (model: gpt-5.6-luna). Addressed the two remaining review findings in commit Validation: full repository suite passed, 7,749 tests collected with no failures; full preset suite passed, 709 tests. |
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Five unresolved moderate issues affect dry-run immutability, reconciliation correctness, alias ownership, and bulk-update skipping.
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 (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — Bulk mode deliberately sets effective_priority to None, but this predicate checks the raw… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — This discards each historical agent's recorded names and passes every affected skill name… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — This current-version check also bypasses an explicitly requested single-preset --priority. Thus… View resolved comment |
|
src/specify_cli/presets/__init__.py — Aliases now participate in preset resolution, but this validation does not reject an alias that… View resolved comment |
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:4248
- Every dry run enters
_workflow_install_transaction, whoseos.open(..., O_CREAT)creates.specify/.workflow-install.lockwhen absent. Thus--dry-runchanges the project despite the documented “without changing anything” behavior. Route the dry-run validation through a read-only path before acquiring this persistent lock, or use locking that does not leave a project artifact.
This issue also appears on line 4248 of the same file.
src/specify_cli/presets/init.py:4576
- Priority changes collect historical command agents stack-wide above, but skill reconciliation only includes directories recorded by the preset being updated. If another preset owns an affected skill in a now-inactive agent and this update changes the winner, that agent's skill remains stale. Build
extra_skill_dirsfrom affectedregistered_skillsacross the preset stack, analogous tohistorical_agentsfor commands.
src/specify_cli/presets/init.py:4248
PresetManagerloads its registry snapshot in__init__, before this lock is acquired. If two update processes construct managers concurrently, the second can wait for the first and then save its stale registry snapshot, reverting the first update's metadata while its new directory remains installed. ReloadPresetRegistryafter acquiring the lock so validation and every subsequent save use the serialized state.
with _workflow_install_transaction(self.project_root):
src/specify_cli/presets/init.py:4465
- This directly registers each changed template before
_reconcile_composed_commandsperforms full-stack resolution, so an updated lower-priority preset temporarily overwrites the actual winner and can remain visible if reconciliation fails. It also contradicts #4427's required single reconciliation pass. Make the full-stack reconciliation the sole writer and derive registry tracking from its result instead of pre-registering the incoming manifest.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
src/specify_cli/presets/init.py:6029
- Alias-aware resolution is incomplete:
_reconcile_composed_commandsstill identifies preset owners at lines 2187 and 2300 using primary-name equality only. Resolving an alias therefore falls through to generic path registration and does not merge the write into the owning preset's registry metadata, which can orphan restored historical-agent output. Update those owner lookups to match the primary name or declared aliases.
tmpl.get("type") == template_type
and (
tmpl.get("name") == template_name
or (
isinstance(aliases, list)
and template_name in aliases
)
src/specify_cli/presets/_commands.py:689
- The execution path repeats the raw
prioritycheck, so an equal-version preset reaching a bulk run with--priorityis reinstalled even though bulk priority is ignored. Checkeffective_priorityhere so only a single-preset explicit reprioritization can bypass the equal-version skip.
if catalog_version < installed_version or (
catalog_version == installed_version and priority is None
):
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Author
|
Posted on behalf of @digimangos by GitHub Copilot (model: gpt-5.6-luna). Addressed the latest bulk-update finding in commit Validation: CI-equivalent Ruff passed across |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two critical concurrency defects and four moderate dry-run, reconciliation, priority, and validation issues remain unresolved.
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 (3)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — The update lock is acquired after PresetManager.__init__ has already cached the registry. If two… |
|
src/specify_cli/presets/_commands.py — The catalogue-version decision is made before update_from_directory() acquires its transaction… |
|
src/specify_cli/presets/__init__.py — dry_run=True still enters _workflow_install_transaction, whose O_CREAT path creates and… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — Bulk mode deliberately sets effective_priority to None, but this predicate checks the raw… View resolved comment |
|
src/specify_cli/presets/__init__.py — This discards each historical agent's recorded names and passes every affected skill name… View resolved comment |
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/_commands.py:560
- Bulk
--priorityis documented and reported as ignored, but this preflight check uses the raw option. Supplying it makes a preset at the catalogue version actionable, so an already-current preset is downloaded and reinstalled instead of skipped.
This issue also appears on line 687 of the same file.
src/specify_cli/presets/_commands.py:653
- The metadata guard assumes every registry value is iterable. For a corrupt scalar entry such as an integer, single-item update raises a raw
TypeErrorat"version" not in metadata, bypassing the command's handled preset errors. Validate that the entry is a dictionary first, consistent with the existing enable/disable commands.
src/specify_cli/presets/init.py:4465
- Changed commands are written here directly and then the same names are written again by
_reconcile_composed_commandsbelow; skills follow the same duplicate pattern. For a changed lower-priority preset, the first pass temporarily installs losing content, and if the later best-effort reconciliation fails that incorrect content remains. The linked #4427 acceptance criteria require one full-stack reconciliation pass per update; route the diff union through that pass once and derive registry ownership from its result.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
src/specify_cli/presets/_commands.py:689
- The execution pass also uses the raw
priorityoption, so bulk--prioritycan bypass the equal-version skip after preflight (including when the installed version changes between the two passes). Use the effective value, which is intentionallyNonefor bulk updates, while preserving single-preset reprioritization.
if catalog_version < installed_version or (
catalog_version == installed_version and effective_priority is None
):
Comment on lines
+714
to
+715
| if source_kind == "dev" or source_path is not None: | ||
| manifest, diff = manager.update_from_directory( |
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The stale registry race and reconciliation/reporting defects must be fixed before approval.
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 (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — The transaction lock does not refresh the registry snapshot that PresetManager loaded in its… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The catalogue-version decision is made before update_from_directory() acquires its transaction… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — dry_run=True still enters _workflow_install_transaction, whose O_CREAT path creates and… View resolved comment |
|
src/specify_cli/presets/__init__.py — The update lock is acquired after PresetManager.__init__ has already cached the registry. If two… View resolved comment |
Suppressed comments (3)
src/specify_cli/presets/init.py:6029
- Alias-aware resolution is not matched by alias-aware reconciliation.
_reconcile_composed_commandsidentifies preset owners only withtmpl["name"] == cmd_name(lines 2188 and 2300). A wrap/prepend/append command whose alias has no lower layer therefore resolves as a lone composing alias,resolve_content(alias)returnsNone, and reconciliation unregisters the alias that registration just wrote; both install and update now feed aliases into that path. Canonicalize aliases to their primary identity during reconciliation, or make the owner/composition logic consistently alias-aware.
callers must NOT fall back to the convention lookup — that would mask a
typo or pick up an undeclared file. Shared by ``resolve()`` and
``collect_all_layers()`` so their manifest-first resolution cannot
silently diverge again (the divergence this fix addressed).
"""
src/specify_cli/presets/init.py:176
- This introduces a third successful return value,
"unchanged", but the function docstring still documents only"copied","composed", andNone. Document the new value so callers do not treat it as the composed case by default.
provenance = memory_constitution.parent / _CONSTITUTION_PROVENANCE_FILE
if memory_constitution.exists() and memory_constitution.read_bytes() == content:
# An unchanged constitution must produce zero file changes, so no
# provenance sidecar is written here either. Provenance is recorded
# only when constitution content is actually materialized below.
return "unchanged"
src/specify_cli/presets/_commands.py:771
- This predicts a constitution change from any manifest delta or
--priority, but it never evaluates the hypothetical post-update stack. Updating a disabled or non-winning constitution provider—or setting an effect-neutral priority—leaves the resolved bytes unchanged, so dry-run reports “constitution change planned” while the real update reports “constitution unchanged.” Compare the live bytes with content resolved from the staged manifest/priority, or describe this only as a possible reconciliation rather than a change.
constitution_status = (
"constitution change planned"
if constitution_can_reconcile
and (constitution_diff or priority is not None)
else "constitution unchanged"
Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77a8e35d-943a-45f9-8afd-b94968b66064
Author
|
Posted on behalf of @digimangos by GitHub Copilot (model: gpt-5.6-luna). Addressed the catalogue-version race in commit Validation: CI-equivalent Ruff passed; full preset suite passed, 712 tests. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Update locking, skill reconciliation, alias handling, dry-run reporting, and CLI help require correction.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The catalogue-version decision is made before update_from_directory() acquires its transaction… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/__init__.py — The transaction lock does not refresh the registry snapshot that PresetManager loaded in its… View resolved comment |
Suppressed comments (5)
src/specify_cli/presets/init.py:4261
- This lock does not serialize updates against the existing preset mutation paths:
preset_addcallsinstall_from_directory()andpreset_removecallsremove()without acquiring_workflow_install_transaction. If a remove runs after this update has renamed the live directory to its backup, it can delete the registry entry, after which this update writes its stale in-memory registry snapshot and resurrects the preset. Use the same transaction lock for every preset add/remove/update mutation (or move locking into shared manager entry points) so the directory swap and registry save cannot interleave.
with _workflow_install_transaction(self.project_root):
# Refresh after waiting for the lock so concurrent updates cannot
# overwrite newer registry metadata with this manager's snapshot.
self.registry.data = PresetRegistry(self.presets_dir).data
src/specify_cli/presets/init.py:4587
extra_skill_dirsis built only from the updated preset's ownregistered_skills, unlike the stack-wide historical-agent scan used for commands above. When--prioritypromotes this preset over another preset that owns the same skill in an inactive agent directory, that directory is never reconciled and continues serving the old winner. Scan other registry entries for affected skill names and add their tracked agent directories here, mirroringhistorical_agents.
include_disabled=True
):
if other_id == target_id or not isinstance(other_meta, dict):
src/specify_cli/presets/init.py:6042
- Alias resolution is not propagated to
_FilteredManifest.templates, which still retains templates only when their primarynamematches. During update reconciliation, aliases are included inreconcile_command_names; if a lower-priority preset is updated, its alias skill is written first, but_reconcile_skillsfilters out the higher-priority winner whose alias matched here, leaving the lower-prioritySKILL.mdactive. Make the reconciliation filters and preset-owner lookups alias-aware as well.
empty, or non-file (e.g. directory) declaration yields ``(entry, None)``.
The manifest is authoritative: when it declares a template (``entry`` is
not ``None``) but the file is unusable (``candidate`` is ``None``),
callers must NOT fall back to the convention lookup — that would mask a
src/specify_cli/presets/_commands.py:760
- A dry run reports
constitution unchangedwhenever the file is absent, but the real update callsreconcile_constitution(..., create_if_missing=True)and will create it when constitution-sync is enabled. Treat a missing constitution as materialization pending so the dry-run output accurately describes the write that a real run will perform.
and constitution_path.exists()
src/specify_cli/presets/_commands.py:475
- The help text implies that the positional value
allselects bulk mode, but bulk mode is only activated by omitting the ID or passing--all;specify preset update allinstead tries to update a preset literally namedall. Remove this claim or implement the positional spelling.
preset_id: str = typer.Argument(None, help="Preset ID to update (or all)"),
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Four moderate correctness and concurrency issues must be addressed 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 against the existing preset mutators. preset set-priority, enable,… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/specify_cli/presets/_commands.py — The catalogue-version decision is made before update_from_directory() acquires its transaction… View comment |
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:4523
- Legacy registries store
registered_skillsas a flat list, but this call omits the requiredfallback_agentargument. Every update of such a preset therefore raisesTypeErrorafter the directory/registry swap; the outer handler only warns and skips the remaining skill and constitution reconciliation. Pass the already-resolved active agent as the fallback, matching the existing migration call sites.
src/specify_cli/presets/init.py:4443
- This update path has the same alias/composition failure as installation: current aliases are added to
reconcile_command_namesand resolved as standalone commands, although they must share the primary command's composed output. A wrapped primary can therefore be updated successfully while its alias is immediately removed as “uncomposable.” Keep live aliases grouped with their primary registration, and only reconcile aliases that were actually removed so a lower-priority provider can be restored; add an update regression test using a wrapped command with an alias.
command_names.update(
alias
for alias in template.get("aliases", [])
if isinstance(alias, str)
)
src/specify_cli/presets/init.py:4495
- This writes each affected command through
_register_commands, and the later full-stack reconciliation writes the same command/skill set again. That violates #4427's requirement for one reconciliation pass over the added/removed/changed union, causes duplicate filesystem writes, and temporarily materializes the incoming preset rather than the winning stack. Consolidate registration and ownership tracking into the single full-stack reconciliation pass.
registered_commands = self._register_commands(
new_manifest, current_dir, command_names=primary_command_names
)
Comment on lines
+4258
to
+4261
| with _workflow_install_transaction(self.project_root): | ||
| # Refresh after waiting for the lock so concurrent updates cannot | ||
| # overwrite newer registry metadata with this manager's snapshot. | ||
| self.registry.data = PresetRegistry(self.presets_dir).data |
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
Implements
specify preset updatefor single and bulk preset updates.--from, and--devupdate sources.Closes #4427.