You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(presets): recompose surviving skills when a lower preset layer is removed - #5013
Removing a preset used as a base or middle layer of another preset's prepend, append, or wrap composition can leave the removed instructions in SKILL.md. Removal originally revisited only destinations owned by the retiring preset, while the composed output belongs to the surviving preset.
The original reproduction installs alpha as a replacement for speckit.specify and beta as an appended layer. After removing alpha, the resolver drops ALPHA BODY, but the original baseline's Claude skill still contains it. This PR refreshes the surviving preset's recorded skill destinations and native-skill aliases from the remaining composition.
Reconciliation uses each output's owner and registration records. Historical agents compose only layers registered for their destination; native extension command records and inferred legacy preset skill records participate in that filtering. A replace-only survivor remains independent of lower layers. Owned output is retired when no base remains; unreadable surviving layers and output without the expected ownership marker are preserved. Install/activation also restores recorded outputs when a new base makes them composable again.
Native command cleanup preserves outputs successfully written by a surviving preset after another preset's partial registration failure. Both command-style and skill-style ownership markers are recognized. An explicit marker for the retiring preset takes precedence over a command-prefix match, so a survivor literally named preset cannot claim preset:<retiring-id> and leave removed instructions behind.
Existing file permissions are preserved when refreshing an existing skill. Legacy extension skill records remain flat lists without agent ownership: nonempty flat records and extensions without registration metadata retain their existing fallback behavior. Legacy preset inference retains its active-agent fallback for names whose owner cannot be recovered.
Testing
Latest local verification on macOS arm64, Python 3.13.14:
Ruff 0.15.0 (check src tests) and git diff --check: pass.
The follow-up regressions use real preset/extension installation and removal APIs in temporary projects:
A Codex-only extension must not enter Claude's historical skill or alias. The negative case fails on a5429b36; the Claude-registered control passes. Both pass with the filter, and stashing its production change reproduces the failure.
A legacy flat preset record must not bypass historical-agent filtering. Core-fallback and no-fallback cases fail on 59a10c1, pass with inference, and fail with that production change stashed; modern-record controls pass throughout.
A base skills-phase failure must not let command cleanup delete a surviving preset's native skill. Failure-injection tests fail on 5f1fc6be for active and historical destinations, pass with the ownership check, and fail with that production change stashed.
After an injected skill-registry write failure, a survivor named preset must not retain the removed preset's skill. This fails on 1bcd341f; the ordinary-name control passes. Both pass with the marker check, and stashing that change reproduces the failure.
The unchanged shell, documentation, dependency, and extension checks were validated for the preceding 59a10c1 follow-up: shellcheck at error severity, workflow Bash portability guard, documentation glob check, Markdownlint, hashed-requirements pip-audit, dependency-input check, and extension version guard all passed. Subsequent commits change only the preset skill manager and its tests.
The full suite needs this checkout's venv on PATH for shell/PyYAML tests and a wide COLUMNS value for existing literal-output assertions. No unrelated code or tests were changed to obtain the passing result. Native Linux/Windows and the CI Python-version matrix were not run locally. The previous remote head's workflows require maintainer approval. These results validate the existing PR and its follow-ups; they are not a fresh reproduction of the original issue on today's default branch.
AI Disclosure
I did not use AI assistance for this contribution
I did use AI assistance
AI disclosure: Earlier work on this PR was disclosed as Claude Code (Claude Opus 5.5, max reasoning, autonomous mode). The current follow-ups and this description were prepared on behalf of @yxshee by Codex (GPT-6, autonomous mode; exact model variant and reasoning settings are not exposed). Codex reproduced the reported functional cases, generated the fixes and regression tests, ran the reported automated checks, and prepared the review text. No human line-by-line review or manual execution is claimed. The human contributor remains the sole Git author.
… removed
Removing a preset that is the base (or a middle layer) of another preset's
prepend/append/wrap composition left the removed preset's content in the
active agent's SKILL.md (Claude, Codex, Copilot with --skills, ...), while
command-file agents such as Gemini were recomposed onto the new stack.
The composed skill is owned by the surviving preset at the top of the
chain, so _unregister_skills() restores nothing for the removed preset,
and _reconcile_skills() only revisited the active skills directory when
that restore had touched it. The recomposed .composed/<cmd>.md that
_reconcile_composed_commands() writes was therefore never rendered into
the skill, and `specify preset resolve` disagreed with what the agent ran.
After a removal, also reconcile active-directory skills for commands whose
winning layer composes, as long as the skill still carries the winning
preset's source marker. A hand-edited skill is left alone, matching the
existing protection for non-owned skills during removal.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When both the modern (speckit-specify) and legacy (speckit.specify)
skill directories exist, the active-directory recomposition added for
lower-layer removal selected the command because the modern skill kept
the winning preset's marker, and _register_skills() then rewrote both
directories, overwriting a user-owned legacy skill.
Give _register_skills() an optional only_skill_names filter and pass the
directory's managed names on removal, so only names that are still
preset-owned are written. Install and rescaffold calls pass None and
keep writing every existing directory, as before.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the Copilot finding that removal recomposition could overwrite a user-owned legacy skill (commit 8c0328f):
_register_skills() gains an optional only_skill_names filter; removal passes the directory's managed names for the active directory. Install and rescaffold behavior is unchanged.
New regression test_remove_base_preset_preserves_user_owned_legacy_skill: fails on the previous head (1239cbe), passes now.
AI disclosure: written and pushed by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the code, test and this comment are AI-generated.
When the removed preset was the only replace base of a composing
command, with no core or extension fallback, resolution returns None.
Command-file agents unregister that command, but the survivor's stale
.composed/<cmd>.md stayed behind, and the removal recomposition rendered
it back into the skill, so the removed preset's instructions stayed
active.
Track composing winners whose stack no longer resolves, delete their
stale composed cache, and on removal delete the active skill only while
it still carries the winning preset's source marker. Hand-edited skills
are kept, matching the existing ownership protection.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the Copilot finding about removing a command's only base with no core or extension fallback (commit 57bdfc9):
The stale .composed/<cmd>.md of a composing winner whose stack no longer resolves is deleted, so it can't be rendered back into a skill.
On removal, the active skill for that command is deleted only while it still carries the winner's source marker; hand-edited skills are kept.
New tests: test_remove_sole_base_preset_retires_stale_composed_skill (fails on the previous head, passes now) and test_remove_sole_base_preset_keeps_hand_edited_skill.
AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.
Handle aliases during preset retirement and recomposition
src/specify_cli/presets/_manager_skills.py:465
This loop checks only the primary command's modern and legacy names, not its manifest aliases. On Codex, an appending preset can declare speckit.alt as an alias of speckit.custom; removing its only base retires speckit-custom but leaves speckit-alt/SKILL.md runnable with the removed instructions. agents.py:934–1015 renders aliases separately, using <id>:<file> source markers, so the preset:<id> check also misses their ownership. Include the winning template's aliases and their actual ownership markers in recomposition and retirement, preserve user-owned files, and add a native-skill alias regression.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated this feedback for the requesting reviewer.
Round-3 review of the no-base removal fix:
- Delete a cache only when no path component below the project root is a
symlink, so a symlinked .composed directory can't redirect the
deletion to a file outside the project.
- Drop every surviving preset's .composed/<cmd>.md for an uncomposable
command, not only the winner's: a middle preset's stale cache could be
rendered back into the skill by a later rescaffold.
- Delete through the journal-aware unlink helper, as the cache writes
already do, so a failing integration lifecycle can restore the cache.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the three Copilot findings on stale composed-cache deletion (commit c184e1a):
Deletion skips any cache reached through a symlinked path component below the project root.
All surviving presets' caches for an uncomposable command are dropped, so a rescaffold can't render the removed base back.
Deletion uses the journal-aware unlink helper, like the cache writes.
Three new tests, each failing on the previous head. Preset, integration, extension and bundle suites: 6101 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.
AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.
presets_by_priority excludes disabled presets (_registry.py:227), so their stale caches survive this cleanup. With a replace base beneath two append presets, disable the middle preset and then remove the sole base: the middle cache still contains the removed instructions. Re-enabling it and rescaffolding can render that cache before reconciliation deletes it; rescaffold reconciliation does not retire the newly written skill. Invalidate caches for all installed presets, including disabled ones, and extend the three-layer regression to cover disabling and re-enabling the middle layer.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.
Preserve output when surviving layers cannot be read
src/specify_cli/presets/_manager_skills.py:211
resolve_content() also returns None when a surviving layer cannot be read or decoded (_resolver.py:780–783,865–871). Treating every None as a missing base deletes the last valid cache and owned skill even though a replace base still exists. A dependency-free execution with a non-UTF-8 base confirmed both deletions. Only retire output when the stack has no replace layer; preserve existing output on read failures, as test_unregister_skills_in_dir_unreadable_core_template_skips already requires for restoration. Add a composed-stack regression for this case.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.
Journal skill-directory removals during force replacement
src/specify_cli/presets/_manager_skills.py:434
This rmtree() bypasses the lifecycle file-change journal. Force replacement calls remove() from _manager.py:408; when that runs under an active journal, a later failure cannot restore this deleted skill, although cache deletion is observed. design/integration.md, “Storage, loading, and lifecycle,” requires preset replacements and generated artifacts to participate in the journal. Wrap this removal like the other skill deletions at lines 1377–1381 and 1644–1648, and extend the observer regression to check the skill-directory removal too.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.
…lures
Round-4 review of the no-base removal fix:
- Skip registry keys (and command names) that are not a single plain
path segment, so a malformed key such as "../../../outside" cannot
steer cache cleanup outside the project.
- Invalidate stale caches for every installed preset, disabled ones
included: re-enabling a disabled composer and rescaffolding could
otherwise render its stale cache back.
- Retire output only when the stack has no replace layer left.
resolve_content() also returns None when a surviving layer can't be
read; that keeps the existing cache and skill instead of deleting them.
- Journal the stale skill-directory removal like the other skill
deletions, so a failing lifecycle can restore it.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the new finding and the three previously missed ones (commit b9a2c35):
Traversing registry keys: cleanup accepts only plain single-segment preset ids and command names.
Disabled presets: stale caches are invalidated for every installed preset, so re-enabling and rescaffolding can't render the removed base back.
Read failures: output is retired only when no replace layer remains; an unreadable surviving base keeps the existing cache and skill.
Journal: the stale skill-directory removal is wrapped in before_file_change/after_file_change like the other skill deletions.
Regressions: test_stale_cache_cleanup_ignores_traversing_registry_key, test_remove_sole_base_drops_disabled_preset_cache, test_unreadable_surviving_base_keeps_composed_output, and the journal test now also checks the skill directory; each fails on the previous head.
AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.
Limit historical-directory reconciliation to managed skill names
src/specify_cli/presets/_manager_skills.py:395
The new name filter protects only the active directory. The historical-directory branch below still calls _register_skills() without only_skill_names, so it writes every existing modern/legacy sibling. If removal restored only speckit-specify in an inactive agent's directory and preserved a user-owned speckit.specify beside it, reconciliation overwrites that preserved file with the surviving preset's content. Pass managed_names to the targeted call too, and add a regression with mixed ownership in a non-active agent directory.
AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.
Retire stale skills during activation and rescaffolding
src/specify_cli/presets/_manager_skills.py:453
This retirement only runs during removal, but activation and rescaffolding call _reconcile_skills() without extra_skills_dirs. After removing a custom command's only base while Codex is active, a previously written Claude skill can retain the removed body when Claude is reactivated: registration has no base/cache, and this deletion is skipped. The same problem occurs when the sole composer was disabled during removal; re-enabling it lets registration render its stale cache before reconciliation deletes only the cache. Move ownership-checked retirement outside the removal-only guard, and add regressions for both flows.
AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.
Handle native-skill aliases during recomposition and retirement
src/specify_cli/presets/_manager_skills.py:520
Native-skill aliases still retain removed instructions. CommandRegistrar.register_commands() renders a separate skill for each manifest alias (agents.py:934–1015), but this ownership scan and _register_skills() consider only the primary command's modern/legacy names. For an appending preset with alias speckit.shortcut, removing its base refreshes the primary skill but leaves speckit-shortcut/SKILL.md stale. Include aliases in recomposition and retirement, and recognize their registrar-generated source markers (<preset-id>:<source-file>), which differ from the primary skill's preset:<id> marker. Add a native-skill alias regression.
AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unresolved artifact-replacement flaws can discard ownership and ACL protections or permit temporary-entry substitution, risking restricted-content exposure and installation of substituted artifacts.
…ered there
Address four review findings:
- Restore a retired alias when its directory still exists. Retiring
removes only SKILL.md, so a directory holding other files stays; the
missing check now looks at the file, not the directory.
- When a survivor's skill or alias in a historical agent's directory is
recomposed after a removal, use only layers registered in that
directory. A preset installed while another agent was active is no
longer composed into it.
- Drop CommandRegistrar._replace_file_text. The alias is written in
place by command registration again, so the same inode keeps its
owner, ACL and mode, and no temp entry is renamed over it. agents.py
and the tests patched for the helper are back to main.
- An alias SKILL.md with more than one link is left alone instead of
being replaced, as other generated artifacts with extra links are.
The .composed cache goes through _write_shared_text(preserve_mode).
Assisted-by: Claude Code (model: Claude Mythos 5.2, autonomous)
Addressed the four findings from the Copilot review on 1b81d0a in 729c799.
Retired alias not restored when its directory still exists (discussion_r4517546288): retiring removes only SKILL.md, so a directory holding other files stays behind. The alias pass now treats the alias as missing when SKILL.md is missing, not when the directory is, so a new base restores it next to the companion files.
Another agent's layer composed into a historical directory (discussion_r4517546241): reproduced. With Claude's skill owned by a composing preset, a base installed while Codex was active ended up in Claude's skill and alias once the original base was removed. Recomposing a survivor's output in a historical directory now uses only layers registered in that directory (_layers_registered_in, from each preset's registered_skills / registered_commands). Core, extension and project-override layers always stay, and the active directory is never filtered. If nothing registered there is left to compose onto, the output is retired, and the next rescaffold for that agent recreates it. The existing path for outputs owned by the removed preset itself is unchanged.
Owner and ACL lost on replace (discussion_r4517546169) and pathname-based chmod and replace (discussion_r4517546057): both come from CommandRegistrar._replace_file_text, which this PR added to avoid writing through a hard-linked alias. I removed it. Command registration writes the alias in place again, exactly as on main, so the same inode keeps its owner, group, ACL and mode, and there is no temp entry to chmod or rename. For the hard-link case, an alias SKILL.md with more than one link is now left alone, which is how _generic_owned_output and the command update transaction already treat generated artifacts with extra links. The .composed cache goes through _write_shared_text(..., preserve_mode=True).
This also shrinks the PR: agents.py, tests/integrations/test_integration_generic.py and tests/test_extensions.py are back to main, and tests/specify_cli/test_agents_replace_file_text.py is gone with the helper. The earlier threads about that helper (mode, umask, temp name, temp mode, descriptor leak) no longer have code to apply to.
Tests:
test_new_base_restores_retired_outputs_beside_companion_files: failed on 1b81d0a
test_remove_base_does_not_compose_other_agents_layer_into_historical_dir (with and without a core fallback): both failed on 1b81d0a
test_alias_refresh_rewrites_the_same_file: failed on 1b81d0a, where the inode changed
test_alias_refresh_does_not_write_through_hardlinked_skill: updated to assert that the alias and the file it is linked to are both untouched
test_new_base_does_not_restore_alias_through_symlinked_dir: a guard for the changed missing check; it passes before and after
CI: pytest (windows-latest, 3.15) failed on 1b81d0a and I couldn't read its log from here. My guess is the 250-byte filename test in test_agents_replace_file_text.py, which exceeds MAX_PATH under the pytest temp directory on Windows. That file is removed in this push. I'll check the next run.
AI disclosure: Claude Code (Claude Mythos 5.2) worked autonomously on this round. It wrote the fixes, the tests and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
New cache files use mode 0644 despite a restrictive umask, potentially exposing private command content when the .composed directory is traversable by other users.
The alias refresh wrote its .composed cache through
_write_shared_text(preserve_mode=True). With no existing entry there is
no mode to preserve, so the new file got the writer's fixed 0644 even
under a restrictive umask. Create a missing entry exclusively instead,
so the kernel applies the process umask as it does for the other
.composed writes; an existing entry still goes through the shared
writer and keeps its bits.
Assisted-by: Claude Code (model: Claude Mythos 5.2, autonomous)
Addressed the new finding from the Copilot review on 729c799 in b14ca0d. That review marked the four findings from the previous round as resolved.
Missing cache files ignore a restrictive umask (discussion_r4527566129): the alias refresh wrote its .composed cache through _write_shared_text(..., preserve_mode=True). With no existing entry there was no mode to preserve, so the new file got the writer's fixed 0644. A missing entry is now created exclusively (open(..., "x")) after the same path guards, so the kernel applies the process umask, as it does for the other .composed writes on main. An existing entry still goes through the shared writer: it is replaced atomically and keeps its bits.
New test: test_historical_alias_refresh_creates_cache_under_umask. Under umask 077 the new cache file was 0644 on 729c799 and is 0600 with the fix.
Preserve file permissions during atomic writes (discussion_r4495631107), which the review still lists as open. Where it stands after this push:
Existing .composed cache entries and existing SKILL.md files keep their permission bits (daac335, 729c799).
New cache entries follow the umask (this push).
A new SKILL.md gets 0644 from _write_shared_bytes, the same as every skill registration on main. This PR doesn't change how new skills are created, so I've left that alone. If you want new skills to follow the umask, I think it belongs in a separate change to the shared writer, and I can open one.
CI on 729c799 was green on all jobs, including the three Windows ones, so the Windows failure on 1b81d0a is gone.
AI disclosure: Claude Code (Claude Mythos 5.2) worked autonomously on this round. It wrote the fix, the test and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because command-layer selection can introduce another agent’s layer, while frontmatter-only composition can leave aliases executing removed instructions.
When a base is removed, a historical agent's skills are recomposed only
from layers registered in that directory. A preset counted as registered
there when its registered_commands named any agent resolving to it. For a
command-backed agent that entry records command files, and its skills
directory can be another agent's: Amp writes .agents/commands while its
skills directory resolves to .agents/skills, Codex's native one. A preset
installed under Amp was therefore composed into Codex's skill and alias.
Count registered_commands only for native skill agents, where a
registered command is a file in the skills directory. registered_skills
counts for any agent, as before.
Also pin that an alias of a composer whose strategy is declared only in
frontmatter never carries a preset base: registration writes it from the
preset's own file, so removing the base leaves it as a rescaffold
renders it.
Assisted-by: Claude Code (model: Claude Mythos 5.2, autonomous)
The Copilot review on b14ca0d left no new inline comments. It marked the umask finding as resolved and gave two reasons for human review in its summary. I checked both; 4ddbcb8 fixes the one that reproduced.
"Command-layer selection can introduce another agent's layer": reproduced and fixed. When a base is removed, a historical agent's skills are recomposed only from layers registered in that directory. _layers_registered_in counted a preset as registered there when its registered_commands named any agent resolving to the directory. For a command-backed agent that entry records command files, not skills, and its skills directory can be another agent's: Amp writes .agents/commands, while its skills directory resolves to .agents/skills, which is Codex's native one. So a preset installed while Amp was active was composed into Codex's skill and alias once Amp was no longer the active agent, although it had never written there. registered_commands now counts only for native skill agents (/SKILL.md), where a registered command is a file in the skills directory. registered_skills counts for any agent, as before.
New test: test_remove_base_does_not_compose_command_mode_layer_into_shared_skills_dir, with and without a core fallback. Both cases failed on b14ca0d (the Amp-only layer's body was composed into Codex's skill, and without a fallback the skill was kept instead of retired) and pass with the fix.
"Frontmatter-only composition can leave aliases executing removed instructions": I could not reproduce this and changed no code for it. Command registration (_register_commands, unchanged from main) composes an alias onto preset layers only when the manifest declares the strategy. When the strategy is declared only in the command file's frontmatter, the alias is written from the preset's own file: the raw fragment for append and prepend, and for wrap the registrar's substitution of the core template. It never holds a lower preset's text, so removing that preset leaves nothing stale in it, and the alias pass skips it for that reason. test_frontmatter_strategy_alias_never_carries_a_preset_base (append and wrap) pins this: the alias has none of the base's text after install, and is byte-identical after the base is removed and after a rescaffold, while the primary skill is recomposed. The same alias assertions hold on main. For these presets the alias and the primary skill render differently on main too; aligning them would be a registration change, which I can propose separately if you want it.
Preserve file permissions during atomic writes (discussion_r4495631107), still listed as open: nothing new since my last note. Existing files keep their permission bits and new cache entries follow the umask. The remaining case is a new SKILL.md at 0644, which is what _write_shared_text gives every preset skill on main. Changing it only for this PR's writes would make them differ from registration, so I would keep that for a separate change to the shared writer.
AI disclosure: Claude Code (Claude Mythos 5.2) worked autonomously on this round. It wrote the fix, the tests and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed to evaluate subtle preset-removal interactions that can discard higher-priority project overrides or overwrite aliases belonging to another command.
… removal
Two removal-pass cases rendered something a rescaffold would not:
- A project override added after the presets were installed wins the
command, but a surviving composer's skill was recomposed from the
composer's layer down (or retired when no base was left) as if the
override were not there. Route such a skill to the winner pass, which
renders the override.
- An alias name declared by two commands of one preset is a single file,
registered for the last of them. Reconciling the other command matched
the file on the preset id alone and wrote its composition over it.
Require the marker of this command's own file (raw or composed).
Assisted-by: Claude Code (model: Claude Mythos 5.2, autonomous)
The Copilot review on 4ddbcb8 left no inline comments again. Its summary asks for human assessment of "preset-removal interactions that can discard higher-priority project overrides or overwrite aliases belonging to another command". I reproduced both and fixed them in 8f47609.
Project override ignored on removal. A project override (.specify/templates/overrides/) wins a command for every agent. If it is added after the presets were installed, a composer's skill still shows the old composition until something re-renders it. When a base under that composer was then removed, the survivor refresh recomposed the skill from the composer's layer down, or retired it when no base was left, as if the override were not there. Such a skill now goes to the winner pass, which renders the override, so the result is what a rescaffold renders.
New test: test_remove_base_renders_project_override_over_survivor_skill, for the active and a historical agent directory, each with and without a core fallback. All four cases failed on 4ddbcb8 (two rendered the composition without the override, two deleted the skill).
Alias of another command overwritten. When two commands of one preset declare the same alias, there is one alias file, registered for the last of them. The alias pass matched the file on the preset id alone, so reconciling the first command wrote its composition over the second command's alias. It now requires the marker of this command's own file (<id>:<file> or <id>:.composed/<command>.md).
New test: test_alias_refresh_leaves_alias_registered_for_another_command. On 4ddbcb8 the alias changed from speckit.b's composition to speckit.a's when speckit.a's base was removed; with the fix it is byte-identical before and after, and after a rescaffold.
Preserve file permissions during atomic writes (discussion_r4495631107) is still listed as open. My position is unchanged from the last two notes: the remaining case is a new SKILL.md at 0644, which is what _write_shared_text gives every preset skill on main, so I would keep it for a separate change to the shared writer.
On 4ddbcb8 only the license check has reported so far; the test workflows have not run on it.
AI disclosure: Claude Code (Claude Mythos 5.2) worked autonomously on this round. It wrote the fixes, the tests and this summary, and ran the checks above. yxshee reviews the PR.
When a composed command has no base left, its stale skill is deleted.
The ownership check accepted any marker starting with an installed
preset's id, so it also matched another preset's alias that uses the
command's skill name: after speckit.custom lost its base, a rescaffold
deleted an alias named speckit.custom that another preset registered
for its own command, and nothing restored it.
Match the markers a preset writes when it renders this command:
preset:<id>, or <id>:<file> with that preset's own file for the
command, raw or composed.
Assisted-by: Claude Code (model: Claude Mythos 5.2, autonomous)
The Copilot review on 8f47609 has no inline comments. Its summary says "rescaffolding after a command loses its base can delete another preset's valid alias without restoring it". That reproduced, and it was a regression from this PR, so I fixed it in a5429b3.
When a composed command has no base left, its stale skill is deleted. The ownership check in _remove_owned_skills accepted any source marker that starts with an installed preset's id. That also matches another preset's alias when the alias uses the command's skill name. Steps: speckit.custom loses its base, so its skill is retired while its composer stays installed. Another preset then registers speckit.custom as an alias of its own command. On the next rescaffold the command is still uncomposable, and the cleanup deleted that alias. A second rescaffold did not bring it back. main keeps the alias.
The check now matches only the markers a preset writes when it renders this command: preset:<id>, or <id>:<file> with that preset's own file for the command (raw or .composed/<command>.md).
New test: test_rescaffold_keeps_other_presets_alias_named_like_baseless_command. It failed on 8f47609 (the alias file was gone after the rescaffold) and passes with the fix. The existing tests that retire a composer's own stale output through a command-registration marker still pass, and two of them fail if that branch is removed, so both directions are covered.
Preserve file permissions during atomic writes (discussion_r4495631107) is still listed as open; my position is unchanged (a new SKILL.md gets 0644 from _write_shared_text, as on main).
The test workflows have not run on the last two pushes (only the license check reported on 4ddbcb8 and 8f47609).
AI disclosure: Claude Code (Claude Mythos 5.2) worked autonomously on this round. It wrote the fix, the test and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed for a cross-agent ownership edge case where removing a preset can introduce extension instructions into Claude’s surviving skill and alias despite no Claude registration.
The extension ownership case is reproduced and addressed in 59a10c193f8126606c2dcf997f5a31aaed8e09b6. Removing a base preset no longer composes a Codex-only extension into Claude's surviving skill or alias. The positive control retains an extension registered for Claude, and the active Codex output still resolves.
The new regression is red on the previous PR head and with the production fix stashed, then green with the fix. The full local suite passes on macOS/Python 3.13.14: 14565 passed, 446 skipped. Ruff, shell checks, Markdown lint, and the committed dependency audit pass. The PR description records the environment settings, skipped platform matrix, and the unchanged legacy flat-registration limitation.
The existing new-file permission discussion remains outside this follow-up; this commit does not change file-writing permissions.
Current-head CI: Test Python, Lint & Security, CodeQL, and Extension Version Guard are waiting for maintainer authorization (action_required). The license compliance check passed.
Prepared on behalf of @yxshee by Codex (GPT-6, autonomous; exact model variant and reasoning settings not exposed). AI produced the code, tests, automated verification, and this review summary; no human line-by-line review is claimed.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because preset removal can delete a surviving preset’s skill without restoring it, while legacy registration handling can incorrectly select another agent’s layer.
Reuse skill ownership inference before filtering historical preset layers, so a legacy flat list does not register a preset in every agent directory.
Assisted-by: Codex (model: GPT-6, autonomous)
The legacy-registration case in the review summary is reproduced and fixed in 5f1fc6b. A flat preset skill record previously let a Codex-only lower layer enter Claude's historical composition. The filter now infers that record's destinations with the same helper used elsewhere in removal. The new legacy variants fail on 59a10c1 with and without a core fallback, pass with the fix, and fail again when only the production change is stashed. The modern-record controls remain green.
The surviving-skill deletion case is fixed in 1bcd341. The regression injects a failure in the base preset's skills phase while allowing command registration and the surviving preset's skill registration to succeed. Removing the base then deleted the survivor's native skill on 5f1fc6be; its preset:<id> marker was missing from the cleanup guard. Recognizing that marker preserves the file for recomposition. Active and historical directory variants now pass and both fail with only this production change stashed.
The older permission thread points to code that has since changed: the composed-cache refresh preserves an existing file's mode and creates missing cache entries exclusively under the process umask. Existing skill refreshes pass preserve_mode=True. The current tests for existing alias/cache/primary skill modes and new alias/cache creation masks pass. This is verification of those covered paths, not a claim that every use of the shared writer honors umask for new files; its default new-file mode remains 0644.
Local verification: all 134 preset-skill tests pass; the full suite reports 14,569 passed, 446 skipped, 73 warnings on macOS arm64/Python 3.13.14 with the documented environment. Ruff 0.15.0 and git diff --check pass. Prior remote runs required maintainer workflow approval; local success does not substitute for that CI matrix. Review conversations remain unresolved for the reviewer.
AI disclosure: Posted on behalf of @yxshee by Codex (GPT-6, autonomous mode; exact model variant and reasoning settings are not exposed). Codex investigated the feedback, generated both patches and their tests, ran the automated checks, and wrote this summary. No human line-by-line review or manual execution is claimed.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because preset-removal ownership collisions and reconciliation edge cases can delete surviving skills or silently retain removed instructions.
A surviving preset named preset must not claim the removed preset ownership marker during partial-registration cleanup.
Assisted-by: Codex (model: GPT-6, autonomous)
The ownership-marker collision is fixed in ce821bb. After an injected skill-registry write failure, native command cleanup can encounter a preset:<retiring-id> marker without a matching skill record. An unrelated survivor named preset previously matched that marker as its command prefix and kept the removed preset's file. The cleanup check now gives the retiring preset's explicit skill marker precedence.
The new case fails on 1bcd341f; an ordinary survivor-name control passes. Both pass with this fix, and stashing only the production change reproduces the failure. All 136 focused tests pass, and the full suite reports 14,571 passed, 446 skipped, 73 warnings. Ruff and diff checks pass. This follow-up changes no file-writing or permission behavior. Remote workflows on the previous head require maintainer approval; review conversations remain unresolved for their reviewers.
AI disclosure: Posted on behalf of @yxshee by Codex (GPT-6, autonomous mode; exact variant and reasoning settings are not exposed). Codex designed the failure-injection reproduction, generated the fix and tests, ran the automated checks, and wrote this summary. No human manual test or line-by-line review is claimed.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Preset removal can render shared skills inconsistently and admit base layers based on unrelated command registrations, producing incorrect primary skills and aliases.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Shared skill directories can be refreshed once per agent, making command-reference syntax depend on registry order and potentially leaving primary skills inconsistent with their aliases.
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
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.
mnriem
added
the
triage-must-have
Description
Removing a preset used as a base or middle layer of another preset's
prepend,append, orwrapcomposition can leave the removed instructions inSKILL.md. Removal originally revisited only destinations owned by the retiring preset, while the composed output belongs to the surviving preset.The original reproduction installs
alphaas a replacement forspeckit.specifyandbetaas an appended layer. After removing alpha, the resolver dropsALPHA BODY, but the original baseline's Claude skill still contains it. This PR refreshes the surviving preset's recorded skill destinations and native-skill aliases from the remaining composition.Reconciliation uses each output's owner and registration records. Historical agents compose only layers registered for their destination; native extension command records and inferred legacy preset skill records participate in that filtering. A replace-only survivor remains independent of lower layers. Owned output is retired when no base remains; unreadable surviving layers and output without the expected ownership marker are preserved. Install/activation also restores recorded outputs when a new base makes them composable again.
Native command cleanup preserves outputs successfully written by a surviving preset after another preset's partial registration failure. Both command-style and skill-style ownership markers are recognized. An explicit marker for the retiring preset takes precedence over a command-prefix match, so a survivor literally named
presetcannot claimpreset:<retiring-id>and leave removed instructions behind.Existing file permissions are preserved when refreshing an existing skill. Legacy extension skill records remain flat lists without agent ownership: nonempty flat records and extensions without registration metadata retain their existing fallback behavior. Legacy preset inference retains its active-agent fallback for names whose owner cannot be recovered.
Testing
Latest local verification on macOS arm64, Python 3.13.14:
.venv/bin/specify --help: passes.tests/specify_cli/presets/test_manager_skills.py: 136 passed.PATH="$PWD/.venv/bin:$PATH" COLUMNS=1000 LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest -n auto: 14,571 passed, 446 skipped, 73 warnings.check src tests) andgit diff --check: pass.The follow-up regressions use real preset/extension installation and removal APIs in temporary projects:
a5429b36; the Claude-registered control passes. Both pass with the filter, and stashing its production change reproduces the failure.59a10c1, pass with inference, and fail with that production change stashed; modern-record controls pass throughout.5f1fc6befor active and historical destinations, pass with the ownership check, and fail with that production change stashed.presetmust not retain the removed preset's skill. This fails on1bcd341f; the ordinary-name control passes. Both pass with the marker check, and stashing that change reproduces the failure.The unchanged shell, documentation, dependency, and extension checks were validated for the preceding
59a10c1follow-up: shellcheck at error severity, workflow Bash portability guard, documentation glob check, Markdownlint, hashed-requirements pip-audit, dependency-input check, and extension version guard all passed. Subsequent commits change only the preset skill manager and its tests.The full suite needs this checkout's venv on PATH for shell/PyYAML tests and a wide COLUMNS value for existing literal-output assertions. No unrelated code or tests were changed to obtain the passing result. Native Linux/Windows and the CI Python-version matrix were not run locally. The previous remote head's workflows require maintainer approval. These results validate the existing PR and its follow-ups; they are not a fresh reproduction of the original issue on today's default branch.
AI Disclosure
AI disclosure: Earlier work on this PR was disclosed as Claude Code (Claude Opus 5.5, max reasoning, autonomous mode). The current follow-ups and this description were prepared on behalf of @yxshee by Codex (GPT-6, autonomous mode; exact model variant and reasoning settings are not exposed). Codex reproduced the reported functional cases, generated the fixes and regression tests, ran the reported automated checks, and prepared the review text. No human line-by-line review or manual execution is claimed. The human contributor remains the sole Git author.