github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-07-03 12:28:06 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: move URL install confirmation prompt before spinner (#2783) (#2784)

Browse Source 
* fix: move URL install confirmation prompt before spinner (#2783)

The typer.confirm() prompt inside console.status() was overwritten by
Rich's spinner animation, making extension add --from <url> appear hung.

Move URL validation and the default-deny confirmation prompt before the
spinner block so the user can see and respond to the [y/N] prompt.

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix: guard prompt with not dev, escape from_url in Rich markup

Address PR review feedback:
- Gate URL confirmation prompt on 'not dev' so --dev + --from does not
  show a confusing prompt for a URL path that will be ignored.
- Escape from_url with rich.markup.escape() in both the warning panel
  and the download message to prevent markup injection via crafted URLs.

* fix: remove unused import, reuse safe_url, add regression tests

Address second round of PR review:
- Remove unused urllib.request import from URL install path
- Remove redundant re-import of rich.markup.escape; reuse safe_url
  computed before the spinner for download and error messages
- Add test_add_from_url_prompts_before_spinner: asserts typer.confirm
  fires before console.status spinner to prevent #2783 regression
- Add test_add_from_url_cancel_exits_cleanly: asserts declining the
  prompt exits with code 0 and prints Cancelled

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
Manfred Riem
2026-06-01 07:50:03 -05:00
committed by GitHub
parent 3617cd9c02
commit 089feca75f
2 changed files with 100 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
src/specify_cli/__init__.py
View File

@@ -3082,6 +3082,43 @@ def extension_add(
manager = ExtensionManager(project_root)
speckit_version = get_speckit_version()
# Prompt for URL-based installs BEFORE the spinner so the user can
# actually see and respond to the confirmation (the Rich status
# spinner overwrites the typer.confirm prompt line, making it appear
# as though the command is hung).
# Guard with ``not dev`` so that --dev + --from does not show a
# confusing confirmation for a URL that will be ignored.
if from_url and not dev:
from urllib.parse import urlparse
from rich.markup import escape as _escape_markup
parsed = urlparse(from_url)
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost):
console.print("[red]Error:[/red] URL must use HTTPS for security.")
console.print("HTTP is only allowed for localhost URLs.")
raise typer.Exit(1)
safe_url = _escape_markup(from_url)
# Warn about untrusted sources — default-deny confirmation
console.print()
console.print(Panel(
f"[bold]You are installing an extension from an external URL that is not\n"
f"listed in any of your configured extension catalogs.[/bold]\n\n"
f"URL: {safe_url}\n\n"
f"Only install extensions from sources you trust.",
title="[bold yellow]⚠ Untrusted Source[/bold yellow]",
border_style="yellow",
padding=(1, 2),
))
console.print()
confirm = typer.confirm("Continue with installation?", default=False)
if not confirm:
console.print("Cancelled")
raise typer.Exit(0)
try:
with console.status(f"[cyan]Installing extension: {extension}[/cyan]"):
if dev:
@@ -3104,37 +3141,9 @@ def extension_add(
elif from_url:
# Install from URL (ZIP file)
import urllib.request
import urllib.error
from urllib.parse import urlparse
# Validate URL
parsed = urlparse(from_url)
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost):
console.print("[red]Error:[/red] URL must use HTTPS for security.")
console.print("HTTP is only allowed for localhost URLs.")
raise typer.Exit(1)
# Warn about untrusted sources — default-deny confirmation
console.print()
console.print(Panel(
f"[bold]You are installing an extension from an external URL that is not\n"
f"listed in any of your configured extension catalogs.[/bold]\n\n"
f"URL: {from_url}\n\n"
f"Only install extensions from sources you trust.",
title="[bold yellow]⚠ Untrusted Source[/bold yellow]",
border_style="yellow",
padding=(1, 2),
))
console.print()
confirm = typer.confirm("Continue with installation?", default=False)
if not confirm:
console.print("Cancelled")
raise typer.Exit(0)
console.print(f"Downloading from {from_url}...")
console.print(f"Downloading from {safe_url}...")
# Download ZIP to temp location
download_dir = project_root / ".specify" / "extensions" / ".cache" / "downloads"
@@ -3151,7 +3160,7 @@ def extension_add(
# Install from downloaded ZIP
manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority)
except urllib.error.URLError as e:
console.print(f"[red]Error:[/red] Failed to download from {from_url}: {e}")
console.print(f"[red]Error:[/red] Failed to download from {safe_url}: {e}")
raise typer.Exit(1)
finally:
# Clean up downloaded ZIP

61
tests/test_extensions.py
View File

@@ -3807,6 +3807,67 @@ class TestExtensionAddCLI:
assert "bundled with spec-kit" in result.output
assert "reinstall" in result.output.lower()
def test_add_from_url_prompts_before_spinner(self, tmp_path):
"""Confirm prompt for --from <url> must fire before the console.status spinner.
Regression test for #2783: typer.confirm() inside console.status()
was overwritten by the Rich spinner, making the command appear hung.
"""
from typer.testing import CliRunner
from unittest.mock import patch, MagicMock
from specify_cli import app
project_dir = tmp_path / "test-project"
project_dir.mkdir()
(project_dir / ".specify").mkdir()
call_order: list[str] = []
original_status = MagicMock()
def record_status(*args, **kwargs):
call_order.append("spinner")
return original_status
runner = CliRunner()
with patch.object(Path, "cwd", return_value=project_dir), \
patch("specify_cli.console.status", side_effect=record_status), \
patch("typer.confirm", side_effect=lambda *a, **kw: (call_order.append("confirm"), False)[-1]):
result = runner.invoke(
app,
["extension", "add", "my-ext", "--from", "https://example.com/ext.zip"],
catch_exceptions=True,
)
assert "confirm" in call_order, "confirm prompt was never called"
# The confirm must fire BEFORE the spinner is entered
if "spinner" in call_order:
assert call_order.index("confirm") < call_order.index("spinner"), \
f"confirm must precede spinner, got: {call_order}"
assert result.exit_code == 0 # user declined → clean exit
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
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=False):
result = runner.invoke(
app,
["extension", "add", "my-ext", "--from", "https://example.com/ext.zip"],
catch_exceptions=True,
)
assert result.exit_code == 0
assert "Cancelled" in result.output
class TestDownloadExtensionBundled:
"""Tests for download_extension handling of bundled extensions."""