Skip to content

feat(extensions): copy declared resources into extension skills - #5383

Open
markuswondrak wants to merge 3 commits into
github:mainfrom
markuswondrak:feat/5366-extension-skill-resources
Open

markuswondrak wants to merge 3 commits into
github:mainfrom
markuswondrak:feat/5366-extension-skill-resources

Conversation

@markuswondrak

@markuswondrak markuswondrak commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

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):

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 AI auto review requested due to automatic review settings November 11, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

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)

Comment thread src/specify_cli/extensions/__init__.py Resolved
Comment thread src/specify_cli/extensions/__init__.py Resolved
Comment thread src/specify_cli/extensions/__init__.py Resolved
Comment thread src/specify_cli/extensions/__init__.py Resolved
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)
Copilot AI auto review requested due to automatic review settings November 11, 2026 11:15
@markuswondrak

Copy link
Copy Markdown
Contributor Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

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

Medium severity 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.

Medium severity 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.

Medium severity 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.

Medium severity 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.

Medium severity 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.

Medium severity 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.

Low severity 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)

Comment on lines +37 to +40
#: Root entries a wildcard never selects: the dev-mode render cache holds
#: generated outputs, and the manifest is not a packaged resource.
_WILDCARD_EXCLUDED_DIRS = frozenset({".specify-dev"})
_WILDCARD_EXCLUDED_FILES = frozenset({"extension.yml"})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. Wildcards copy what is in the installed extension directory, and the set of local files there is open-ended (local-config.yml, *-config.local.yml, user notes), so an exclusion list would never be complete. The original and the copy both sit in the agent-accessible worktree. The authoring docs already warn that user files in .specify/extensions/<id>/ can match and recommend explicit paths or narrow patterns.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5), run interactively; the contributor decided to dismiss this finding.

Comment on lines +118 to +122
if candidate.is_symlink():
if spec.match_file(relative) or spec.match_file(relative + "/"):
raise ValidationError(f"Resource match is a symlink: {relative}")
elif name in files and candidate.is_file() and spec.match_file(relative):
candidates.append(relative)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed in code; documented. Install copies the package with symlinks resolved (copytree default, unchanged by this PR), so the linked files already become regular files in the installed extension on main, and resource matching sees them as such. Making resource matching trace links would need a change to install semantics for every extension. docs/extensions/developing.md and design/extension.md now say symlinks in a package are not followed or vetted and that packages should not use them (f74926e).

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5), run interactively; the contributor decided to dismiss this finding and document it.

}
if not selected:
continue
_, skipped = remove_skill_resources(ext_id, root, selected, skill_names)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed. Home resources follow the same trust as the existing Hermes cleanup, and are narrower: HermesIntegration.teardown on main removes the global skills with no ownership record at all, while resource cleanup only deletes under the configured skills directory of an integration that declares home_scoped. A user-local ownership ledger would be new infrastructure for one integration, which is out of scope for this PR.

AI disclosure: posted on behalf of @markuswondrak by the pi coding agent (GitHub Copilot, model: claude-sonnet-5.5), run interactively; the contributor decided to dismiss this finding.

…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)
Copilot AI auto review requested due to automatic review settings November 11, 2026 12:25
@markuswondrak

Copy link
Copy Markdown
Contributor Author

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.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Add extension.yml-controlled skill resource copying with legacy no-copy support

2 participants