Skip to content

Commit f5e9752

Browse files
committed
Address review feedback: validate script type, handle --flag=value, fix metadata cleanup
- Add _normalize_script_type() to validate script type against SCRIPT_TYPE_CHOICES - Handle --name=value syntax in _parse_integration_options() - Clear init-options.json keys in no-manifest uninstall early-return path - Clear stale metadata between switch teardown and install phases - Add 5 tests covering the new edge cases
1 parent 8a36cd9 commit f5e9752

2 files changed

Lines changed: 159 additions & 3 deletions

File tree

src/specify_cli/__init__.py

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1535,14 +1535,26 @@ def _remove_integration_json(project_root: Path) -> None:
15351535
path.unlink()
15361536

15371537

1538+
def _normalize_script_type(script_type: str, source: str) -> str:
1539+
"""Normalize and validate a script type from CLI/config sources."""
1540+
normalized = script_type.strip().lower()
1541+
if normalized in SCRIPT_TYPE_CHOICES:
1542+
return normalized
1543+
console.print(
1544+
f"[red]Error:[/red] Invalid script type {script_type!r} from {source}. "
1545+
f"Expected one of: {', '.join(sorted(SCRIPT_TYPE_CHOICES.keys()))}."
1546+
)
1547+
raise typer.Exit(1)
1548+
1549+
15381550
def _resolve_script_type(project_root: Path, script_type: str | None) -> str:
15391551
"""Resolve the script type from the CLI flag or init-options.json."""
15401552
if script_type:
1541-
return script_type
1553+
return _normalize_script_type(script_type, "--script")
15421554
opts = load_init_options(project_root)
15431555
saved = opts.get("script")
1544-
if saved:
1545-
return saved
1556+
if isinstance(saved, str) and saved.strip():
1557+
return _normalize_script_type(saved, ".specify/init-options.json")
15461558
return "ps" if os.name == "nt" else "sh"
15471559

15481560

