fix: rewrite extension-relative subdir paths in generated command bodies#3444
Open
marcelsafin wants to merge 7 commits into
Open
fix: rewrite extension-relative subdir paths in generated command bodies#3444marcelsafin wants to merge 7 commits into
marcelsafin wants to merge 7 commits into
Conversation
Extension command bodies reference bundled files relative to the extension root (agents/, knowledge-base/, templates/, ...). Generated SKILL.md and command files emitted those paths verbatim, so agents resolved them against the workspace root where they do not exist. Add CommandRegistrar.rewrite_extension_paths, which rewrites references to subdirectories that actually exist in the installed extension to .specify/extensions/<id>/..., and call it once in register_commands so every output format and alias gets the fix. commands/, specs/ and dot-directories are never rewritten. Fixes github#2101 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes extension command bodies that reference bundled files via extension-root-relative paths (e.g., agents/..., knowledge-base/...) by rewriting those references to the installed extension location under .specify/extensions/<id>/... during command registration.
Changes:
- Add
CommandRegistrar.rewrite_extension_paths()to rewrite extension-relative subdir paths based on subdirs that actually exist in the installed extension directory. - Apply the extension path rewrite during
register_commands()whenextension_idis set so all output formats benefit. - Add tests covering SKILL.md and markdown registrations, alias reuse, and conservative rewriting behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/specify_cli/agents.py | Adds and wires in extension-relative path rewriting during command registration. |
| tests/test_extensions.py | Adds regression and unit tests validating rewritten extension subdir references and exclusions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+249
to
+253
| text = re.sub( | ||
| r'(^|[\s`"\'(])(?:\.?/)?' + re.escape(subdir) + "/", | ||
| rf"\1.specify/extensions/{extension_id}/{subdir}/", | ||
| text, | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 5c0746b. The pattern now only accepts an optional ./ prefix, so /agents/... is left alone and keeps its leading slash. Regression assertions added to the existing unit test.
Collaborator
|
Please address Copilot feedback |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+251
to
+255
| text = re.sub( | ||
| r'(^|[\s`"\'(])(?:\./)?' + re.escape(subdir) + "/", | ||
| rf"\1.specify/extensions/{extension_id}/{subdir}/", | ||
| text, | ||
| ) |
subdir and extension_id come from filesystem directory names and were interpolated into a re.sub string replacement template. A directory name containing a backslash (e.g. assets\q) would raise re.error: bad escape, aborting command registration even when the body didn't reference it. Use a callable replacement so these values are treated literally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+2514
to
+2530
| def test_rewrite_extension_paths_handles_backslash_in_subdir_name(self, temp_dir): | ||
| """A subdir/extension_id containing backslashes must not raise or be | ||
| interpreted as a regex group reference in the replacement (#2101).""" | ||
| from specify_cli.agents import CommandRegistrar as AgentCommandRegistrar | ||
|
|
||
| ext_dir = temp_dir / "ext-backslash" | ||
| weird_subdir = "assets\\q" | ||
| (ext_dir / weird_subdir).mkdir(parents=True) | ||
|
|
||
| text = f"Read {weird_subdir}/file.md but not /{weird_subdir}/abs.md.\n" | ||
| rewritten = AgentCommandRegistrar.rewrite_extension_paths( | ||
| text, "ext\\1", ext_dir | ||
| ) | ||
|
|
||
| assert f".specify/extensions/ext\\1/{weird_subdir}/file.md" in rewritten | ||
| # absolute paths are still left untouched | ||
| assert f"/{weird_subdir}/abs.md" in rewritten |
Renamed the test's subdir fixture from "assets\\q" to "assets[q]": on Windows, backslash is a path separator, so mkdir would create nested "assets/q" dirs instead of one literally-named directory, and iterdir() would only discover "assets", never exercising the rewrite. extension_id keeps a real backslash/"\\1" since it isn't used to create a directory, still verifying the callable replacement handles it literally. Added a sanity assertion for this assumption. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+688
to
+689
| if extension_id: | ||
| body = self.rewrite_extension_paths(body, extension_id, source_root) |
register_commands() rewrote extension-relative subdir references (agents/, knowledge-base/, etc.) via rewrite_extension_paths(), but _register_extension_skills() - the separate renderer used for active non-native skills agents (e.g. Claude with ai_skills: true) - never called it. Generated SKILL.md files left agents/... and knowledge-base/... unresolved, and mapped the extension's own templates/ through the generic project-level rewrite instead of its installed .specify/extensions/<id>/templates/ location. Reuse the existing rewrite_extension_paths() helper in _register_extension_skills() at the same point register_commands() applies it (before resolve_skill_placeholders' generic rewrite), and add a skills-mode regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # extension-relative subdir references (agents/, knowledge-base/, | ||
| # etc.) to their installed .specify/extensions/<id>/ location | ||
| # before the generic placeholder/path resolution below. | ||
| body = registrar.rewrite_extension_paths(body, manifest.id, extension_dir) |
…paths _unregister_skills() restored extension-backed SKILL.md content via resolve_skill_placeholders() without first calling rewrite_extension_paths(), so removing a preset override that shadowed an extension command restored the bare, unresolvable agents/... and knowledge-base/... references. Carried extension_id/extension_dir through _build_extension_skill_restore_index() and applied the same rewrite used at initial registration before restoring. Found the identical gap in _reconcile_composed_commands()'s non-skill agent path: when a removed preset's command reverts to an extension winner, register_commands_for_non_skill_agents() was called without extension_id, so the rewrite never ran for plain command-file agents either. Passed extension_id through there too. Added regression tests for both restore paths (skills-mode and non-skill-agent command files) in tests/test_presets.py. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n base PresetResolver.resolve_content() read the effective base layer's raw content directly via path.read_text() before composing append/prepend/ wrap overlays on top of it, and its outright-replace shortcut did the same. When that base layer was extension-provided, neither read path applied rewrite_extension_paths(), so composing a preset over an extension command (or an extension winning outright through resolve_content) left bare, unresolvable agents/... and knowledge-base/... references in the composed output. All three call sites (PresetManager._register_commands()'s composed path, _reconcile_composed_commands()'s composed path, and skills-mode reading the .composed file written by either) consume resolve_content's return value, so fixing the read at its source covers command output, skill output, and both initial-install and reconcile flows without threading extension identity through each caller. Tagged extension layers in collect_all_layers() with extension_id/ extension_dir, and added a _read_layer_content() helper in resolve_content() that applies rewrite_extension_paths() whenever a layer carries that extension identity — used at both raw-read sites (outright-replace shortcut and composition base). Composing (append/prepend/wrap) layers are never extension-provided (extensions are always inserted with strategy "replace"), so no other read site needs the rewrite. Added regression tests: a parametrized resolve_content() test covering append/prepend/wrap composing over an extension base, a skills-mode test asserting the composed SKILL.md resolves the extension's subdir references, and a non-skill-agent (Gemini) install-time test matching the reported live repro. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Fixed the remaining composition path in 8c92ead: extension-relative paths are now rewritten when append/prepend/wrap presets compose over an extension base, covering command, skill, initial-registration, and reconciliation outputs. Full suite: 3904 passed, 108 skipped. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2101
Root cause
Extension command bodies reference bundled files relative to the extension root (
agents/control/commander.md,knowledge-base/agent-scores.yaml,templates/kill-report.md). After install those files live under.specify/extensions/<id>/, butregister_commandsemitted the bare paths verbatim. Agents resolve them against the workspace root, where they don't exist.rewrite_project_relative_pathsonly knows aboutmemory/,scripts/andtemplates/, and rewritestemplates/to.specify/templates/even when the extension bundles its own.Fix
New static
CommandRegistrar.rewrite_extension_paths(text, extension_id, extension_dir):subdir/...references to.specify/extensions/<id>/subdir/.... Existence-based discovery keeps it conservative: no false positives on prose, and extensions without a given subdir keep today's behavior.commands/(slash-command sources),specs/(user project artifacts) or dot-directories.register_commandsloop whenextension_idis set, so every output format (SKILL.md, markdown, TOML, YAML) and every alias gets the fix through the shared path.Before:
Read agents/control/commander.md(unresolvable)After:
Read .specify/extensions/echelon/agents/control/commander.mdTests
agents/,knowledge-base/andtemplates/refs;specs/andcommands/stay untouchedFull suite: 3896 passed, 108 skipped.
ruff checkclean.Credit
Approach follows @mbachorik's design in his fork (existence-based subdir discovery, the
specs/exclusion). He offered the takeover in #2101. This PR extracts only the path-rewrite fix; the behavior-vocabulary work in #2103 is untouched.