feat(extensions): copy declared resources into extension skills by markuswondrak · Pull Request #5383 · github/spec-kit

markuswondrak · 2026-11-11T08:42:44Z

Description

Closes #5366.

Extension commands can declare resources in extension.yml. When a command is rendered as an agent skill, the matched files are copied as regular files into the generated skill directory (primary and alias skills), and the skill references them with skill-relative paths.

provides:
  commands:
    - name: speckit.example.review
      file: commands/review.md
      resources:
        - references/reviewer.md   # explicit file
        - templates/*.md           # gitignore-style wildcard (same dialect as .extensionignore)
        - scripts/bash/review.sh

Behaviour

Opt-in per command. Without resources, rendering is unchanged (extension-local .specify/extensions/<id>/… rewriting, no copies, no new warnings). Non-skill outputs (Markdown/TOML/YAML) are unchanged even when resources is declared.
Copy mode has no path rewriting at all. Body extension paths, project-relative paths and frontmatter scripts: are emitted as authored; non-path placeholders ({SCRIPT}, {ARGS}, __AGENT__, command tokens, Python prefix) still resolve.
Validation: manifest shape at load time (list of non-empty strings; no absolute, .., ! or invalid patterns). At install/render, before any output is written: explicit entries must be regular files, wildcards must match at least one file, symlinked or escaping matches are rejected, and so is a SKILL.md destination (case-insensitive). Wildcards never select the root extension.yml or the .specify-dev development render cache. Resources are resolved against the source tree (through the .extensionignore filter) before a forced reinstall removes the old package. Copies keep a restrictive source mode (group/other bits the source lacks are dropped; bits are never added). Symlinks in a package are not followed or vetted by resource matching; install resolves them, as on main.
Ownership (reuse): writes and removals go through a throwaway, never-saved IntegrationManifest (record_file / uninstall(force=True, remove_manifest=False)); no new hashing, path-safety or deletion code. Hashes are stored in the registry as skill_resource_hashes with project / home buckets of root-relative recorded paths, so home-scoped skills (Hermes) are supported. Project resources are removed by recorded path even after the configured directory moves (same precedent as generic_artifact_hashes). Home resources are removed only under the skills directory of an integration that declares the new optional registrar_config["home_scoped"] (Hermes), like its SKILL.md; a moved home directory leaves its resources in place. External adapters cannot set the flag.
Lifecycle:
- Unowned existing destinations are skipped with a warning and not recorded.
- Modified recorded copies are kept on refresh unless the user passes --force (integration upgrade/switch, extension add); the internal upgrade force does not overwrite them.
- extension remove / update delete all recorded copies, including edits. Per-agent unregistration only touches the agent's current skills directory.
- Preset removal restores extension copy-mode skills, including their resources.
- Install, update, generic refresh and generic disable rollbacks cover the resources: install rollback retires exactly the files the attempt wrote (observed through the existing file-change observer), the update backup covers recorded paths from both buckets, restored scripts keep their file mode, generic refresh rollback removes the nested directories it created, and a failed native refresh retires the resource copies it created (copies recorded before the attempt stay).
Scripts: ensure_executable_scripts takes an optional files list and only marks resources written in this render.
Docs: docs/extensions/developing.md (new "Skill resources" section), design/extension.md, design/integration.md, docs/integrations/api.md (the home_scoped field), docs/integrations/cli.md (switch --force also overwrites modified or unowned resource copies), extensions/template/extension.yml.

Known limitation (documented): agents run {SCRIPT} themselves. In the manual test below, Copilot CLI ran commands from the project root, so a skill-relative scripts/bash/x.sh only worked because the agent resolved it to the skill directory. Authoring docs state that the working directory is not guaranteed.

Follow-ups filed (out of scope, mostly pre-existing on main):

[Bug]: Failed extension add leaves native SKILL.md outputs behind #5379 failed extension add leaves native SKILL.md outputs behind
[Bug]: extension add overwrites a pre-existing user skill; extension remove then deletes it #5380 extension add overwrites a pre-existing user skill; extension remove then deletes it
[Bug]: Removing supplemental extension skills deletes unrelated user files (rmtree) #5381 removing supplemental extension skills deletes unrelated user files (rmtree)
[Feature]: Skill resources for presets (copy-mode aware composition and preset-owned resources) #5382 skill resources for presets (copy-mode aware composition and preset-owned resources)

Testing

Tested locally with .venv/bin/specify --help
Ran relevant existing tests with .venv/bin/python -m pytest tests -q. Full suite (-n auto): 15,632 passed, 519 skipped, 1 failed. The failure, tests/integration/test_preset_update_workflow.py::test_preset_update_cli_contract, also fails on a clean checkout of main (7ec720e). uvx ruff@0.15.0 check src tests and git diff --check are clean.
Tested with a sample project. Manual agent run with real CLIs, using a probe extension in copy and no-copy mode:
- Copilot CLI 1.0.105: in both modes the skill read its bundled reference and ran the probe script; the working directory was the project root. In copy mode the agent resolved the skill-relative script to the skill directory itself.
- Claude Code 2.1.116 and Codex 0.117.0 could not run (expired local auth, HTTP 401), so they are not verified.
New tests cover the schema, resolution, copy-mode rendering (native and supplemental), no-copy regression, non-skill outputs, ownership on install, refresh and --force, integration switch/upgrade, home-scoped destinations, removal (including symlink safety and legacy registries), preset restoration, and rollback of install, update and disable. Existing test expectations are unchanged; a few test doubles gained the new keyword parameters.
Regression evidence: copy-mode no-rewrite (test_copy_mode_skill_preserves_authored_paths), native removal orphans (test_remove_native_recorded_resources_preserves_unrecorded) and every review fix (disable/update rollback snapshot, install rollback, case-insensitive SKILL.md, per-resource source validation) were shown failing without the fix and passing with it.
Not run locally: Windows. No PowerShell changed; resources use regular-file copies only (no symlinks), so the Windows CI job is the platform check.

AI Disclosure

I did not use AI assistance for this contribution
I did use AI assistance (fill in the disclosure below)

AI disclosure: Planning decisions and the scope triage of review findings were made by @markuswondrak. The pi coding agent wrote the implementation, tests and documentation (orchestrator: Claude Opus 5.5 via GitHub Copilot; implementation subagents: GPT-6.1-terra; independent code review: GPT-6-astra). The agent ran autonomously within that plan; the filer did not review every line before commit. Reproductions, the test runs above and the manual agent run were executed by the agent.

Review rounds 1 and 2 (Copilot review): fixes were made by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous in implementation, directed finding by finding by @markuswondrak; no line-by-line human review of the code). Summary and commit SHA are in the PR comment.

Let extension commands declare per-command `resources` (explicit paths or gitignore-style patterns) in extension.yml. When a command is rendered as an agent skill, the matched files are copied as regular files into each primary and alias skill directory, and the skill body and frontmatter scripts are not path-rewritten. Commands without resources keep the existing no-copy behaviour byte for byte; non-skill outputs are unchanged. Copies are hash-tracked in the registry (`skill_resource_hashes`, project and home buckets of root-relative recorded paths) through a throwaway IntegrationManifest: unowned destinations are skipped with a warning, modified copies are kept unless the user passes --force, and remove/update delete all recorded copies. Lifecycle coverage includes refresh, integration switch/upgrade, preset restoration of extension skills, install and update rollback, and generic disable rollback. ensure_executable_scripts accepts an explicit file list for written scripts. Closes github#5366 Assisted-by: pi coding agent (GitHub Copilot, model: claude-opus-5.5 orchestrating gpt-6.1-terra subagents; review: gpt-6-astra; autonomous)

Copilot

🟡 Changes recommended

Unsafe resource cleanup and incomplete rollback can delete unrelated home files or lose existing installations and edited resources when updates or forced reinstalls fail.

9 open issues

What changed in this PR

Extends Spec Kit’s extension packaging to copy declared resources alongside generated agent skills, addressing #5366 while retaining legacy extension-local delivery.

Changes:

Adds per-command resource declarations, validation, and copy-mode rendering for primary and alias skills.
Tracks resource ownership across installation, refresh, removal, rollback, and preset restoration.
Adds focused tests and documents resource authoring, lifecycle behavior, and script limitations.

AI disclosure: GitHub Copilot (model: unknown, autonomous) prepared this review for the requester.

File	Description
`tests/test_extensions.py`	Tests manifest validation and selective script permissions.
`tests/test_extension_skills.py`	Tests resource ownership, refresh, removal, and home-scoped skills.
`tests/test_extension_registration.py`	Tests native skills, aliases, placeholders, and non-skill compatibility.
`tests/specify_cli/presets/test_manager_skills.py`	Tests resource restoration after preset removal.
`tests/specify_cli/integrations/test_command_upgrade.py`	Tests user-controlled resource overwrite behavior.
`tests/specify_cli/integrations/test_command_switch.py`	Tests target resource delivery and agent-scoped cleanup.
`tests/specify_cli/extensions/test_command_update_transaction.py`	Tests update rollback and copy-mode transitions.
`tests/integrations/test_integration_generic.py`	Tests generic resource ownership and lifecycle rollback.
`tests/extensions/test_skill_resources.py`	Adds resource resolution, copying, and cleanup tests.
`src/specify_cli/presets/_manager_skills.py`	Restores extension resources and copy-mode rendering.
`src/specify_cli/integrations/command_upgrade.py`	Forwards explicit resource overwrite consent.
`src/specify_cli/integrations/command_switch.py`	Forwards overwrite consent during switching.
`src/specify_cli/integrations/_helpers.py`	Passes overwrite settings into extension registration.
`src/specify_cli/extensions/_skill_resources.py`	Adds resource resolution, delivery, ownership, and retirement helpers.
`src/specify_cli/extensions/_command_update_transaction.py`	Expands skill backups and incoming-output tracking.
`src/specify_cli/extensions/__init__.py`	Adds manifest validation and resource lifecycle coordination.
`src/specify_cli/agents.py`	Copies resources into native skills and aliases.
`src/specify_cli/__init__.py`	Supports permission updates for selected scripts.
`extensions/template/extension.yml`	Adds commented resource declaration examples.
`docs/extensions/developing.md`	Documents copy mode, compatibility, ownership, and script caveats.
`design/integration.md`	Defines skill-only extension resource delivery.
`design/extension.md`	Defines resource contracts and lifecycle behavior.

_{Review effort: Max (auto)}

Validate resources before a forced reinstall removes the old package, by resolving them against the source tree through the .extensionignore callback instead of a temporary copy. Rollback and cleanup: - Update backup snapshots recorded resources from both the project and home buckets for native and supplemental routes, not only generic ones. - Generic refresh rollback also covers the resource destinations the refresh is about to write, restores deepest paths first, and restores file modes so executable scripts keep their execute bit. - Install rollback retires exactly the resource files the attempt wrote, observed through the existing file-change observer, instead of inferring them from hash differences. Unchanged rewritten files are now covered. Home scope: integrations that keep skills in the user's home directory declare registrar_config["home_scoped"] (Hermes). Recorded home resources are only removed under such an integration's configured skills directory; external adapters cannot set it. Project resources still follow recorded paths. Wildcard resources no longer select the root extension.yml or the .specify-dev development render cache. Move the resource tests to tests/specify_cli/extensions to mirror the source. Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous)

markuswondrak · 2026-11-11T11:15:50Z

Review round 1: changes pushed in a6eb024.

Forced reinstall now checks declared resources against the source tree, through the .extensionignore filter, before the old package is removed.
Update rollback restores recorded resources from both the project and home buckets, for native and supplemental skills as well as generic ones.
Generic refresh rollback also removes the resource destinations the refresh was about to write, and restores file modes so executable scripts keep their execute bit.
Install rollback now removes exactly the resource files the failed attempt wrote, observed through the existing file-change observer, instead of inferring them from hash differences. This covers rewritten files whose bytes did not change.
Home resources are removed only under the skills directory of an integration that declares the new optional registrar_config["home_scoped"] (Hermes). External adapters cannot set it, and records elsewhere under ~ are left alone. A moved home directory therefore leaves its resources in place, unlike the project bucket.
Wildcard resources no longer select the root extension.yml or the .specify-dev development render cache.
The resource test suite moved to tests/specify_cli/extensions/.

Full suite: 15,628 passed, 1 failed (test_preset_update_cli_contract, which also fails on main). design/extension.md, design/integration.md, docs/integrations/api.md and the PR description are updated.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5), run interactively and directed finding by finding by the contributor. The agent wrote the code, tests and this comment. The contributor has not reviewed every changed line.

Copilot

🟡 Changes recommended

Unresolved resource-handling flaws can expose machine-local credentials, copy files from outside the source tree, and delete other projects’ shared files without home-local ownership checks.

3 open issues

7 previously missed issues

In code that hasn't changed since last review

Preflight all skill targets before writing any commands

src/specify_cli/agents.py:703

register_commands_for_all_agents() can write a non-skill target before a later skill target rejects an invalid resource. Its new test passes because Claude is visited before Gemini, not because validation happens before all writes. Preflight every detected skill target before the write loop, and test a non-skill-first ordering in tests/test_extension_registration.py.

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

Generic refresh rollback loses replaced unowned files

src/specify_cli/extensions/__init__.py:2046

A forced generic refresh can replace an existing unowned resource, but this snapshot retains existing files only if already recorded. If the registry update then fails, rollback leaves the user's original contents lost and the replacement unowned. Include safe resource destinations the render may replace in the snapshot without treating the backup as ownership.

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

Rollback leaves newly created ancestor directories behind

src/specify_cli/extensions/__init__.py:2069

After a failed generic commands-to-skills refresh with a resource such as scripts/bash/run.sh, rollback removes bash/ but leaves scripts/, preventing removal of the new skill directory. Subsequent refreshes skip that directory as unowned, so registration cannot recover without manual cleanup. Record every previously absent ancestor and remove those empty directories deepest-first during rollback, preserving pre-existing directories.

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

Force-add rollback deletes pre-existing unrecorded files

src/specify_cli/extensions/__init__.py:3171

extension add --force can replace an unrecorded file left in an owned skill directory. If hook registration or the registry write then fails, this rollback deletes the replacement rather than restoring the user's original bytes. Snapshot existing destinations before writing and restore their bytes and mode on failure; retire only files that were absent before the attempt.

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

Native refresh failures leave unowned resource copies

src/specify_cli/extensions/__init__.py:4203

Native refreshes write resource copies before persisting their ownership hashes, but artifact rollback runs only for generic refreshes. If a Claude refresh creates a resource and registry.update() then fails, the file remains unrecorded, so later refreshes skip it as unowned and removal cannot clean it up. Add resource rollback to native refresh failures, restoring prior destinations and removing newly created copies.

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

Preserve restrictive source permissions when copying resources

src/specify_cli/extensions/_skill_resources.py:195

New resource copies use the process umask instead of preserving restrictive source permissions. A private 0600 source can therefore become 0644 under a 022 umask, allowing other local accounts to read it. Preserve restrictive source access when creating copies, while retaining the shell-script execution policy.

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

Document switch --force target-side resource overwrites

src/specify_cli/integrations/command_switch.py:121

switch --force now permits overwriting modified and unowned resource destinations when a target skill is rendered, but the help and shared CLI reference do not describe that target-side effect. Users can therefore replace target files while the option appears limited to uninstall cleanup and shared-template refresh. Update the switch help and docs/integrations/cli.md, following design/integration.md (“Integration delivery boundary”).

AI disclosure: GitHub Copilot (model: unknown, autonomous) verified and edited this feedback for the requester.

9 resolved since last review

_{Review effort: Max (auto)}

…completely Generic refresh rollback removes every directory the refresh created below the nearest pre-existing ancestor, so a nested resource no longer leaves a parent directory that blocks the skill directory and later retries. A failed non-generic refresh retires the resource copies it created, so they are not left unrecorded (skipped as unowned on the next refresh and never removed). Copies recorded before the attempt are kept. Resource copies drop the group and other permission bits their source lacks, so a private source file is not widened by the process umask. This only tightens; script execution handling is unchanged. Document that symlinks in an extension package are not followed or vetted by resource matching, and that `integration switch --force` also overwrites modified or unowned resource copies when the target's skills render. Tests: the install preflight test now uses a real non-skill-first agent order (amp before claude) and asserts the registrar is never reached; the registrar abort test is renamed to describe what it covers. Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous)

markuswondrak · 2026-11-11T12:25:26Z

Review round 2: changes pushed in f74926e.

Changed:

Generic refresh rollback removes every directory the refresh created below the nearest pre-existing ancestor, so a nested resource such as scripts/bash/run.sh no longer leaves scripts/ behind and blocks retries.
A failed non-generic (native or supplemental) refresh retires the resource copies it created, so they are no longer left unrecorded and skipped as unowned. Copies recorded before the attempt are kept.
Resource copies drop the group and other permission bits their source lacks. This only tightens; script execution handling is unchanged.
integration switch --force help and docs/integrations/cli.md now say it also overwrites modified or unowned resource copies when the target's skills render.
The install preflight test now uses a real non-skill-first order (amp before claude) and fails without the preflight. The registrar abort test is renamed to describe what it covers.

Not changed:

Registrar-level preflight across all agents: extension add already resolves all resources against the installed package before registering anything, refresh renders a single agent, and main has no cross-agent atomicity for other failures.
Rollback restoring a user file replaced under --force (generic refresh, extension add --force): --force is an explicit overwrite, and switch --force overwriting an unowned destination is asserted by an existing test. Existing main behaviour for pre-existing user skills is tracked in [Bug]: extension add overwrites a pre-existing user skill; extension remove then deletes it #5380.
Items on local-config.yml, directory symlinks and home ownership are answered inline.

Full suite: 15,632 passed, 1 failed (test_preset_update_cli_contract, which also fails on main).

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5), run interactively and directed finding by finding by the contributor. The agent wrote the code, tests and this comment. The contributor has not reviewed every changed line.

Copilot

🔵 Needs a closer look

Unresolved rollback defects can lose user resource contents or leave stale hashes after registry-save failures, while unsafe permission handling can expose private file contents.

No open issues 🎉

5 previously missed issues

In code that hasn't changed since last review

Snapshot unowned resources before forced replacement

src/specify_cli/extensions/__init__.py:2086

A forced generic refresh can replace an unowned resource inside an owned skill, but this snapshot condition omits the user's original bytes. If the registry save then fails, rollback leaves the packaged file in place while restoring the old ownership metadata. Snapshot existing resource destinations that the forced render may overwrite, including their modes, and add a registry-failure regression test.

AI disclosure: GitHub Copilot (model: unknown, autonomous) analyzed and drafted this feedback for the requester.

Restore removed resources with permissions applied before publication

src/specify_cli/extensions/__init__.py:2151

When rollback recreates a removed 0600 resource under umask 022, write_bytes() exposes its contents at 0644 before the saved mode is restored. Other local users can read the file during that window, and a failed chmod leaves it exposed. Restore through the existing _write_shared_bytes(..., mode=mode) helper so the saved permissions are applied before the file is published.

AI disclosure: GitHub Copilot (model: unknown, autonomous) inspected and verified this feedback for the requester.

Restore changed resources when registry save fails

src/specify_cli/extensions/__init__.py:4508

When a native refresh writes changed source bytes to a recorded resource and the registry save fails, this filter leaves the new bytes paired with the old recorded hash. The next ordinary refresh mistakes Specify's own output for a user edit and preserves it instead of updating it. Snapshot and restore overwritten resources and their modes on failure, and test a changed source followed by a failed save and retry.

AI disclosure: GitHub Copilot (model: unknown, autonomous) analyzed and drafted this feedback for the requester.

Reject resource paths beneath SKILL.md during preflight

src/specify_cli/extensions/_skill_resources.py:153

A regular source file such as SKILL.md/reference.md passes this check, but its destination cannot coexist with the generated <skill>/SKILL.md file. Resource delivery then catches the filesystem error as a warning, so installation succeeds without the declared resource. Reject paths whose first destination component is SKILL.md, case-insensitively, during preflight as well as the exact filename.

AI disclosure: GitHub Copilot (model: unknown, autonomous) analyzed and drafted this feedback for the requester.

Apply restrictive permissions before publishing copied resources

src/specify_cli/extensions/_skill_resources.py:204

With POSIX umask 022, copying a private 0600 resource publishes its contents at 0644 before _keep_restrictive_mode() runs, so other local users can read it if the directory is traversable. A failed chmod is ignored and can leave the copy readable indefinitely. Use a permission-aware IntegrationManifest write that applies restrictive permissions before exposing any bytes, while preserving ownership and change-observer tracking.

AI disclosure: GitHub Copilot (model: unknown, autonomous) inspected and verified this feedback for the requester.

3 resolved since last review

_{Review effort: Max (auto)}

mnriem · 2026-11-11T13:39:47Z

Please address the previously missed issues. If not applicable, please explain why

markuswondrak · 2026-11-11T13:40:52Z

Review round 3 (Copilot review 5858902778 on f74926e2): no code changes. The five "previously missed" findings were each checked against the code and decided by @markuswondrak. All are declined as not worth changing. No new commit.

Unowned resources not snapshotted before forced generic refresh (extensions/__init__.py:2086): declined. The overwrite only happens with --force, which is explicit consent to overwrite unowned files, and docs/integrations/cli.md says so. Rollback also needs a registry-save failure on top of that.
Rollback restore writes at umask mode, then chmods (extensions/__init__.py:2151): declined as a rare edge case. The window is between the write and the chmod, on a failed generic refresh only. Restored SKILL.md and command files follow the same pattern on main.
Native refresh leaves a rewritten resource with a stale hash after a registry-save failure (extensions/__init__.py:4508): declined. It needs a changed resource source plus a registry-write failure. The result is a "modified resource preserved" warning that --force resolves, with no data loss. main has no native refresh rollback and records no native hashes, so this is not a regression.
SKILL.md/reference.md passes preflight (_skill_resources.py:153): confirmed that resolve_skill_resources accepts it. Declined because it needs an unrealistic declaration, and delivery already reports it as a "Skipped resource" warning, so nothing is corrupted.
Copy published at umask mode, then tightened (_skill_resources.py:204): declined. The window is microseconds. record_file writes every other recorded artifact the same way on main. _keep_restrictive_mode already tightens resources beyond that. A fix would need a new write path in shared manifest code.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, supervised: each finding was discussed and decided by @markuswondrak before this comment). The agent read the code, ran a single read-only reproduction for the SKILL.md/ finding, and drafted this summary. No code was changed or tested in this round.

Copilot

🟡 Changes recommended

Unresolved resource-handling vulnerabilities can expose configuration secrets and private file contents, while unsafe cleanup can delete unrelated project files.

9 open issues

_{Review effort: Max (auto)}

mnriem · 2026-11-11T14:10:05Z

Please address Copilot feedback, fix test & lint errors and resolve conflicts

Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

The generic_extension fixture writes scripts/run.sh with write_text, which produces CRLF on Windows. Compare the delivered resource with the source bytes instead of a hard-coded LF literal. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

record_file wrote copies at the process-umask default and a later chmod tightened them, leaving a window where a private resource was readable and losing execute bits on non-.sh scripts. Add an optional mode to record_file that publishes the content atomically from a sibling file created with that mode, and pass the source's permission bits for resource copies. This drops _keep_restrictive_mode and no longer depends on the .sh-only execute-bit pass. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

Aliases are free-form, so a skill name can contain "/" (for example speckit-ext/group-alias). Resource cleanup compared single path components with the whole name and left those copies behind, orphaned once the registry entry was gone. Compare the name's path segments against the recorded path instead, in retire_skill_resources and remove_skill_resources, and ignore names with empty, "." or ".." segments. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

…a skill A failing resource resolution reused the unreadable-source warning, which told users the command file could not be read and to fix or remove it although that file was read fine. Emit a resource-specific warning naming the extension directory and the recovery steps. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

Hermes now receives home-scoped, shared resource copies; describe where they go, that projects share them, and how removal, update and refresh treat them. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

markuswondrak · 2026-11-11T14:53:49Z

Review round 4: merged main, fixed the Windows test failures, and addressed the Copilot review 5860015263 on f74926e2. Latest commit: db2fe9d0.

Changes:

Merged upstream/main. Conflicts in docs/integrations/cli.md and command_switch.py kept upstream's new --force / --refresh-shared-infra semantics and the PR's resource clause for --force (9514352f).
Windows CI: the resource tests compared copies with a hard-coded \n literal while the fixture writes CRLF on Windows. They now compare with the source bytes (499104f9).
Permissions (57fd93a1): IntegrationManifest.record_file takes an optional mode. The content is written to a sibling file created with that mode and then atomically replaced, so it is never on disk under a wider mode. Resource copies use the source's permission bits, narrowed by the umask and never widened. This replaces _keep_restrictive_mode, closes the umask window, and keeps execute bits on non-.sh scripts.
Nested alias names (e1d5e25a): resource cleanup now matches skill names by path segments, so resources of an alias such as speckit.ext/group.alias are removed with the extension. Names with empty, . or .. segments never match.
Preset restore (e0b89acc): an unresolved resource now produces a resource-specific warning naming the extension directory instead of the unreadable-source warning.
Docs (db2fe9d0): docs/integrations/hermes.md describes resource locations, cross-project sharing, and removal, update and refresh behaviour, including the moved-directory exception.

Each fix has a test that fails without it. ruff and markdownlint are clean. The full suite passes on Linux locally apart from tests/integration/test_preset_update_workflow.py::test_preset_update_cli_contract, which fails on this checkout on a rich-formatted usage error in CLI output and is not touched by this change. Windows and macOS are confirmed by CI only.

Not changed, with reasons:

Forced generic refresh overwrites an unowned resource without a rollback snapshot: --force is explicit consent to overwrite, and the rollback gap needs a registry-save failure on top of that.
Native refresh leaves a rewritten recorded resource with its old hash after a registry-save failure: rare, and the result is a "modified resource" warning that --force clears; main has no native refresh rollback or hashes.
SKILL.md/reference.md passes preflight: it needs an unrealistic declaration and already produces a "Skipped resource" warning at delivery.
extension disable on the generic integration deletes edited resource copies: this matches extension remove and update, which delete recorded resources including edits by design (design/extension.md, skill resource lifecycle table). A guard on disable alone would be inconsistent.

I did not resolve any review threads.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, supervised: each finding was discussed and decided by @markuswondrak, and the commits carry Assisted-by: trailers). The agent investigated the findings, wrote and ran the code and tests above on Linux, and drafted this summary. No testing was done on Windows or macOS.

Copilot

🟡 Changes recommended

Critical rollback and cleanup flaws can overwrite or delete user files and expose private resource contents through overly broad permissions.

6 open issues

9 resolved since last review

_{Review effort: Max (auto)}

Rollback recreated a deleted private resource at the umask default and chmodded it afterwards, and returned early for identical bytes without restoring the saved mode. Recreate through the mode-at-creation writer and restore the mode before the early return. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

…ces" Skill names are single path components everywhere on main. Nested names only arose because the name producers let "/" through from free-form aliases; that is fixed at the source in the following commit, so cleanup needs no nested-path matching. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

Skill names are one path component everywhere on main, and registry cleanup, ownership and rollback code reject anything else. Free-form aliases such as "speckit.ext/group.alias" let a "/" through, producing a nested skill directory that those consumers then missed one by one (resource cleanup, update backup). Add skill_name_for_command, which collapses separators to hyphens like dots, and use it in the registrar, the extension manager, the hook skill mapping, the install conflict check and the preset skill lookup. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

The temporary file name embedded the destination name, so a valid 240-byte resource file name overflowed the 255-byte component limit and the copy was skipped. Name the temporary file independently of the destination. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

markuswondrak · 2026-11-11T17:01:09Z

Review round 5 (Copilot review 5860980904 on db2fe9d0): fixes pushed, latest commit 25322af6. Dismissed findings, including ones raised again from earlier rounds, are listed with their reasons below.

Fixed:

Restore of generic refresh artifacts (905af01a): a deleted private resource is recreated through the mode-at-creation writer (_write_shared_bytes), so it is never on disk under a wider mode. The saved mode is also restored when the bytes are already identical.
Nested alias names (76f377bb, reverts e7c83d1a): the root cause was that skill names are a single path component everywhere (registry cleanup, ownership, rollback), but free-form aliases such as speckit.ext/group.alias produced a nested directory. A new skill_name_for_command now turns separators into hyphens, and the registrar, extension manager, hook mapping, install conflict check and preset lookup all use it. The update-backup guard is therefore correct as written, and the nested path matching added last round is reverted. One edge is not handled: a nested alias directory created by an older version stays behind after an upgrade.
Temporary filename (25322af6): a regression from 57fd93a1. The temp name embedded the destination name, so a valid 240-byte resource name overflowed the 255-byte limit. It is now fixed-length.

Each fix has a test that fails without it. ruff and markdownlint are clean. The full suite passes on Linux locally apart from test_preset_update_cli_contract, which is unrelated to this change. Windows and macOS are confirmed by CI only.

Dismissed, with reasons:

A forced refresh overwrites an existing resource, then the registry save fails, and rollback does not restore it. This covers the unowned destination, which rollback deletes, and the edited recorded copy, which keeps the package bytes under the old digest. It was raised in rounds 3 and 5.
- --force is explicit consent to overwrite an unowned or edited copy; the PR and docs/integrations/cli.md say so. The only difference on failure is that the file is removed rather than overwritten.
- It needs three things at once: --force, a file already at the destination, and a failed registry write (an I/O error such as a full disk). Re-running recreates the file from the package.
- Fixing it means extending the write observer plus restore logic in rollback code, which has been the source of most findings in this PR. I judged that risk higher than the benefit.
SKILL.md/reference.md passes preflight. It was raised in rounds 3 and 5.
- It needs a declaration nobody would write: a directory named SKILL.md inside the extension.
- Delivery already reports it as a "Skipped resource" warning. Nothing is lost or corrupted, and the generated SKILL.md is intact.
extension disable on the generic integration deletes edited resource copies (round 3, thread since resolved). This is the same behaviour as extension remove and update, which delete recorded resources including edits by design (design/extension.md, skill resource lifecycle table). A guard on disable alone would make resources the one artifact that behaves differently.

I did not resolve any review threads. The two dismissed ones above (1 and 2) have an inline reply.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, supervised: each finding was discussed and decided by @markuswondrak, and the commits carry Assisted-by: trailers). The agent investigated the findings, wrote and ran the code and tests above on Linux, and drafted this summary. No testing was done on Windows or macOS.

Copilot

🟡 Changes recommended

Resource cleanup trusts registry paths without verifying extension ownership, allowing a crafted repository registry to trigger deletion of unrelated files in global skill directories.

3 open issues

5 resolved since last review

_{Review effort: Max (auto)}

Older versions rendered an alias such as speckit.ext/group.alias to speckit-ext/group-alias/SKILL.md. Since skill names are now always one path component, removal, agent unregistration and the update backup no longer looked there and left that skill behind. Keep the old spelling as an extra candidate through legacy_skill_name_for_command, keep the existing containment checks, and prune the parent directory that removal empties. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

record_file's mode-aware copy and _write_shared_bytes each had their own temp-file-and-replace code, and both derived the temporary name from the destination, so a long file name overflowed the component limit in the rollback writer as it had in the copy. Move the writer to _file_changes as atomic_write_bytes (fixed-length temporary name, creation mode narrowed by the umask or exact, optional pre-replace check) and use it from both callers. Assisted-by: pi coding agent (model: claude-sonnet-5.5, supervised)

markuswondrak · 2026-11-11T17:38:08Z

Review round 6 (Copilot review 5862691162 on 25322af6): fixes pushed, latest commit e58c9659. Dismissed findings, including ones raised again from earlier rounds, are listed with their reasons below.

Fixed:

Skills written under the old nested alias spelling (518ba741): last round's naming change (a skill name is always one path component) left a skill that an older version rendered for an alias like speckit.ext/group.alias at speckit-ext/group-alias/SKILL.md behind on removal. Removal, agent unregistration and the update backup now also look at that old spelling through legacy_skill_name_for_command, keep the existing containment checks, and prune the directory removal empties. The regression starts from the old layout and covers remove and update rollback. This also covers the open thread about backing up nested alias directories for update rollback.
Long resource file names in rollback (e58c9659): _write_shared_bytes derived its temporary name from the destination, so restoring a resource with a 250-byte name failed with ENAMETOOLONG and could leave the deleted copy, including user edits, unrestored. record_file(mode=) and _write_shared_bytes now share one writer, atomic_write_bytes in integrations/_file_changes.py, with a fixed-length temporary name, umask-narrowed or exact mode, and the existing pre-replace safety check. This removes the duplicate writer I added earlier. A 250-byte rollback regression and unit tests for the shared writer are added.

Each fix has a test that fails without it. ruff is clean. The full suite passes on Linux locally apart from tests/integration/test_preset_update_workflow.py::test_preset_update_cli_contract, which is unrelated to this change. Windows and macOS are confirmed by CI only.

Dismissed, with reasons (unchanged from round 5, still open on the PR):

A forced refresh overwrites an existing resource, then the registry save fails, and rollback does not restore it (the unowned copy is deleted, an edited recorded copy keeps the package bytes under the old digest).
- --force is explicit consent to overwrite an unowned or edited copy; the PR and docs/integrations/cli.md say so. The only difference on failure is that the file is removed rather than overwritten.
- It needs three things at once: --force, a file already at the destination, and a failed registry write (an I/O error such as a full disk). Re-running recreates the file from the package.
- Fixing it means extending the write observer plus restore logic in rollback code, which has been the source of most findings in this PR. I judged that risk higher than the benefit.
SKILL.md/reference.md passes preflight.
- It needs a declaration nobody would write: a directory named SKILL.md inside the extension.
- Delivery already reports it as a "Skipped resource" warning. Nothing is lost or corrupted, and the generated SKILL.md is intact.
extension disable on the generic integration deletes edited resource copies. This is the same behaviour as extension remove and update, which delete recorded resources including edits by design (design/extension.md, skill resource lifecycle table). A guard on disable alone would make resources the one artifact that behaves differently.

No finding was newly dismissed this round, so there are no new inline replies. I did not resolve any review threads.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, supervised: each finding was discussed and decided by @markuswondrak, and the commits carry Assisted-by: trailers). The agent investigated the findings, wrote and ran the code and tests above on Linux, and drafted this summary. No testing was done on Windows or macOS.

Copilot

🔵 Needs a closer look

A malicious project registry can cause extension removal to delete unrelated home-scoped skill files because cleanup forces deletion without validating resource ownership or hashes.

No open issues 🎉

3 resolved since last review

_{Review effort: Max (auto)}

Copilot

🟡 Changes recommended

Unresolved critical findings allow normalized skill-name collisions to overwrite other extensions’ output and registry-driven cleanup to delete protected project files such as .git/config.

5 open issues

Reject normalized skill-name collisions · New
Validate skill names before forced resource cleanup · New
Normalize aliases in command invocation builders · New
Clean up resources at historical generic roots · New
Rebase cleanup paths onto the project root · New

_{Review effort: Max}

+    short_name = command_name
+    if short_name.startswith("speckit."):
+        short_name = short_name[len("speckit."):]
+    return "speckit-" + short_name.replace(".", "-").replace("/", "-")


+        if (
+            not key or "\\" in key or path.is_absolute()
+            or PureWindowsPath(key).drive or path.as_posix() != key
+            or ".." in path.parts or not set(path.parts[:-1]) & allowed_skill_names


-        short_name = short_name.replace(".", "-")
-
-        return f"speckit-{short_name}"
+        return skill_name_for_command(cmd_name)


+                    retire_skill_resources(
+                        self.project_root, extension_id, resource_hashes, set(skills),
+                        skills_dir=directory,


+                if isinstance(resource_hashes, dict):
+                    retire_skill_resources(
+                        self.project_root, extension_id, resource_hashes, set(skills),
+                        skills_dir=directory,


mnriem · 2026-11-11T19:56:32Z

Please address Copilot feedback and fix test errors

markuswondrak requested a review from mnriem as a code owner November 11, 2026 08:42

Copilot AI auto review requested due to automatic review settings November 11, 2026 08:42

Copilot started reviewing on behalf of markuswondrak November 11, 2026 08:43

View session