@@ -1673,10 +1685,17 @@ def _parse_integration_options(integration: Any, raw_options: str) -> dict[str,
16731685
while i < len(tokens):
16741686
token = tokens[i]
16751687
name = token.lstrip("-")
1688+
value: str | None = None
1689+
# Handle --name=value syntax
1690+
if "=" in name:
1691+
name, value = name.split("=", 1)
16761692
opt = declared.get(name)
16771693
if opt and opt.is_flag:
16781694
parsed[name.replace("-", "_")] = True
16791695
i += 1
1696+
elif opt and value is not None:
1697+
parsed[name.replace("-", "_")] = value
1698+
i += 1
16801699
elif opt and i + 1 < len(tokens):
16811700
parsed[name.replace("-", "_")] = tokens[i + 1]
16821701
i += 2
@@ -1740,6 +1759,13 @@ def integration_uninstall(
17401759
if not manifest_path.exists():
17411760
console.print(f"[yellow]No manifest found for integration '{key}'. Nothing to uninstall.[/yellow]")
17421761
_remove_integration_json(project_root)
1762+
# Clear integration-related keys from init-options.json
1763+
opts = load_init_options(project_root)
1764+
if opts.get("integration") == key:
1765+
opts.pop("integration", None)
1766+
opts.pop("ai", None)
1767+
opts.pop("ai_skills", None)
1768+
save_init_options(project_root, opts)
17431769
raise typer.Exit(0)
17441770

17451771
manifest = IntegrationManifest.load(key, project_root)
@@ -1820,6 +1846,14 @@ def integration_switch(
18201846
else:
18211847
console.print(f"[dim]No manifest for '{installed_key}' — skipping uninstall phase[/dim]")
18221848

1849+
# Clear stale metadata so a failed Phase 2 doesn't reference the removed integration
1850+
_remove_integration_json(project_root)
1851+
opts = load_init_options(project_root)
1852+
opts.pop("integration", None)
1853+
opts.pop("ai", None)
1854+
opts.pop("ai_skills", None)
1855+
save_init_options(project_root, opts)
1856+
18231857
# Phase 2: Install target integration
18241858
console.print(f"Installing integration: [cyan]{target}[/cyan]")
18251859
manifest = IntegrationManifest(

tests/integrations/test_integration_subcommand.py

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,3 +394,125 @@ def test_install_modify_uninstall_preserves_modified(self, tmp_path):
394394
assert plan_file.read_text(encoding="utf-8") == "# user customization\n"
395395
finally:
396396
os.chdir(old_cwd)
397+
398+
399+
# ── Edge-case fixes ─────────────────────────────────────────────────
400+
401+
402+
class TestScriptTypeValidation:
403+
def test_invalid_script_type_rejected(self, tmp_path):
404+
"""--script with an invalid value should fail with a clear error."""
405+
project = tmp_path / "proj"
406+
project.mkdir()
407+
(project / ".specify").mkdir()
408+
old_cwd = os.getcwd()
409+
try:
410+
os.chdir(project)
411+
result = runner.invoke(app, [
412+
"integration", "install", "claude",
413+
"--script", "bash",
414+
])
415+
finally:
416+
os.chdir(old_cwd)
417+
assert result.exit_code != 0
418+
assert "Invalid script type" in result.output
419+
420+
def test_valid_script_types_accepted(self, tmp_path):
421+
"""Both 'sh' and 'ps' should be accepted."""
422+
project = tmp_path / "proj"
423+
project.mkdir()
424+
(project / ".specify").mkdir()
425+
old_cwd = os.getcwd()
426+
try:
427+
os.chdir(project)
428+
result = runner.invoke(app, [
429+
"integration", "install", "claude",
430+
"--script", "sh",
431+
], catch_exceptions=False)
432+
finally:
433+
os.chdir(old_cwd)
434+
assert result.exit_code == 0
435+
436+
437+
class TestParseIntegrationOptionsEqualsForm:
438+
def test_equals_form_parsed(self):
439+
"""--commands-dir=./x should be parsed the same as --commands-dir ./x."""
440+
from specify_cli import _parse_integration_options
441+
from specify_cli.integrations import get_integration
442+
443+
integration = get_integration("generic")
444+
assert integration is not None
445+
446+
result_space = _parse_integration_options(integration, "--commands-dir ./mydir")
447+
result_equals = _parse_integration_options(integration, "--commands-dir=./mydir")
448+
assert result_space is not None
449+
assert result_equals is not None
450+
assert result_space["commands_dir"] == "./mydir"
451+
assert result_equals["commands_dir"] == "./mydir"
452+
453+
454+
class TestUninstallNoManifestClearsInitOptions:
455+
def test_init_options_cleared_on_no_manifest_uninstall(self, tmp_path):
456+
"""When no manifest exists, uninstall should still clear init-options.json."""
457+
project = tmp_path / "proj"
458+
project.mkdir()
459+
(project / ".specify").mkdir()
460+
461+
# Write integration.json and init-options.json without a manifest
462+
int_json = project / ".specify" / "integration.json"
463+
int_json.write_text(json.dumps({"integration": "claude"}), encoding="utf-8")
464+
465+
opts_json = project / ".specify" / "init-options.json"
466+
opts_json.write_text(json.dumps({
467+
"integration": "claude",
468+
"ai": "claude",
469+
"ai_skills": True,
470+
"script": "sh",
471+
}), encoding="utf-8")
472+
473+
old_cwd = os.getcwd()
474+
try:
475+
os.chdir(project)
476+
result = runner.invoke(app, ["integration", "uninstall", "claude"])
477+
finally:
478+
os.chdir(old_cwd)
479+
assert result.exit_code == 0
480+
481+
# init-options.json should have integration keys cleared
482+
opts = json.loads(opts_json.read_text(encoding="utf-8"))
483+
assert "integration" not in opts
484+
assert "ai" not in opts
485+
assert "ai_skills" not in opts
486+
# Non-integration keys preserved
487+
assert opts.get("script") == "sh"
488+
489+
490+
class TestSwitchClearsMetadataAfterTeardown:
491+
def test_metadata_cleared_between_phases(self, tmp_path):
492+
"""If install phase fails during switch, metadata should not reference the removed integration."""
493+
project = _init_project(tmp_path, "claude")
494+
495+
# Verify initial state
496+
int_json = project / ".specify" / "integration.json"
497+
assert json.loads(int_json.read_text(encoding="utf-8"))["integration"] == "claude"
498+
499+
old_cwd = os.getcwd()
500+
try:
501+
os.chdir(project)
502+
# Switch to copilot — should succeed and update metadata
503+
result = runner.invoke(app, [
504+
"integration", "switch", "copilot",
505+
"--script", "sh",
506+
], catch_exceptions=False)
507+
finally:
508+
os.chdir(old_cwd)
509+
assert result.exit_code == 0
510+
511+
# integration.json should reference copilot, not claude
512+
data = json.loads(int_json.read_text(encoding="utf-8"))
513+
assert data["integration"] == "copilot"
514+
515+
# init-options.json should reference copilot
516+
opts_json = project / ".specify" / "init-options.json"
517+
opts = json.loads(opts_json.read_text(encoding="utf-8"))
518+
assert opts.get("ai") == "copilot"

0 commit comments

Comments
 (0)