Modernize provider support

tests/test_providers.py

@@ -11,7 +11,7 @@ from cmdforge.providers import (
     Provider, ProviderResult,
     load_providers, save_providers, get_provider,
     add_provider, delete_provider,
-    call_provider, mock_provider,
+    call_provider, discover_installed_providers, mock_provider,
     DEFAULT_PROVIDERS, strip_ansi
 )
 
@@ -71,6 +71,29 @@ class TestProvider:
         assert restored.command == original.command
         assert restored.description == original.description
 
+    def test_legacy_positional_fallback_is_preserved(self):
+        provider = Provider("primary", "primary-cmd", "Primary", "backup")
+
+        assert provider.fallback == "backup"
+        assert provider.type == "subprocess"
+
+    def test_extended_fields_roundtrip(self):
+        original = Provider(
+            name="api-test",
+            command="https://example.test/v1",
+            description="Test API",
+            fallback="mock",
+            type="api",
+            model="test/model",
+            tags=["api", "test"],
+            install={"cost": "test"},
+            fallback_chain=["backup", "mock"],
+            api_key_env="TEST_API_KEY",
+            pty_config={"prompt_pattern": ">"},
+        )
+
+        assert Provider.from_dict(original.to_dict()) == original
+
 
 class TestProviderResult:
     """Tests for ProviderResult dataclass."""
@@ -144,6 +167,34 @@ class TestProviderPersistence:
         assert loaded[0].name == "test1"
         assert loaded[1].name == "test2"
 
+    def test_save_providers_uses_private_file_permissions(self, temp_providers_file):
+        save_providers([Provider("test", "cmd")])
+
+        assert oct(temp_providers_file.stat().st_mode & 0o777) == "0o600"
+
+    def test_legacy_config_gains_missing_defaults_without_losing_custom_provider(self, temp_providers_file):
+        temp_providers_file.parent.mkdir(parents=True)
+        temp_providers_file.write_text(yaml.safe_dump({
+            "providers": [
+                {
+                    "name": "custom",
+                    "command": "custom-command",
+                    "description": "Keep me",
+                    "fallback": "mock",
+                }
+            ]
+        }))
+
+        loaded = load_providers()
+
+        custom = next(provider for provider in loaded if provider.name == "custom")
+        assert custom.command == "custom-command"
+        assert custom.fallback == "mock"
+        assert any(provider.name == "opencode-free" for provider in loaded)
+        assert any(provider.name == "openrouter" for provider in loaded)
+        saved = yaml.safe_load(temp_providers_file.read_text())
+        assert saved["version"] == 2
+
     def test_get_provider_exists(self, temp_providers_file):
         providers = [
             Provider("target", "target-cmd", "Target provider")
@@ -246,6 +297,16 @@ class TestCallProvider:
         assert result.success is False
         assert "not found" in result.error.lower()
 
+    @patch('subprocess.run')
+    def test_call_provider_rejects_non_integer_max_tokens(self, mock_run, temp_providers_file):
+        save_providers([Provider("claude", "claude -p")])
+
+        result = call_provider("claude", "Test", max_tokens="1; touch /tmp/pwned")
+
+        assert result.success is False
+        assert "max_tokens" in result.error
+        mock_run.assert_not_called()
+
     @patch('subprocess.run')
     @patch('shutil.which')
     def test_call_real_provider_success(self, mock_which, mock_run, temp_providers_file):
@@ -456,6 +517,18 @@ class TestProviderFallback:
         assert result.success is True
         assert "[MOCK]" in result.text
 
+    def test_fallback_chain_continues_after_failed_candidate(self, temp_providers_file):
+        save_providers([
+            Provider("primary", "missing-primary", fallback_chain=["secondary", "mock"]),
+            Provider("secondary", "missing-secondary"),
+            Provider("mock", "mock"),
+        ])
+
+        result = call_provider("primary", "Test prompt")
+
+        assert result.success is True
+        assert "[MOCK]" in result.text
+
     def test_fallback_prevents_infinite_loop(self, temp_providers_file):
         """Circular fallback references should not cause infinite loop."""
         save_providers([
@@ -626,3 +699,88 @@ class TestMaxTokens:
         assert "--max-tokens 4096" in calls[0][0][0]
         assert "--max-tokens 4096" in calls[1][0][0]
 
+
+class TestApiProviders:
+    @pytest.fixture
+    def temp_providers_file(self, tmp_path):
+        providers_file = tmp_path / ".cmdforge" / "providers.yaml"
+        with patch('cmdforge.providers.PROVIDERS_FILE', providers_file):
+            yield providers_file
+
+    @patch('requests.post')
+    def test_openai_compatible_api_dispatch(self, mock_post, temp_providers_file):
+        response = MagicMock(status_code=200)
+        response.json.return_value = {
+            "choices": [{"message": {"content": "API response"}}]
+        }
+        mock_post.return_value = response
+        save_providers([
+            Provider(
+                "test-api",
+                "https://example.test/v1",
+                type="api",
+                model="example/model",
+                api_key_env="TEST_API_KEY",
+            )
+        ])
+
+        with patch.dict('os.environ', {"TEST_API_KEY": "secret"}):
+            result = call_provider("test-api", "Prompt", max_tokens=128)
+
+        assert result.success is True
+        assert result.text == "API response"
+        mock_post.assert_called_once_with(
+            "https://example.test/v1/chat/completions",
+            json={
+                "model": "example/model",
+                "messages": [{"role": "user", "content": "Prompt"}],
+                "max_tokens": 128,
+            },
+            headers={
+                "Authorization": "Bearer secret",
+                "Content-Type": "application/json",
+            },
+            timeout=300,
+        )
+
+    def test_api_provider_requires_configured_key(self, temp_providers_file):
+        save_providers([
+            Provider(
+                "test-api",
+                "https://example.test/v1",
+                type="api",
+                model="example/model",
+                api_key_env="MISSING_TEST_API_KEY",
+            )
+        ])
+
+        with patch.dict('os.environ', {"MISSING_TEST_API_KEY": ""}):
+            result = call_provider("test-api", "Prompt")
+
+        assert result.success is False
+        assert "MISSING_TEST_API_KEY" in result.error
+
+
+class TestProviderDiscovery:
+    def test_ollama_model_names_include_tags_and_are_unique(self):
+        ollama_list = MagicMock(
+            returncode=0,
+            stdout=(
+                "NAME ID SIZE MODIFIED\n"
+                "hermes4.3:latest abc 1 GB now\n"
+                "hermes4.3:q4_k_m def 1 GB now\n"
+            ),
+        )
+
+        def which(binary):
+            return "/usr/bin/ollama" if binary == "ollama" else None
+
+        with patch('cmdforge.providers.shutil.which', side_effect=which):
+            with patch('cmdforge.providers.subprocess.run', return_value=ollama_list):
+                found = discover_installed_providers()
+
+        models = [item for item in found if item["source"] == "ollama-model"]
+        assert len(models) == 2
+        assert len({item["name"] for item in models}) == 2
+        assert models[0]["name"] == "ollama-hermes4-3-latest"
+        assert models[1]["name"] == "ollama-hermes4-3-q4-k-m"