mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
refactor: move extension command handlers to extensions/_commands.py (PR-7/8) (#3014)
* refactor: move extension command handlers to extensions/_commands.py (PR-7/8) Convert the flat extensions.py module into an extensions/ package and extract all extension_app and catalog_app command handlers plus their private helpers (_resolve_installed_extension, _resolve_catalog_extension, _print_extension_info) out of __init__.py into the new extensions/_commands.py, mirroring the domain-dir layout used for presets/_commands.py (PR-6) and integrations/_commands.py (PR-5). - extensions.py -> extensions/__init__.py (pure rename, 99%); intra-module relative imports bumped from `.x` to `..x` since they reference root siblings. - Root helpers (_require_specify_project, _locate_bundled_extension, load_init_options, _display_project_path) are reached through thin shims that re-fetch from the parent package at call time, so test monkeypatching of specify_cli.<helper> keeps working unchanged. - __init__.py drops ~1444 lines (3511 -> 2067); CLI surface preserved via register(app). No behavior change. Full suite failure set is identical before/after (82 pre-existing env failures, 0 new). * fix(extensions): preserve per-command path in update backup for skills agents Skills agents (extension == "/SKILL.md") name every command file SKILL.md, each in its own per-command subdir (e.g. speckit-plan/SKILL.md). The update backup keyed the backup path on cmd_file.name alone, so all of an agent's skill files collided onto a single backup path — each shutil.copy2 overwrote the previous one, and rollback restored one skill's content over all the others, corrupting or losing the rest. Mirror the real on-disk layout by using cmd_file.relative_to(commands_dir), keeping each backup path unique. This also makes backed_up_command_files values unique so restore copies the correct content back to each command. Add a regression test asserting two distinct skill files survive a backup -> failed-update -> rollback cycle with their own content. * style(extensions): use yaml.safe_dump when writing catalog config The catalog add/remove handlers wrote the integration catalog config with yaml.dump. Switch to yaml.safe_dump to align with the SafeDumper used by the presets commands and to refuse emitting !!python/object tags if a non-basic value ever reaches the config dict. Output is unchanged for the current basic-type payload (str/int/bool/dict/ list) — this is a defensive/consistency change, not a behavioral fix. * fix(extensions): correct _print_cli_warning import path in skill registration register_enabled_extensions_for_agent imported _print_cli_warning from `.` (the extensions package), but the helper lives in the parent specify_cli package. The wrong level raised ImportError inside the error handlers, aborting extension/skill registration on the first failure instead of warning and continuing. Use `..` to match the other parent-package imports. * fix(extensions): escape untrusted values in Rich markup output User-provided arguments and extension/catalog metadata (names, descriptions, versions, IDs, paths) were interpolated into Rich markup strings without escaping. Values containing markup sequences (e.g. [red]...) would be parsed as markup, allowing output injection that could corrupt or mislead CLI messages. Wrap all such interpolations with rich.markup.escape across the extension/catalog command handlers: list, search, info (_print_extension_info), add (including --dev paths), remove, enable, disable, set-priority, update, and the ambiguous-match resolvers (error strings and Table rows). Reuse the already-computed safe_extension where available. Escaping is a no-op for benign strings, so normal output is unchanged. * Prevent Rich markup injection in extension CLI output User-controlled catalog URLs and extension IDs are rendered through Rich-enabled console paths, so every remaining output-only interpolation now escapes markup while leaving stored values and filesystem behavior unchanged. Regression tests cover catalog add, install hints, remove hints, and state command messages with bracketed markup-like values. * Prevent markup injection from exception text Rich markup remains enabled for styled CLI messages, so exception text and config path labels must be escaped before rendering. YAML parser errors, URL validation failures, download errors, and extension validation errors can include user-controlled catalog or manifest values. Constraint: Preserve existing exception handling and user-facing error paths Rejected: Disable Rich markup for these messages | existing output intentionally uses markup for labels and styling Confidence: high Scope-risk: narrow Directive: Escape user-controlled exception text before interpolating into Rich-rendered strings Tested: .venv/bin/python -m pytest tests/test_extensions.py -q Co-authored-by: OmX <omx@oh-my-codex.dev> * Prevent path and manifest review regressions Catalog path labels are rendered through Rich markup and downloaded update manifests are trusted long enough to validate extension IDs. Escape displayed project paths before rendering, and reject non-mapping extension.yml payloads before ID validation so bad archives fail with a clear rollback reason. --------- Co-authored-by: OmX <omx@oh-my-codex.dev>
This commit is contained in:
File diff suppressed because it is too large
Load Diff
@@ -26,11 +26,11 @@ import yaml
|
||||
from packaging import version as pkg_version
|
||||
from packaging.specifiers import InvalidSpecifier, SpecifierSet
|
||||
|
||||
from ._init_options import is_ai_skills_enabled
|
||||
from ._invocation_style import is_dollar_skills_agent, is_slash_skills_agent
|
||||
from ._utils import dump_frontmatter, relative_extension_path_violation
|
||||
from .catalogs import CatalogEntry as BaseCatalogEntry
|
||||
from .catalogs import CatalogStackBase
|
||||
from .._init_options import is_ai_skills_enabled
|
||||
from .._invocation_style import is_dollar_skills_agent, is_slash_skills_agent
|
||||
from .._utils import dump_frontmatter, relative_extension_path_violation
|
||||
from ..catalogs import CatalogEntry as BaseCatalogEntry
|
||||
from ..catalogs import CatalogStackBase
|
||||
|
||||
_FALLBACK_CORE_COMMAND_NAMES = frozenset(
|
||||
{
|
||||
@@ -905,7 +905,7 @@ class ExtensionManager:
|
||||
be created due to symlink, containment, or permission issues so
|
||||
that callers can fall back gracefully.
|
||||
"""
|
||||
from . import (
|
||||
from .. import (
|
||||
_print_cli_warning,
|
||||
load_init_options,
|
||||
resolve_active_skills_dir,
|
||||
@@ -948,7 +948,7 @@ class ExtensionManager:
|
||||
if not isinstance(selected_ai, str) or not selected_ai:
|
||||
return _ensure_usable(skills_dir)
|
||||
|
||||
from .agents import CommandRegistrar
|
||||
from ..agents import CommandRegistrar
|
||||
|
||||
registrar = CommandRegistrar()
|
||||
agent_config = registrar.AGENT_CONFIGS.get(selected_ai)
|
||||
@@ -985,9 +985,9 @@ class ExtensionManager:
|
||||
if not skills_dir:
|
||||
return []
|
||||
|
||||
from . import load_init_options
|
||||
from .agents import CommandRegistrar
|
||||
from .integrations import get_integration
|
||||
from .. import load_init_options
|
||||
from ..agents import CommandRegistrar
|
||||
from ..integrations import get_integration
|
||||
|
||||
written: List[str] = []
|
||||
opts = load_init_options(self.project_root)
|
||||
@@ -1201,7 +1201,7 @@ class ExtensionManager:
|
||||
shutil.rmtree(skill_subdir)
|
||||
else:
|
||||
# Fallback: scan all possible agent skills directories
|
||||
from . import AGENT_CONFIG, DEFAULT_SKILLS_DIR
|
||||
from .. import AGENT_CONFIG, DEFAULT_SKILLS_DIR
|
||||
|
||||
candidate_dirs: set[Path] = set()
|
||||
for cfg in AGENT_CONFIG.values():
|
||||
@@ -1616,7 +1616,7 @@ class ExtensionManager:
|
||||
# Resolve the skills directory for the specific agent so cleanup is
|
||||
# agent-scoped and does not depend on the currently-active agent in
|
||||
# init-options. Use the same helper that extension install uses.
|
||||
from . import _get_skills_dir as resolve_skills_dir
|
||||
from .. import _get_skills_dir as resolve_skills_dir
|
||||
|
||||
agent_skills_dir = resolve_skills_dir(self.project_root, agent_name)
|
||||
|
||||
@@ -1692,7 +1692,7 @@ class ExtensionManager:
|
||||
if not agent_name:
|
||||
return
|
||||
|
||||
from . import load_init_options
|
||||
from .. import load_init_options
|
||||
|
||||
registrar = CommandRegistrar()
|
||||
agent_config = registrar.AGENT_CONFIGS.get(agent_name)
|
||||
@@ -1750,7 +1750,7 @@ class ExtensionManager:
|
||||
# Skills are a companion artifact. If command registration
|
||||
# already succeeded, still persist it so later cleanup can
|
||||
# find those command files.
|
||||
from . import _print_cli_warning
|
||||
from .. import _print_cli_warning
|
||||
|
||||
_print_cli_warning(
|
||||
"register extension skills for",
|
||||
@@ -1775,7 +1775,7 @@ class ExtensionManager:
|
||||
except Exception as ext_err:
|
||||
# Best-effort per extension: warn and move on so a single bad
|
||||
# extension cannot silently drop the others. See #2950.
|
||||
from . import _print_cli_warning
|
||||
from .. import _print_cli_warning
|
||||
|
||||
_print_cli_warning(
|
||||
"register extension artifacts for",
|
||||
@@ -1882,31 +1882,31 @@ class CommandRegistrar:
|
||||
"""
|
||||
|
||||
# Re-export AGENT_CONFIGS at class level for direct attribute access
|
||||
from .agents import CommandRegistrar as _AgentRegistrar
|
||||
from ..agents import CommandRegistrar as _AgentRegistrar
|
||||
|
||||
AGENT_CONFIGS = _AgentRegistrar.AGENT_CONFIGS
|
||||
|
||||
def __init__(self):
|
||||
from .agents import CommandRegistrar as _Registrar
|
||||
from ..agents import CommandRegistrar as _Registrar
|
||||
|
||||
self._registrar = _Registrar()
|
||||
|
||||
# Delegate static/utility methods
|
||||
@staticmethod
|
||||
def parse_frontmatter(content: str) -> tuple[dict, str]:
|
||||
from .agents import CommandRegistrar as _Registrar
|
||||
from ..agents import CommandRegistrar as _Registrar
|
||||
|
||||
return _Registrar.parse_frontmatter(content)
|
||||
|
||||
@staticmethod
|
||||
def render_frontmatter(fm: dict) -> str:
|
||||
from .agents import CommandRegistrar as _Registrar
|
||||
from ..agents import CommandRegistrar as _Registrar
|
||||
|
||||
return _Registrar.render_frontmatter(fm)
|
||||
|
||||
@staticmethod
|
||||
def _write_copilot_prompt(project_root, cmd_name: str) -> None:
|
||||
from .agents import CommandRegistrar as _Registrar
|
||||
from ..agents import CommandRegistrar as _Registrar
|
||||
|
||||
_Registrar.write_copilot_prompt(project_root, cmd_name)
|
||||
|
||||
@@ -2857,7 +2857,7 @@ class HookExecutor:
|
||||
instance to avoid repeated filesystem reads during hook rendering.
|
||||
"""
|
||||
if self._init_options_cache is None:
|
||||
from . import load_init_options
|
||||
from .. import load_init_options
|
||||
|
||||
payload = load_init_options(self.project_root)
|
||||
self._init_options_cache = payload if isinstance(payload, dict) else {}
|
||||
@@ -2896,7 +2896,7 @@ class HookExecutor:
|
||||
if kimi_skill_mode and skill_name:
|
||||
return f"/skill:{skill_name}"
|
||||
if cline_mode:
|
||||
from .integrations.cline import format_cline_command_name
|
||||
from ..integrations.cline import format_cline_command_name
|
||||
|
||||
return f"/{format_cline_command_name(command_id)}"
|
||||
|
||||
1556
src/specify_cli/extensions/_commands.py
Normal file
1556
src/specify_cli/extensions/_commands.py
Normal file
File diff suppressed because it is too large
Load Diff
@@ -1315,6 +1315,78 @@ class TestIntegrationCatalogDiscoveryCLI:
|
||||
assert extension_list.exit_code == 0, extension_list.output
|
||||
assert "Config: .specify/extension-catalogs.yml" in extension_list.output
|
||||
|
||||
def test_extension_catalog_add_rejects_non_mapping_config_root(self, tmp_path):
|
||||
project = self._make_project(tmp_path)
|
||||
cfg_path = project / ".specify" / "extension-catalogs.yml"
|
||||
cfg_path.write_text("- not\n- a\n- mapping\n", encoding="utf-8")
|
||||
|
||||
result = self._invoke([
|
||||
"extension", "catalog", "add",
|
||||
"https://example.com/extension-catalog.yml",
|
||||
"--name", "demo-extensions",
|
||||
], project)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
output = _normalize_cli_output(result.output)
|
||||
assert "Invalid catalog config .specify/extension-catalogs.yml" in output
|
||||
assert "expected a YAML mapping at the root" in output
|
||||
assert "AttributeError" not in output
|
||||
|
||||
def test_extension_catalog_remove_rejects_non_mapping_config_root(self, tmp_path):
|
||||
project = self._make_project(tmp_path)
|
||||
cfg_path = project / ".specify" / "extension-catalogs.yml"
|
||||
cfg_path.write_text("- not\n- a\n- mapping\n", encoding="utf-8")
|
||||
|
||||
result = self._invoke(["extension", "catalog", "remove", "demo"], project)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
output = _normalize_cli_output(result.output)
|
||||
assert "Invalid catalog config .specify/extension-catalogs.yml" in output
|
||||
assert "expected a YAML mapping at the root" in output
|
||||
assert "AttributeError" not in output
|
||||
|
||||
def test_extension_catalog_add_escapes_catalog_name_markup(self, tmp_path):
|
||||
project = self._make_project(tmp_path)
|
||||
catalog_name = "[red]demo[/red]"
|
||||
|
||||
result = self._invoke([
|
||||
"extension", "catalog", "add",
|
||||
"https://example.com/extension-catalog.yml",
|
||||
"--name", catalog_name,
|
||||
], project)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
output = _normalize_cli_output(result.output)
|
||||
assert f"Added catalog '{catalog_name}'" in output
|
||||
|
||||
def test_extension_catalog_remove_escapes_catalog_name_markup(self, tmp_path):
|
||||
project = self._make_project(tmp_path)
|
||||
catalog_name = "[red]demo[/red]"
|
||||
cfg_path = project / ".specify" / "extension-catalogs.yml"
|
||||
cfg_path.write_text(
|
||||
yaml.safe_dump(
|
||||
{
|
||||
"catalogs": [
|
||||
{
|
||||
"name": catalog_name,
|
||||
"url": "https://example.com/extension-catalog.yml",
|
||||
"priority": 10,
|
||||
"install_allowed": False,
|
||||
"description": "",
|
||||
}
|
||||
]
|
||||
},
|
||||
sort_keys=False,
|
||||
),
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
result = self._invoke(["extension", "catalog", "remove", catalog_name], project)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
output = _normalize_cli_output(result.output)
|
||||
assert f"Removed catalog '{catalog_name}'" in output
|
||||
|
||||
# -- search ------------------------------------------------------------
|
||||
|
||||
def test_search_lists_all(self, tmp_path, monkeypatch):
|
||||
|
||||
@@ -107,3 +107,51 @@ def test_extension_update_rollback_corrupted_config(project_dir, monkeypatch):
|
||||
assert isinstance(restored_config, dict)
|
||||
assert "hooks" in restored_config
|
||||
assert restored_config["hooks"] == {}
|
||||
|
||||
|
||||
def test_extension_update_skills_backup_no_collision(project_dir, monkeypatch):
|
||||
"""Regression: skills agents name every command file SKILL.md (one per
|
||||
command subdirectory). Backup must keep the per-command path so rollback
|
||||
restores each skill's own content instead of overwriting them onto a
|
||||
single backup path."""
|
||||
monkeypatch.chdir(project_dir)
|
||||
|
||||
config_path = project_dir / ".specify" / "extensions.yml"
|
||||
config_path.write_text(yaml.dump({"installed": ["test-ext"], "hooks": {}}))
|
||||
|
||||
# Two skill command files with DISTINCT content, mirroring the claude
|
||||
# skills layout (.claude/skills/<name>/SKILL.md).
|
||||
skills_root = project_dir / ".claude" / "skills"
|
||||
plan_file = skills_root / "speckit-plan" / "SKILL.md"
|
||||
tasks_file = skills_root / "speckit-tasks" / "SKILL.md"
|
||||
plan_file.parent.mkdir(parents=True)
|
||||
tasks_file.parent.mkdir(parents=True)
|
||||
plan_file.write_text("PLAN CONTENT")
|
||||
tasks_file.write_text("TASKS CONTENT")
|
||||
|
||||
monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: [{"id": "test-ext", "name": "Test Ext", "version": "1.0.0"}])
|
||||
monkeypatch.setattr(ExtensionRegistry, "get", lambda self, ext_id: {
|
||||
"version": "1.0.0",
|
||||
"enabled": True,
|
||||
"registered_commands": {"claude": ["speckit.plan", "speckit.tasks"]},
|
||||
})
|
||||
monkeypatch.setattr(ExtensionCatalog, "get_extension_info", lambda self, ext_id: {"id": "test-ext", "name": "Test Ext", "version": "1.1.0", "download_url": "https://example.com/ext.zip"})
|
||||
|
||||
# Fail at download (step 5, after the command backup in step 3). Delete the
|
||||
# originals first to simulate an install clobbering them, forcing rollback
|
||||
# to rely entirely on the backups.
|
||||
def mock_download_fail(self, ext_id):
|
||||
plan_file.unlink()
|
||||
tasks_file.unlink()
|
||||
raise Exception("Download failed")
|
||||
|
||||
monkeypatch.setattr(ExtensionCatalog, "download_extension", mock_download_fail)
|
||||
monkeypatch.setattr("typer.confirm", lambda _: True)
|
||||
|
||||
result = runner.invoke(app, ["extension", "update", "test-ext"], obj={"project_root": project_dir})
|
||||
|
||||
assert result.exit_code == 1
|
||||
# Rollback must restore EACH skill's own content, not a single collided copy.
|
||||
assert plan_file.exists() and tasks_file.exists()
|
||||
assert plan_file.read_text() == "PLAN CONTENT"
|
||||
assert tasks_file.read_text() == "TASKS CONTENT"
|
||||
|
||||
@@ -4670,6 +4670,177 @@ class TestExtensionIgnore:
|
||||
class TestExtensionAddCLI:
|
||||
"""CLI integration tests for extension add command."""
|
||||
|
||||
def test_catalog_add_escapes_url_markup(self, tmp_path):
|
||||
"""Catalog add should render user-supplied URLs literally."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
url = "https://example.com/[red]catalog[/red].json"
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"extension",
|
||||
"catalog",
|
||||
"add",
|
||||
url,
|
||||
"--name",
|
||||
"community",
|
||||
],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert f"URL: {url}" in result.output
|
||||
|
||||
def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path):
|
||||
"""Catalog add's saved-path label should render literally under Rich."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
display_path = "project[red]/.specify/extension-catalogs.yml"
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch("specify_cli.extensions._commands._display_project_path", return_value=display_path):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"extension",
|
||||
"catalog",
|
||||
"add",
|
||||
"https://example.com/catalog.json",
|
||||
"--name",
|
||||
"community",
|
||||
],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert f"Config saved to {display_path}" in result.output
|
||||
|
||||
def test_catalog_list_escapes_config_path_markup(self, tmp_path):
|
||||
"""Catalog list's config-path label should render literally under Rich."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
import yaml
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
specify_dir = project_dir / ".specify"
|
||||
specify_dir.mkdir()
|
||||
(specify_dir / "extension-catalogs.yml").write_text(
|
||||
yaml.safe_dump(
|
||||
{
|
||||
"catalogs": [
|
||||
{
|
||||
"name": "community",
|
||||
"url": "https://example.com/catalog.json",
|
||||
"priority": 10,
|
||||
"install_allowed": False,
|
||||
}
|
||||
]
|
||||
}
|
||||
),
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
display_path = "project[red]/.specify/extension-catalogs.yml"
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch("specify_cli.extensions._commands._display_project_path", return_value=display_path):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "catalog", "list"],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert f"Config: {display_path}" in result.output
|
||||
|
||||
def test_catalog_add_escapes_config_read_exception_markup(self, tmp_path):
|
||||
"""Catalog config parse errors can include user-controlled file content."""
|
||||
import yaml
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
specify_dir = project_dir / ".specify"
|
||||
specify_dir.mkdir()
|
||||
(specify_dir / "extension-catalogs.yml").write_text("[red]bad[/red]", encoding="utf-8")
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch(
|
||||
"specify_cli.extensions._commands.yaml.safe_load",
|
||||
side_effect=yaml.YAMLError("bad [red]catalog[/red] yaml"),
|
||||
):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"extension",
|
||||
"catalog",
|
||||
"add",
|
||||
"https://example.com/catalog.json",
|
||||
"--name",
|
||||
"community",
|
||||
],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "bad [red]catalog[/red]" in result.output
|
||||
assert "yaml" in result.output
|
||||
|
||||
def test_catalog_add_escapes_url_validation_exception_markup(self, tmp_path):
|
||||
"""URL validation errors may include user-controlled URL text."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch.object(
|
||||
ExtensionCatalog,
|
||||
"_validate_catalog_url",
|
||||
side_effect=ValidationError("bad [red]url[/red]"),
|
||||
):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"extension",
|
||||
"catalog",
|
||||
"add",
|
||||
"https://example.com/[red]catalog[/red].json",
|
||||
"--name",
|
||||
"community",
|
||||
],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "bad [red]url[/red]" in result.output
|
||||
|
||||
def test_add_dev_links_copilot_agent_when_supported(
|
||||
self, extension_dir, project_dir, temp_dir
|
||||
):
|
||||
@@ -4883,6 +5054,85 @@ class TestExtensionAddCLI:
|
||||
f"confirm must precede spinner, got: {call_order}"
|
||||
assert result.exit_code == 0 # user declined → clean exit
|
||||
|
||||
def test_add_status_escapes_extension_markup(self, tmp_path):
|
||||
"""User-controlled extension names must not be parsed as Rich markup."""
|
||||
from rich.markup import escape as escape_markup
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import MagicMock, patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
status_messages: list[str] = []
|
||||
|
||||
def record_status(message, *args, **kwargs):
|
||||
status_messages.append(message)
|
||||
return MagicMock()
|
||||
|
||||
extension_name = "[red]bad[/red]"
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch("specify_cli.console.status", side_effect=record_status):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "add", extension_name, "--dev"],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1
|
||||
assert status_messages == [
|
||||
f"[cyan]Installing extension: {escape_markup(extension_name)}[/cyan]"
|
||||
]
|
||||
|
||||
def test_add_post_install_hint_escapes_manifest_id_markup(self, tmp_path):
|
||||
"""Extension IDs printed in Rich-rendered hints must stay literal."""
|
||||
import io
|
||||
from types import SimpleNamespace
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
class FakeResponse(io.BytesIO):
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb):
|
||||
return False
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
manifest_id = "[red]bad[/red]"
|
||||
|
||||
def fake_install_from_zip(self_obj, zip_path, speckit_version, priority=10, force=False):
|
||||
return SimpleNamespace(
|
||||
id=manifest_id,
|
||||
name="Bad Extension",
|
||||
version="1.0.0",
|
||||
description="Test extension",
|
||||
warnings=[],
|
||||
commands=[],
|
||||
hooks=[],
|
||||
)
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch("typer.confirm", return_value=True), \
|
||||
patch("specify_cli.authentication.http.open_url", return_value=FakeResponse(b"zip-bytes")), \
|
||||
patch.object(ExtensionManager, "install_from_zip", fake_install_from_zip), \
|
||||
patch.object(ExtensionRegistry, "get", return_value={}):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "add", "bad", "--from", "https://example.com/ext.zip"],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert ".specify/extensions/[red]bad[/red]/" in result.output
|
||||
|
||||
def test_add_from_url_cancel_exits_cleanly(self, tmp_path):
|
||||
"""Declining the --from <url> confirmation should exit with code 0."""
|
||||
from typer.testing import CliRunner
|
||||
@@ -4905,6 +5155,131 @@ class TestExtensionAddCLI:
|
||||
assert result.exit_code == 0
|
||||
assert "Cancelled" in result.output
|
||||
|
||||
def test_add_from_url_escapes_download_exception_markup(self, tmp_path):
|
||||
"""Download errors can include user-controlled URL text."""
|
||||
import urllib.error
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch("typer.confirm", return_value=True), \
|
||||
patch(
|
||||
"specify_cli.authentication.http.open_url",
|
||||
side_effect=urllib.error.URLError("bad [red]download[/red]"),
|
||||
):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"extension",
|
||||
"add",
|
||||
"my-ext",
|
||||
"--from",
|
||||
"https://example.com/[red]ext[/red].zip",
|
||||
],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "https://example.com/[red]ext[/red].zip" in result.output
|
||||
assert "bad [red]download[/red]" in result.output
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("exc_type", "label"),
|
||||
[
|
||||
(ValidationError, "Validation Error"),
|
||||
(CompatibilityError, "Compatibility Error"),
|
||||
(ExtensionError, "Error"),
|
||||
],
|
||||
)
|
||||
def test_add_exception_handlers_escape_markup(self, tmp_path, exc_type, label):
|
||||
"""Extension install exceptions can include manifest-controlled values."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
ext_dir = tmp_path / "ext"
|
||||
ext_dir.mkdir()
|
||||
(ext_dir / "extension.yml").write_text("extension:\n id: test\n", encoding="utf-8")
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch.object(
|
||||
ExtensionManager,
|
||||
"install_from_directory",
|
||||
side_effect=exc_type("bad [red]extension[/red]"),
|
||||
):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "add", str(ext_dir), "--dev"],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert f"{label}:" in result.output
|
||||
assert "bad [red]extension[/red]" in result.output
|
||||
|
||||
def test_add_from_url_uses_cache_tempfile_for_untrusted_extension_name(self, tmp_path):
|
||||
"""The extension argument must not control the downloaded ZIP path."""
|
||||
import io
|
||||
from types import SimpleNamespace
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
class FakeResponse(io.BytesIO):
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb):
|
||||
return False
|
||||
|
||||
project_dir = tmp_path / "test-project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
downloads_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads"
|
||||
installed = {}
|
||||
|
||||
def fake_install_from_zip(self_obj, zip_path, speckit_version, priority=10, force=False):
|
||||
captured_path = Path(zip_path)
|
||||
installed["zip_path"] = captured_path
|
||||
installed["zip_bytes"] = captured_path.read_bytes()
|
||||
return SimpleNamespace(
|
||||
id="escape",
|
||||
name="Escape Test",
|
||||
version="1.0.0",
|
||||
description="Test extension",
|
||||
warnings=[],
|
||||
commands=[],
|
||||
hooks=[],
|
||||
)
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch("typer.confirm", return_value=True), \
|
||||
patch("specify_cli.authentication.http.open_url", return_value=FakeResponse(b"zip-bytes")), \
|
||||
patch.object(ExtensionManager, "install_from_zip", fake_install_from_zip):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "add", "../outside", "--from", "https://example.com/ext.zip"],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0
|
||||
assert installed["zip_bytes"] == b"zip-bytes"
|
||||
assert installed["zip_path"].resolve().is_relative_to(downloads_dir.resolve())
|
||||
assert installed["zip_path"].name.startswith("extension-url-download-")
|
||||
assert not installed["zip_path"].exists()
|
||||
|
||||
|
||||
class TestDownloadExtensionBundled:
|
||||
"""Tests for download_extension handling of bundled extensions."""
|
||||
@@ -5169,6 +5544,62 @@ class TestExtensionUpdateCLI:
|
||||
for cmd_file in command_files:
|
||||
assert cmd_file.exists(), f"Expected command file to be restored after rollback: {cmd_file}"
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("manifest_text", "expected_detail"),
|
||||
[
|
||||
("- not\n- a\n- mapping\n", "YAML mapping"),
|
||||
("extension: []\n", "'extension' mapping"),
|
||||
],
|
||||
)
|
||||
def test_update_rejects_malformed_zip_manifest(
|
||||
self, tmp_path, monkeypatch, manifest_text, expected_detail
|
||||
):
|
||||
"""Downloaded extension.yml shape must be valid before ID validation."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
import zipfile
|
||||
|
||||
fake_home = tmp_path / "home"
|
||||
fake_home.mkdir()
|
||||
monkeypatch.setattr(Path, "home", lambda: fake_home)
|
||||
|
||||
runner = CliRunner()
|
||||
project_dir = tmp_path / "project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
(project_dir / ".claude" / "skills").mkdir(parents=True)
|
||||
|
||||
manager = ExtensionManager(project_dir)
|
||||
v1_dir = self._create_extension_source(tmp_path, "1.0.0")
|
||||
manager.install_from_directory(v1_dir, "0.1.0")
|
||||
original_registry_entry = manager.registry.get("test-ext")
|
||||
|
||||
zip_path = tmp_path / "bad-manifest.zip"
|
||||
with zipfile.ZipFile(zip_path, "w") as zf:
|
||||
zf.writestr("extension.yml", manifest_text)
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch.object(ExtensionCatalog, "get_extension_info", return_value={
|
||||
"id": "test-ext",
|
||||
"name": "Test Extension",
|
||||
"version": "2.0.0",
|
||||
"_install_allowed": True,
|
||||
}), \
|
||||
patch.object(ExtensionCatalog, "download_extension", return_value=zip_path):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "update", "test-ext"],
|
||||
input="y\n",
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "Invalid extension manifest in downloaded archive" in result.output
|
||||
assert expected_detail in result.output
|
||||
assert "AttributeError" not in result.output
|
||||
assert ExtensionManager(project_dir).registry.get("test-ext") == original_registry_entry
|
||||
|
||||
|
||||
class TestExtensionListCLI:
|
||||
"""Test extension list CLI output format."""
|
||||
@@ -6263,6 +6694,118 @@ class TestExtensionRemoveCLI:
|
||||
|
||||
assert "2 commands" in result.output
|
||||
|
||||
def test_remove_output_escapes_extension_id_markup(self, tmp_path):
|
||||
"""Removal paths and reinstall hints must not parse extension IDs as markup."""
|
||||
from types import SimpleNamespace
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
extension_id = "[red]bad[/red]"
|
||||
installed = [
|
||||
{
|
||||
"id": extension_id,
|
||||
"name": "Bad Extension",
|
||||
"version": "1.0.0",
|
||||
"description": "Test extension",
|
||||
"enabled": True,
|
||||
}
|
||||
]
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch.object(ExtensionManager, "list_installed", return_value=installed), \
|
||||
patch.object(ExtensionManager, "get_extension", return_value=SimpleNamespace(commands=[])), \
|
||||
patch.object(ExtensionRegistry, "get", return_value={"registered_commands": {}, "registered_skills": []}), \
|
||||
patch.object(ExtensionManager, "remove", return_value=True):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "remove", extension_id, "--force"],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert ".specify/extensions/.backup/[red]bad[/red]/" in result.output
|
||||
assert "specify extension add [red]bad[/red]" in result.output
|
||||
|
||||
|
||||
class TestExtensionStateCLI:
|
||||
"""CLI tests for installed extension state commands."""
|
||||
|
||||
def test_enable_registry_error_escapes_extension_id_markup(self, tmp_path):
|
||||
"""Registry-corruption errors should render extension IDs literally."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
extension_id = "[red]bad[/red]"
|
||||
installed = [
|
||||
{
|
||||
"id": extension_id,
|
||||
"name": "Bad Extension",
|
||||
"version": "1.0.0",
|
||||
"description": "Test extension",
|
||||
"enabled": False,
|
||||
}
|
||||
]
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch.object(ExtensionManager, "list_installed", return_value=installed), \
|
||||
patch.object(ExtensionRegistry, "get", return_value=None):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "enable", extension_id],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "Extension '[red]bad[/red]' not found in registry" in result.output
|
||||
|
||||
def test_disable_reenable_hint_escapes_extension_id_markup(self, tmp_path):
|
||||
"""Disable success hints should not parse extension IDs as markup."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
project_dir = tmp_path / "project"
|
||||
project_dir.mkdir()
|
||||
(project_dir / ".specify").mkdir()
|
||||
|
||||
extension_id = "[red]bad[/red]"
|
||||
installed = [
|
||||
{
|
||||
"id": extension_id,
|
||||
"name": "Bad Extension",
|
||||
"version": "1.0.0",
|
||||
"description": "Test extension",
|
||||
"enabled": True,
|
||||
}
|
||||
]
|
||||
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir), \
|
||||
patch.object(ExtensionManager, "list_installed", return_value=installed), \
|
||||
patch.object(ExtensionRegistry, "get", return_value={"enabled": True}), \
|
||||
patch.object(ExtensionRegistry, "update", return_value=None), \
|
||||
patch.object(HookExecutor, "get_project_config", return_value={}):
|
||||
result = runner.invoke(
|
||||
app,
|
||||
["extension", "disable", extension_id],
|
||||
catch_exceptions=True,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert "specify extension enable [red]bad[/red]" in result.output
|
||||
|
||||
|
||||
class TestClineExtensionHyphenation:
|
||||
"""Test that Cline integration uses hyphenated commands and frontmatter references."""
|
||||
|
||||
Reference in New Issue
Block a user