diff --git a/.env.quickstart.example b/.env.quickstart.example index ab79258..a092448 100644 --- a/.env.quickstart.example +++ b/.env.quickstart.example @@ -21,7 +21,9 @@ GITHUB_TOKEN= P10Y_API_KEY= # LLM provider — set ONE of the two keys below; leave the other blank. -# If both are set, OpenRouter is used by default. +# The provider is derived from whichever single key you set (Anthropic key +# only → Anthropic; OpenRouter key → OpenRouter). If both are set, OpenRouter is +# used by default. If neither is set, the backend refuses to start. # # OPENROUTER_API_KEY (default provider) # What: API key for OpenRouter, an LLM provider aggregator that routes diff --git a/QUICKSTART.md b/QUICKSTART.md index 519c71c..ce8d5d7 100644 --- a/QUICKSTART.md +++ b/QUICKSTART.md @@ -66,7 +66,8 @@ How to obtain P10Y_API_KEY: #### Prefer Anthropic? Set `ANTHROPIC_API_KEY` instead of `OPENROUTER_API_KEY`. -`specflow init` will set `DEFAULT_PROVIDER=anthropic` automatically. +The backend uses Anthropic automatically when it's the only provider key set +(if both are set, OpenRouter is used). Everything else in `.env.quickstart.example` is already set for local mode or is optional. diff --git a/backend/app/core/config.py b/backend/app/core/config.py index bf494ad..173cb40 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -1,10 +1,11 @@ import ast from dataclasses import dataclass from datetime import timedelta +from functools import cached_property import os from typing import Annotated, FrozenSet, Optional -from pydantic import AliasChoices, Field, field_validator, model_validator +from pydantic import AliasChoices, Field, computed_field, field_validator, model_validator from pydantic_settings import BaseSettings, NoDecode, SettingsConfigDict from app.core.enums import AuthMode, DatabaseType, LLMProvider @@ -51,7 +52,18 @@ @dataclass class EmailConfig: username: str - password: str + password: str + + +def is_key_valid(raw_key: Optional[str]) -> bool: + """True only when ``raw_key`` is a non-blank string. + + A whitespace-only value counts as unset, so callers can test key presence + without relying on bare truthiness (``if key`` == ``if bool(key)``, which + would treat ``" "`` as set). + """ + return bool(raw_key and str(raw_key).strip()) + class Settings(BaseSettings): PROJECT_NAME: str = "SpecFlow Backend" @@ -229,8 +241,7 @@ def _empty_str_to_none_int(cls, v: object) -> object: FIRESTORE_DATABASE_NAME: str = "default" # Firestore database name (default: "(default)") # LLM Provider Configuration - # Active LLM provider: "openrouter" (default) or "anthropic". - DEFAULT_PROVIDER: LLMProvider = LLMProvider.OPENROUTER + # DEFAULT_PROVIDER is derived from the API keys below — see the computed_field. # LLM Model Tier Configuration # Values follow OpenRouter naming convention: provider/model (e.g., anthropic/claude-opus-4.5) @@ -361,6 +372,22 @@ def _derive_claude_code_tmpdir(self) -> "Settings": ) return self + @computed_field + @cached_property + def DEFAULT_PROVIDER(self) -> LLMProvider: + # Derived solely from which key is present — there is no configurable + # knob (see .env.quickstart.example: "set ONE of the two keys"), so a + # provider/key mismatch is impossible by construction: OpenRouter when + # its key is set (also the documented default when both are), Anthropic + # when only that key is set. Neither set → OpenRouter, and startup + # validation then fails fast naming both keys. is_key_valid ignores + # blank/whitespace-only values. + if is_key_valid(self.OPENROUTER_API_KEY): + return LLMProvider.OPENROUTER + if is_key_valid(self.ANTHROPIC_API_KEY): + return LLMProvider.ANTHROPIC + return LLMProvider.OPENROUTER + @property def langfuse_enabled(self) -> bool: return bool( diff --git a/backend/app/services/startup_validation.py b/backend/app/services/startup_validation.py index 1779291..0459093 100644 --- a/backend/app/services/startup_validation.py +++ b/backend/app/services/startup_validation.py @@ -23,7 +23,7 @@ from pathlib import Path from typing import Dict, Any, Optional -from app.core.config import settings +from app.core.config import is_key_valid, settings from app.core.enums import DatabaseType, LLMProvider from app.database.interface import IDatabase from app.services.workspace_pool import WorkspacePoolService @@ -135,14 +135,20 @@ async def _check_environment(self) -> Dict[str, Any]: """ missing: list[str] = [] - # Check the key for whichever provider is active + # The provider is derived from whichever key is present (see + # Settings.DEFAULT_PROVIDER computed_field), so a provider/key mismatch is + # impossible: the anthropic branch always has its key. The only way the + # openrouter branch lacks a key is when NEITHER key was set — fail fast + # naming both so the user knows what to add. + # is_key_valid so a blank / whitespace-only env value counts as unset, + # matching how DEFAULT_PROVIDER derives the provider. if settings.DEFAULT_PROVIDER == LLMProvider.OPENROUTER: - if not os.environ.get("OPENROUTER_API_KEY"): + if not is_key_valid(os.environ.get("OPENROUTER_API_KEY")): missing.append( - "OPENROUTER_API_KEY (required for the active LLM provider: openrouter)" + "OPENROUTER_API_KEY or ANTHROPIC_API_KEY (set one LLM provider key)" ) elif settings.DEFAULT_PROVIDER == LLMProvider.ANTHROPIC: - if not os.environ.get("ANTHROPIC_API_KEY"): + if not is_key_valid(os.environ.get("ANTHROPIC_API_KEY")): missing.append( "ANTHROPIC_API_KEY (required for the active LLM provider: anthropic)" ) diff --git a/backend/test/core/test_enums.py b/backend/test/core/test_enums.py index 7c6d496..b8499e4 100644 --- a/backend/test/core/test_enums.py +++ b/backend/test/core/test_enums.py @@ -63,22 +63,28 @@ def test_invalid_raises(self): class TestSettingsDefaultProvider: - """Verify DEFAULT_PROVIDER is a Settings field and env-overridable.""" + """DEFAULT_PROVIDER is derived from the key present — not an env-settable knob.""" - def test_default_is_openrouter(self): + def test_no_keys_resolve_openrouter(self): from app.core.config import Settings - s = Settings() + with pytest.MonkeyPatch.context() as mp: + mp.delenv("ANTHROPIC_API_KEY", raising=False) + mp.delenv("OPENROUTER_API_KEY", raising=False) + s = Settings(_env_file=None) assert s.DEFAULT_PROVIDER == LLMProvider.OPENROUTER assert s.DEFAULT_PROVIDER == "openrouter" - def test_env_override_to_anthropic(self): + def test_env_default_provider_is_ignored(self): from app.core.config import Settings + # Setting DEFAULT_PROVIDER has no effect: the present key decides. with pytest.MonkeyPatch.context() as mp: + mp.delenv("ANTHROPIC_API_KEY", raising=False) + mp.setenv("OPENROUTER_API_KEY", "or-key") mp.setenv("DEFAULT_PROVIDER", "anthropic") - s = Settings() - assert s.DEFAULT_PROVIDER == LLMProvider.ANTHROPIC + s = Settings(_env_file=None) + assert s.DEFAULT_PROVIDER == LLMProvider.OPENROUTER def test_database_type_bogus_raises(self): from app.core.config import Settings diff --git a/backend/test/services/test_startup_validation.py b/backend/test/services/test_startup_validation.py index ee6a540..212afd9 100644 --- a/backend/test/services/test_startup_validation.py +++ b/backend/test/services/test_startup_validation.py @@ -96,24 +96,31 @@ class TestEnvironmentValidation: @pytest.mark.asyncio async def test_environment_check_passes(self, validator): - """Environment check passes with OPENROUTER_API_KEY set (default provider).""" + """Environment check passes with OPENROUTER_API_KEY set (provider resolves to openrouter).""" + from app.core.config import Settings + with patch.dict(os.environ, { "OPENROUTER_API_KEY": "test-key", "DATABASE_TYPE": "memory", - }): - result = await validator._check_environment() + }, clear=True): + with patch("app.services.startup_validation.settings", Settings(_env_file=None)): + result = await validator._check_environment() assert result["passed"] is True assert result["error"] is None @pytest.mark.asyncio async def test_environment_check_fails_missing_vars(self, validator): - """Environment check fails when the active provider key is missing.""" + """With no provider key set, the check fails fast naming both provider keys.""" + from app.core.config import Settings + with patch.dict(os.environ, {}, clear=True): - result = await validator._check_environment() + with patch("app.services.startup_validation.settings", Settings(_env_file=None)): + result = await validator._check_environment() assert result["passed"] is False assert "OPENROUTER_API_KEY" in result["error"] + assert "ANTHROPIC_API_KEY" in result["error"] @pytest.mark.asyncio async def test_environment_check_firestore_requires_git_secrets(self, validator): @@ -300,48 +307,55 @@ class TestProviderKeyValidation: @pytest.mark.asyncio async def test_openrouter_only_passes(self, validator): - """OpenRouter-only config passes (FR-1).""" + """OpenRouter key present → provider resolves to openrouter, check passes (FR-1).""" + from app.core.config import Settings + with patch.dict(os.environ, { "OPENROUTER_API_KEY": "or-key", "DATABASE_TYPE": "memory", }, clear=True): - result = await validator._check_environment() + with patch("app.services.startup_validation.settings", Settings(_env_file=None)): + result = await validator._check_environment() assert result["passed"] is True @pytest.mark.asyncio async def test_anthropic_only_passes(self, validator): - """Anthropic-only config passes when DEFAULT_PROVIDER=anthropic (FR-2).""" + """Anthropic key only → provider resolves to anthropic, check passes (FR-2).""" from app.core.config import Settings - from unittest.mock import patch as mpatch - - anthropic_settings = Settings(DEFAULT_PROVIDER="anthropic") - with mpatch("app.services.startup_validation.settings", anthropic_settings): - with patch.dict(os.environ, { - "ANTHROPIC_API_KEY": "sk-ant-key", - "DATABASE_TYPE": "memory", - }, clear=True): + + with patch.dict(os.environ, { + "ANTHROPIC_API_KEY": "sk-ant-key", + "DATABASE_TYPE": "memory", + }, clear=True): + with patch("app.services.startup_validation.settings", Settings(_env_file=None)): result = await validator._check_environment() assert result["passed"] is True @pytest.mark.asyncio - async def test_both_provider_keys_missing_fails_with_provider_name(self, validator): - """When no provider key set, message names both the variable and the active provider (FR-3).""" + async def test_no_provider_key_fails_naming_both(self, validator): + """When neither key is set, the message names both provider keys (FR-3, fail fast).""" + from app.core.config import Settings + with patch.dict(os.environ, { "DATABASE_TYPE": "memory", }, clear=True): - result = await validator._check_environment() + with patch("app.services.startup_validation.settings", Settings(_env_file=None)): + result = await validator._check_environment() assert result["passed"] is False assert "OPENROUTER_API_KEY" in result["error"] - assert "openrouter" in result["error"] + assert "ANTHROPIC_API_KEY" in result["error"] @pytest.mark.asyncio async def test_memory_skips_git_secrets(self, validator): """memory database mode does not require git/token secrets (FR-4).""" + from app.core.config import Settings + with patch.dict(os.environ, { "OPENROUTER_API_KEY": "or-key", "DATABASE_TYPE": "memory", }, clear=True): - result = await validator._check_environment() + with patch("app.services.startup_validation.settings", Settings(_env_file=None)): + result = await validator._check_environment() assert result["passed"] is True # Confirm no git-secret keys were required assert result["error"] is None @@ -441,3 +455,81 @@ def test_local_auth_rejected_with_firestore(self): }, clear=True): with pytest.raises(ValidationError, match="AUTH_MODE=local is not allowed"): Settings(_env_file=None) + + +class TestProviderResolution: + """DEFAULT_PROVIDER is derived solely from which key is present — not configurable. + + Mirrors the .env.quickstart.example "set ONE of the two keys" UX so an + Anthropic-only setup selects Anthropic instead of silently defaulting to + OpenRouter and failing the startup key check (backend stuck at 503). + """ + + def _settings(self, env: dict): + from app.core.config import Settings + + with patch.dict(os.environ, {"DATABASE_TYPE": "memory", **env}, clear=True): + return Settings(_env_file=None) + + def test_anthropic_only_resolves_anthropic(self): + from app.core.enums import LLMProvider + + settings = self._settings({"ANTHROPIC_API_KEY": "sk-ant-key"}) + assert settings.DEFAULT_PROVIDER == LLMProvider.ANTHROPIC + + def test_openrouter_only_resolves_openrouter(self): + from app.core.enums import LLMProvider + + settings = self._settings({"OPENROUTER_API_KEY": "or-key"}) + assert settings.DEFAULT_PROVIDER == LLMProvider.OPENROUTER + + def test_both_keys_resolve_openrouter(self): + from app.core.enums import LLMProvider + + settings = self._settings( + {"ANTHROPIC_API_KEY": "sk-ant-key", "OPENROUTER_API_KEY": "or-key"} + ) + assert settings.DEFAULT_PROVIDER == LLMProvider.OPENROUTER + + def test_no_keys_resolve_openrouter(self): + from app.core.enums import LLMProvider + + # Left at openrouter; startup validation then fails fast naming both keys. + settings = self._settings({}) + assert settings.DEFAULT_PROVIDER == LLMProvider.OPENROUTER + + def test_whitespace_key_is_treated_as_unset(self): + from app.core.enums import LLMProvider + + # A blank/whitespace-only key must not count as "set": OpenRouter is + # whitespace, only Anthropic is real → provider resolves to anthropic. + settings = self._settings( + {"OPENROUTER_API_KEY": " ", "ANTHROPIC_API_KEY": "sk-ant-key"} + ) + assert settings.DEFAULT_PROVIDER == LLMProvider.ANTHROPIC + + def test_explicit_default_provider_is_ignored(self): + from app.core.enums import LLMProvider + + # There is no knob: an attempt to set DEFAULT_PROVIDER is overridden by + # the key-derived value (anthropic key only → anthropic, despite the env). + settings = self._settings( + {"ANTHROPIC_API_KEY": "sk-ant-key", "DEFAULT_PROVIDER": "openrouter"} + ) + assert settings.DEFAULT_PROVIDER == LLMProvider.ANTHROPIC + + +class TestIsKeyValid: + """is_key_valid — the shared blank/whitespace-safe key presence check.""" + + def test_non_blank_is_valid(self): + from app.core.config import is_key_valid + + assert is_key_valid("sk-ant-key") is True + + def test_none_blank_and_whitespace_are_invalid(self): + from app.core.config import is_key_valid + + assert is_key_valid(None) is False + assert is_key_valid("") is False + assert is_key_valid(" ") is False diff --git a/docker-compose.yml b/docker-compose.yml index 4e917f1..8f348f5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -83,9 +83,9 @@ services: - "${SPECFLOW_BACKEND_PORT:-8000}:8000" environment: # Common environment variables + # The active LLM provider is derived from whichever key is set (both -> openrouter). - ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY} - OPENROUTER_API_KEY=${OPENROUTER_API_KEY} - - DEFAULT_PROVIDER=${DEFAULT_PROVIDER:-openrouter} - LOG_LEVEL=${LOG_LEVEL:-DEBUG} - WORKSPACE_BASE_PATH=/workspaces - NOTIFY_EMAIL_USERNAME=${NOTIFY_EMAIL_USERNAME} diff --git a/mcp_server/tests/test_tui_app.py b/mcp_server/tests/test_tui_app.py index 6939642..ec75058 100644 --- a/mcp_server/tests/test_tui_app.py +++ b/mcp_server/tests/test_tui_app.py @@ -337,6 +337,46 @@ async def test_start_yes_starts_then_proceeds(self): await pilot.pause() assert isinstance(app.screen, tui_app.SessionsScreen) + @pytest.mark.asyncio + async def test_containers_up_but_backend_not_ready_says_unhealthy_not_down(self): + # Containers running + backend not ready → the gate must not claim the + # containers aren't running; it shows the "backend isn't healthy" prompt. + with ( + patch("tui.app.local_env.is_setup_complete", return_value=True), + patch("tui.app.local_env.containers_running", return_value=True), + patch("tui.app.local_env.backend_ready", new=AsyncMock(return_value=False)), + ): + app = tui_app.SpecFlowTUI(root=Path("/tmp/x"), generation_id=None, poll_interval=999) + async with app.run_test() as pilot: + await pilot.pause() + assert isinstance(app.screen, tui_app.StartContainersScreen) + prompt = str(app.screen.query_one("#docker-prompt").render()) + assert "aren't running" not in prompt + assert "isn't healthy" in prompt + + @pytest.mark.asyncio + async def test_backend_not_ready_retry_skips_compose_up(self): + # In the up-but-not-ready path, retrying (y) re-polls readiness without + # re-running `docker compose up` (the containers are already up). + start = AsyncMock(return_value=0) + with ( + patch("tui.app.local_env.is_setup_complete", return_value=True), + patch("tui.app.local_env.containers_running", return_value=True), + patch("tui.app.local_env.backend_ready", new=AsyncMock(return_value=False)), + patch("tui.app.local_env.start_containers", new=start), + patch("tui.app.local_env.wait_backend_ready", new=AsyncMock(return_value=True)), + patch("tui.app.fetch_sessions", new=AsyncMock(return_value=[])), + patch.object(tui_app.ClientSetupScreen, "_probe_verifiable", new=AsyncMock()), + patch("tui.app.mcp_clients.is_any_client_connected", return_value=True), + ): + app = tui_app.SpecFlowTUI(root=Path("/tmp/x"), generation_id=None, poll_interval=999) + async with app.run_test() as pilot: + await pilot.pause() + await pilot.press("y") + await pilot.pause() + assert isinstance(app.screen, tui_app.SessionsScreen) + start.assert_not_awaited() + class TestOnboarding: """Drive the step-by-step onboarding wizard via the pilot. diff --git a/mcp_server/tui/app.py b/mcp_server/tui/app.py index 64244ef..0eb5ee5 100644 --- a/mcp_server/tui/app.py +++ b/mcp_server/tui/app.py @@ -1383,11 +1383,18 @@ async def _run_init(self) -> None: class StartContainersScreen(_SpecFlowScreen): - """Docker gate: offer to start the SpecFlow stack, or quit. - - ``y`` runs ``docker compose up -d`` and waits for the backend to report - ready (streamed into a log pane), then dismisses ``True``; ``n`` dismisses - ``False`` so the gate exits the app. + """Docker gate: start the stack (or wait out an unhealthy backend), or quit. + + Two modes, selected by ``containers_up``: + * ``False`` (default) — the containers aren't running. ``y`` runs + ``docker compose up -d`` then waits for the backend to report ready. + * ``True`` — the containers ARE running but ``/health/ready`` isn't 200 + yet. ``y`` only re-polls readiness (no redundant ``compose up``); this is + typically a config problem (e.g. an LLM provider/key mismatch), not a + stopped container, so we must not claim "containers aren't running". + + ``y`` dismisses ``True`` once the backend is ready; ``n`` dismisses ``False`` + so the gate exits the app. """ BINDINGS = [ @@ -1395,12 +1402,20 @@ class StartContainersScreen(_SpecFlowScreen): Binding("n", "no", "quit"), ] + def __init__(self, *, containers_up: bool = False) -> None: + super().__init__() + self._containers_up = containers_up + def compose(self) -> ComposeResult: yield Header() - yield Static( - "The SpecFlow containers aren't running.\n\n" "Start them now? [y] start [n] quit", - id="docker-prompt", - ) + if self._containers_up: + prompt = ( + "Containers are running but the backend isn't healthy yet.\n\n" + "Retry the health check? [y] retry [n] quit" + ) + else: + prompt = "The SpecFlow containers aren't running.\n\n" "Start them now? [y] start [n] quit" + yield Static(prompt, id="docker-prompt") log = RichLog(id="docker-log", highlight=False, markup=False, wrap=True) log.display = False yield log @@ -1415,7 +1430,9 @@ def action_yes(self) -> None: async def _start(self) -> None: log = self.query_one("#docker-log", RichLog) log.display = True - await local_env.start_containers(self.app.root, on_line=log.write) + # Containers already up → skip the redundant `compose up` and just re-poll. + if not self._containers_up: + await local_env.start_containers(self.app.root, on_line=log.write) backend_url = self.app.backend_url ok = await local_env.wait_backend_ready( backend_url, @@ -1425,7 +1442,8 @@ async def _start(self) -> None: self.dismiss(True) else: self.notify( - "Backend didn't become healthy — check `docker compose logs`.", + "Backend didn't become healthy — check `docker compose logs` " + "(e.g. an LLM provider/key mismatch).", severity="error", ) @@ -1611,8 +1629,9 @@ async def _startup_gate(self) -> None: self.exit() return elif not await local_env.backend_ready(self.backend_url): - # Containers up but not ready yet — reuse the gate to wait it out. - if not await self.push_screen_wait(StartContainersScreen()): + # Containers up but not ready yet — wait it out with an accurate prompt + # (do not claim the containers aren't running; they are). + if not await self.push_screen_wait(StartContainersScreen(containers_up=True)): self.exit() return diff --git a/specflow-init.sh b/specflow-init.sh index b4fb681..393dbc9 100755 --- a/specflow-init.sh +++ b/specflow-init.sh @@ -233,20 +233,6 @@ export FIRESTORE_DATABASE_NAME="${FIRESTORE_DATABASE_NAME:-specflow}" log "INFO: Firestore emulator data dir: ${FIRESTORE_EMULATOR_DATA_DIR}" log "INFO: Firestore database name: ${FIRESTORE_DATABASE_NAME}" -# --------------------------------------------------------------------------- -# Step 1b: Derive local LLM provider -# --------------------------------------------------------------------------- -if [[ -z "${DEFAULT_PROVIDER:-}" && -z "${OPENROUTER_API_KEY:-}" && -n "${ANTHROPIC_API_KEY:-}" ]]; then - if [[ "${DRY_RUN}" == "true" ]]; then - export DEFAULT_PROVIDER="anthropic" - info "[DRY RUN] DEFAULT_PROVIDER is blank and only ANTHROPIC_API_KEY is set - would persist DEFAULT_PROVIDER=anthropic to .env" - else - set_env_value "DEFAULT_PROVIDER" "anthropic" - info "DEFAULT_PROVIDER was blank and OPENROUTER_API_KEY was not set, so DEFAULT_PROVIDER=anthropic was written to .env." - log "INFO: DEFAULT_PROVIDER resolved to anthropic from available LLM key" - fi -fi - # --------------------------------------------------------------------------- # Step 1c: Derive local git identity # ---------------------------------------------------------------------------