From 4089256245d86e5950eb3f42ea25284c5f9f6bb1 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Sat, 16 May 2026 22:13:43 +0800 Subject: [PATCH] fix(cli): clarify exception diagnostics Consolidate the CLI diagnostic plan, implementation, and test hardening into one reviewable change. The CLI now reports phase and target context for broad failure paths while preserving existing fail-fast behavior for real setup failures and warning-only behavior for optional best-effort work. The workflow unit tests also avoid discovering real local agent CLIs, so developer machines with tools such as gemini installed do not hang pytest during metadata-only assertions. Constraint: CLI setup failures must remain fail-fast, while optional preset and cleanup paths should continue with clear warnings. Rejected: Replace broad handlers across the whole codebase in one pass | too broad for a targeted CLI diagnostic fix Rejected: Add runtime timeouts to workflow agent dispatch | dispatch may legitimately be long-running and the observed hang was test isolation Confidence: high Scope-risk: moderate Directive: Keep future best-effort CLI warnings tied to the failed phase and target so users can diagnose setup state. Tested: uvx ruff check src/; uv run pytest tests/integrations/test_cli.py -v; uv run pytest tests/test_workflows.py::TestCommandStep::test_step_override_integration tests/test_workflows.py::TestPromptStep::test_execute_with_step_integration tests/test_workflows.py::TestPromptStep::test_execute_with_model -vv; uv run pytest Not-tested: Real Nacos/PG/Redis-style external service failure injection; real interactive workflow dispatch against installed gemini CLI --- src/specify_cli/__init__.py | 106 ++++++++++++++---- tests/integrations/test_cli.py | 193 +++++++++++++++++++++++++++++++++ tests/test_workflows.py | 12 +- 3 files changed, 289 insertions(+), 22 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index d4e8632215..f410639af3 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -425,6 +425,35 @@ def _get_skills_dir(project_path: Path, selected_ai: str) -> Path: return project_path / ".agents" / "skills" +def _cli_error_detail(exc: BaseException) -> str: + """Return a compact one-line exception detail for CLI output.""" + detail = str(exc).replace("\n", " ").strip() + return detail or exc.__class__.__name__ + + +def _cli_phase_label(phase: str, target_kind: str, target: str | None = None) -> str: + """Format a stable operation label for user-visible diagnostics.""" + label = f"{phase} {target_kind}".strip() + if target: + label = f"{label} '{target}'" + return label + + +def _print_cli_warning( + phase: str, + target_kind: str, + target: str | None, + exc: BaseException, + *, + continuing: str | None = None, +) -> None: + """Print a warning that names the failed CLI phase and target.""" + label = _cli_phase_label(phase, target_kind, target) + console.print(f"[yellow]Warning:[/yellow] Failed to {label}: {_cli_error_detail(exc)}") + if continuing: + console.print(f"[dim]{continuing}[/dim]") + + # Constants kept for backward compatibility with presets and extensions. DEFAULT_SKILLS_DIR = ".agents/skills" SKILL_DESCRIPTIONS = { @@ -857,9 +886,8 @@ def init( git_messages.append("bundled extension not found") except Exception as ext_err: git_has_error = True - sanitized_ext = str(ext_err).replace('\n', ' ').strip() git_messages.append( - f"extension install failed: {sanitized_ext[:120]}" + f"extension install failed during optional git setup: {_cli_error_detail(ext_err)[:120]}" ) summary = "; ".join(git_messages) if git_has_error: @@ -897,8 +925,10 @@ def init( else: tracker.skip("workflow", "bundled workflow not found") except Exception as wf_err: - sanitized_wf = str(wf_err).replace('\n', ' ').strip() - tracker.error("workflow", f"install failed: {sanitized_wf[:120]}") + tracker.error( + "workflow", + f"install bundled workflow 'speckit' failed: {_cli_error_detail(wf_err)[:120]}", + ) # Fix permissions after all installs (scripts + extensions) ensure_executable_scripts(project_path, tracker=tracker) @@ -960,7 +990,13 @@ def init( zip_path = preset_catalog.download_pack(preset) preset_manager.install_from_zip(zip_path, speckit_ver) except PresetError as preset_err: - console.print(f"[yellow]Warning:[/yellow] Failed to install preset '{preset}': {preset_err}") + _print_cli_warning( + "install", + "preset", + preset, + preset_err, + continuing="Continuing without the optional preset.", + ) finally: if zip_path is not None: # Clean up downloaded ZIP to avoid cache accumulation @@ -970,7 +1006,14 @@ def init( # Best-effort cleanup; failure to delete is non-fatal pass except Exception as preset_err: - console.print(f"[yellow]Warning:[/yellow] Failed to install preset: {preset_err}") + # Optional preset install must not abort project initialization. + _print_cli_warning( + "install", + "preset", + preset, + preset_err, + continuing="Continuing without the optional preset.", + ) tracker.complete("final", "project ready") except (typer.Exit, SystemExit): @@ -1782,20 +1825,29 @@ def integration_install( if new_default == integration.key: _update_init_options_for_integration(project_root, integration, script_type=selected_script) - except Exception as e: + except Exception as exc: # Attempt rollback of any files written by setup try: integration.teardown(project_root, manifest, force=True) except Exception as rollback_err: # Suppress so the original setup error remains the primary failure - console.print(f"[yellow]Warning:[/yellow] Failed to roll back integration changes: {rollback_err}") + _print_cli_warning( + "rollback", + "integration", + key, + rollback_err, + continuing="The original install failure is still the primary error.", + ) if installed_keys: _write_integration_json( project_root, default_key, installed_keys, _integration_settings(current) ) else: _remove_integration_json(project_root) - console.print(f"[red]Error:[/red] Failed to install integration: {e}") + console.print( + f"[red]Error:[/red] Failed to {_cli_phase_label('install', 'integration', key)}: " + f"{_cli_error_detail(exc)}" + ) raise typer.Exit(1) name = (integration.config or {}).get("name", key) @@ -2159,9 +2211,12 @@ def integration_switch( ext_mgr = ExtensionManager(project_root) ext_mgr.unregister_agent_artifacts(installed_key) except Exception as ext_err: - console.print( - f"[yellow]Warning:[/yellow] Could not clean up extension artifacts " - f"(commands, skills, registry entries) for '{installed_key}': {ext_err}" + _print_cli_warning( + "clean up extension artifacts for", + "integration", + installed_key, + ext_err, + continuing="Continuing with integration switch; old extension artifacts may need manual cleanup.", ) # Clear metadata so a failed Phase 2 doesn't leave stale references @@ -2256,18 +2311,27 @@ def integration_switch( ext_mgr = ExtensionManager(project_root) ext_mgr.register_enabled_extensions_for_agent(target) except Exception as ext_err: - console.print( - f"[yellow]Warning:[/yellow] Could not register extension commands, skills, " - f"or related artifacts for '{target}': {ext_err}" + _print_cli_warning( + "register extension artifacts for", + "integration", + target, + ext_err, + continuing="The integration switch succeeded, but installed extensions may need re-registration.", ) - except Exception as e: + except Exception as exc: # Attempt rollback of any files written by setup try: target_integration.teardown(project_root, manifest, force=True) except Exception as rollback_err: # Suppress so the original setup error remains the primary failure - console.print(f"[yellow]Warning:[/yellow] Failed to roll back integration '{target}': {rollback_err}") + _print_cli_warning( + "rollback", + "integration", + target, + rollback_err, + continuing="The original switch failure is still the primary error.", + ) if installed_keys: fallback_key = installed_keys[0] fallback_integration = get_integration(fallback_key) @@ -2296,7 +2360,10 @@ def integration_switch( ) else: _remove_integration_json(project_root) - console.print(f"[red]Error:[/red] Failed to install integration '{target}': {e}") + console.print( + f"[red]Error:[/red] Failed to {_cli_phase_label('install', 'integration', target)} " + f"during switch: {_cli_error_detail(exc)}" + ) raise typer.Exit(1) name = (target_integration.config or {}).get("name", target) @@ -2431,7 +2498,8 @@ def integration_upgrade( except Exception as exc: # Don't teardown — setup overwrites in-place, so teardown would # delete files that were working before the upgrade. Just report. - console.print(f"[red]Error:[/red] Failed to upgrade integration: {exc}") + console.print(f"[red]Error:[/red] Failed to {_cli_phase_label('upgrade', 'integration', key)}.") + console.print(f"[dim]Details:[/dim] {_cli_error_detail(exc)}") console.print("[yellow]The previous integration files may still be in place.[/yellow]") raise typer.Exit(1) diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index de09205310..6b171c4164 100644 --- a/tests/integrations/test_cli.py +++ b/tests/integrations/test_cli.py @@ -22,6 +22,26 @@ def _normalize_cli_output(output: str) -> str: return output.strip() +class TestCliDiagnosticFormatting: + def test_cli_error_detail_flattens_newlines(self): + import specify_cli + + assert specify_cli._cli_error_detail(RuntimeError("line one\nline two")) == "line one line two" + + def test_cli_error_detail_handles_empty_message(self): + import specify_cli + + assert specify_cli._cli_error_detail(RuntimeError()) == "RuntimeError" + + def test_cli_phase_label_includes_target(self): + import specify_cli + + assert ( + specify_cli._cli_phase_label("rollback", "integration", "codex") + == "rollback integration 'codex'" + ) + + class TestInitIntegrationFlag: def test_integration_and_ai_mutually_exclusive(self, tmp_path): from typer.testing import CliRunner @@ -174,6 +194,42 @@ def test_ai_generic_warning_suggests_integration_options_equivalent(self, tmp_pa assert normalized_output.index("Deprecation Warning") < normalized_output.index("Next Steps") assert (project / ".myagent" / "commands" / "speckit.plan.md").exists() + def test_init_optional_preset_failure_reports_target_and_continues( + self, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.presets import PresetManager + + def fail_install(self, path, version): + raise OSError("preset install exploded\nwith context") + + monkeypatch.setattr(PresetManager, "install_from_directory", fail_install) + + project = tmp_path / "init-preset-warning" + result = CliRunner().invoke( + app, + [ + "init", + str(project), + "--integration", + "copilot", + "--script", + "sh", + "--no-git", + "--preset", + "lean", + ], + catch_exceptions=False, + ) + normalized = _normalize_cli_output(result.output) + + assert result.exit_code == 0, result.output + assert "Failed to install preset 'lean'" in normalized + assert "preset install exploded with context" in normalized + assert "Continuing without the optional preset" in normalized + assert "Project ready" in normalized + def test_ai_claude_here_preserves_preexisting_commands(self, tmp_path): from typer.testing import CliRunner from specify_cli import app @@ -1055,6 +1111,143 @@ def _invoke(self, argv, cwd): finally: os.chdir(old) + def test_integration_install_failure_reports_phase_target_and_rollback( + self, tmp_path, monkeypatch + ): + from specify_cli.integrations import INTEGRATION_REGISTRY + from specify_cli.integrations.base import IntegrationBase + + class BrokenIntegration(IntegrationBase): + key = "broken-test" + config = { + "name": "Broken Test", + "folder": ".broken/", + "commands_subdir": "commands", + "install_url": None, + "requires_cli": False, + } + registrar_config = { + "dir": ".broken/commands", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": ".md", + } + context_file = "BROKEN.md" + + def setup(self, project_root, manifest, **kwargs): + raise OSError("setup exploded\nwith context") + + def teardown(self, project_root, manifest, force=False): + raise OSError("rollback exploded") + + project = self._make_project(tmp_path) + monkeypatch.setitem(INTEGRATION_REGISTRY, "broken-test", BrokenIntegration()) + + result = self._invoke(["integration", "install", "broken-test"], project) + normalized = _normalize_cli_output(result.output) + + assert result.exit_code == 1, result.output + assert "Failed to rollback integration 'broken-test'" in normalized + assert "rollback exploded" in normalized + assert "Failed to install integration 'broken-test'" in normalized + assert "setup exploded with context" in normalized + + def test_integration_upgrade_failure_reports_phase_and_target( + self, tmp_path, monkeypatch + ): + from specify_cli.integrations import INTEGRATION_REGISTRY + from specify_cli.integrations.copilot import CopilotIntegration + + class UpgradeBrokenIntegration(CopilotIntegration): + key = "upgrade-broken" + config = dict(CopilotIntegration.config) + config["name"] = "Upgrade Broken" + + def setup(self, project_root, manifest, **kwargs): + raise OSError("upgrade exploded\nwith context") + + project = self._make_project(tmp_path) + monkeypatch.setitem( + INTEGRATION_REGISTRY, "upgrade-broken", UpgradeBrokenIntegration() + ) + + (project / ".specify" / "integrations").mkdir(parents=True, exist_ok=True) + (project / ".specify" / "integration.json").write_text( + json.dumps( + { + "version": 1, + "integration": "upgrade-broken", + "integrations": ["upgrade-broken"], + "integration_settings": {"upgrade-broken": {"script": "sh"}}, + } + ), + encoding="utf-8", + ) + ( + project / ".specify" / "integrations" / "upgrade-broken.manifest.json" + ).write_text( + json.dumps( + { + "integration": "upgrade-broken", + "version": "0.0.0", + "installed_at": "2026-05-16T00:00:00+00:00", + "files": {}, + } + ), + encoding="utf-8", + ) + + result = self._invoke(["integration", "upgrade", "upgrade-broken"], project) + normalized = _normalize_cli_output(result.output) + + assert result.exit_code == 1, result.output + assert "Failed to upgrade integration 'upgrade-broken'" in normalized + assert "upgrade exploded with context" in normalized + assert "previous integration files may still be in place" in normalized + + def test_integration_switch_cleanup_warning_reports_phase_and_targets( + self, tmp_path, monkeypatch + ): + from specify_cli.extensions import ExtensionManager + + project = self._make_project(tmp_path) + (project / ".specify" / "integrations").mkdir(parents=True, exist_ok=True) + (project / ".specify" / "integration.json").write_text( + json.dumps( + { + "version": 1, + "integration": "copilot", + "integrations": ["copilot"], + "integration_settings": {"copilot": {"script": "sh"}}, + } + ), + encoding="utf-8", + ) + (project / ".specify" / "integrations" / "copilot.manifest.json").write_text( + json.dumps( + { + "integration": "copilot", + "version": "0.0.0", + "installed_at": "2026-05-16T00:00:00+00:00", + "files": {}, + } + ), + encoding="utf-8", + ) + + def fail_cleanup(self, integration_key): + raise OSError("cleanup exploded") + + monkeypatch.setattr(ExtensionManager, "unregister_agent_artifacts", fail_cleanup) + + result = self._invoke(["integration", "switch", "claude"], project) + normalized = _normalize_cli_output(result.output) + + assert result.exit_code == 0, result.output + assert "Failed to clean up extension artifacts for integration 'copilot'" in normalized + assert "cleanup exploded" in normalized + assert "Switched to integration" in normalized + # -- Project guard ----------------------------------------------------- def test_search_requires_specify_project(self, tmp_path): diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 4c042fc7d5..39d43ea759 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -463,6 +463,7 @@ def test_validate_missing_command(self): assert any("missing 'command'" in e for e in errors) def test_step_override_integration(self): + from unittest.mock import patch from specify_cli.workflows.steps.command import CommandStep from specify_cli.workflows.base import StepContext @@ -474,7 +475,8 @@ def test_step_override_integration(self): "integration": "gemini", "input": {}, } - result = step.execute(config, ctx) + with patch("specify_cli.workflows.steps.command.shutil.which", return_value=None): + result = step.execute(config, ctx) assert result.output["integration"] == "gemini" def test_step_override_model(self): @@ -626,6 +628,7 @@ def test_execute_basic(self): assert result.output["dispatched"] is False def test_execute_with_step_integration(self): + from unittest.mock import patch from specify_cli.workflows.steps.prompt import PromptStep from specify_cli.workflows.base import StepContext @@ -637,10 +640,12 @@ def test_execute_with_step_integration(self): "prompt": "Summarize the codebase", "integration": "gemini", } - result = step.execute(config, ctx) + with patch("specify_cli.workflows.steps.prompt.shutil.which", return_value=None): + result = step.execute(config, ctx) assert result.output["integration"] == "gemini" def test_execute_with_model(self): + from unittest.mock import patch from specify_cli.workflows.steps.prompt import PromptStep from specify_cli.workflows.base import StepContext @@ -652,7 +657,8 @@ def test_execute_with_model(self): "prompt": "hello", "model": "opus-4", } - result = step.execute(config, ctx) + with patch("specify_cli.workflows.steps.prompt.shutil.which", return_value=None): + result = step.execute(config, ctx) assert result.output["model"] == "opus-4" def test_dispatch_with_mock_cli(self, tmp_path):