fix: address all code review issues

- Remove default sampling parameters (temperature, max_tokens) - Add input validation with max_input_chars limit - Improve error messages and remove emojis - Fix arrow/backspace input with get_multiline_input() - Enhance credential validation to test actual API access - Update configuration defaults and validation
2026-01-27 11:23:03 +01:00
parent 0b37f329c7
commit 1807c7eee1
3 changed files with 250 additions and 76 deletions
--- a/taletorrent/ai_client.py
+++ b/taletorrent/ai_client.py
@@ -7,7 +7,7 @@ from typing import Any
 from openai.types.chat import ChatCompletion

 from .api_key import get_openai_client
-from .errors import AIError, wrap_error
+from .errors import AIError, ValidationError, wrap_error


 logger = logging.getLogger(__name__)
@@ -28,22 +28,29 @@ class AIClient:
            # GameConfig instance
            self.max_retries = config.get("api.max_retries", 3)
            self.timeout = config.get("api.timeout", 30)
-            self.temperature = config.get("api.temperature", 0.7)
-            self.max_tokens = config.get("api.max_tokens", 1000)
+            self.max_input_chars = config.get("api.max_input_chars", 4000)
            self.creative_model = config.get("models.creative")
            self.pedantic_model = config.get("models.pedantic")
+
+            # Optional parameters - only use if explicitly configured
+            self.temperature = config.get("api.temperature")
+            self.max_tokens = config.get("api.max_tokens")
        else:
            # Dictionary
            self.max_retries = config.get("api", {}).get("max_retries", 3)
            self.timeout = config.get("api", {}).get("timeout", 30)
-            self.temperature = config.get("api", {}).get("temperature", 0.7)
-            self.max_tokens = config.get("api", {}).get("max_tokens", 1000)
+            self.max_input_chars = config.get("api", {}).get("max_input_chars", 4000)
            self.creative_model = config.get("models", {}).get("creative")
            self.pedantic_model = config.get("models", {}).get("pedantic")

+            # Optional parameters
+            self.temperature = config.get("api", {}).get("temperature")
+            self.max_tokens = config.get("api", {}).get("max_tokens")
+
        logger.debug(
            f"AIClient initialized with models: "
-            f"creative={self.creative_model}, pedantic={self.pedantic_model}"
+            f"creative={self.creative_model}, pedantic={self.pedantic_model}, "
+            f"max_input_chars={self.max_input_chars}"
        )

    def creative_completion(self, messages: list[dict[str, str]]) -> str:
@@ -57,12 +64,20 @@ class AIClient:

        Raises:
            AIError: If all retries fail or response is invalid
+            ValidationError: If input exceeds character limits
        """
+        self._validate_input_length(messages)
+
+        kwargs = {}
+        if self.temperature is not None:
+            kwargs["temperature"] = self.temperature
+        if self.max_tokens is not None:
+            kwargs["max_tokens"] = self.max_tokens
+
        return self._completion_with_retry(
            messages=messages,
            model=self.creative_model,
-            temperature=self.temperature,
-            max_tokens=self.max_tokens,
+            **kwargs,
        )

    def pedantic_completion(self, messages: list[dict[str, str]]) -> dict[str, Any]:
@@ -76,7 +91,10 @@ class AIClient:

        Raises:
            AIError: If all retries fail or response is invalid
+            ValidationError: If input exceeds character limits
        """
+        self._validate_input_length(messages)
+
        response = self._completion_with_retry(
            messages=messages,
            model=self.pedantic_model,
@@ -92,12 +110,35 @@ class AIClient:
        except Exception as e:
            raise wrap_error("Failed to parse JSON response", e)

+    def _validate_input_length(self, messages: list[dict[str, str]]) -> None:
+        """Validate that input messages don't exceed character limits.
+
+        Args:
+            messages: List of message dictionaries
+
+        Raises:
+            ValidationError: If total character count exceeds limit
+        """
+        total_chars = 0
+        for msg in messages:
+            content = msg.get("content", "")
+            if content:
+                total_chars += len(content)
+
+        if total_chars > self.max_input_chars:
+            raise ValidationError(
+                f"Input exceeds character limit: {total_chars} characters > "
+                f"{self.max_input_chars} maximum"
+            )
+
+        logger.debug(f"Input validation passed: {total_chars} characters")
+
    def _completion_with_retry(
        self,
        messages: list[dict[str, str]],
        model: str,
-        temperature: float,
-        max_tokens: int,
+        temperature: float | None = None,
+        max_tokens: int | None = None,
        response_format: dict[str, str] | None = None,
    ) -> str:
        """Execute completion with exponential backoff retry logic."""
@@ -147,25 +188,27 @@ class AIClient:
        self,
        messages: list[dict[str, str]],
        model: str,
-        temperature: float,
-        max_tokens: int,
+        temperature: float | None = None,
+        max_tokens: int | None = None,
        response_format: dict[str, str] | None = None,
    ) -> str:
        """Execute single completion request."""
        try:
            logger.debug(
-                f"Executing completion with model={model}, "
-                f"temperature={temperature}, messages={len(messages)}"
+                f"Executing completion with model={model}, messages={len(messages)}"
            )

            kwargs = {
                "model": model,
                "messages": messages,
-                "temperature": temperature,
-                "max_tokens": max_tokens,
                "timeout": self.timeout,
            }

+            # Only add optional parameters if provided
+            if temperature is not None:
+                kwargs["temperature"] = temperature
+            if max_tokens is not None:
+                kwargs["max_tokens"] = max_tokens
            if response_format:
                kwargs["response_format"] = response_format

@@ -182,7 +225,26 @@ class AIClient:
            return message.content.strip()

        except Exception as e:
-            raise wrap_error("Failed to execute completion", e)
+            raise wrap_error("API call failed", e)
+
+    def validate_api_access(self) -> bool:
+        """Validate API access with actual credentials.
+
+        Returns:
+            True if API access is valid, False otherwise
+        """
+        try:
+            # Test with a minimal request that requires valid credentials
+            test_messages = [{"role": "user", "content": "test"}]
+            self._execute_completion(
+                messages=test_messages,
+                model=self.creative_model,
+                max_tokens=5,  # Minimal tokens for test
+            )
+            return True
+        except Exception as e:
+            logger.error(f"API access validation failed: {e}")
+            return False

    def validate_model_availability(self) -> bool:
        """Check if configured models are available.
@@ -223,6 +285,7 @@ class AIClient:
            "settings": {
                "max_retries": self.max_retries,
                "timeout": self.timeout,
+                "max_input_chars": self.max_input_chars,
                "temperature": self.temperature,
                "max_tokens": self.max_tokens,
            },
--- a/taletorrent/cli.py
+++ b/taletorrent/cli.py
@@ -27,9 +27,56 @@ app = typer.Typer(help="Interactive role-playing game engine using AI")
 console = Console()


+def get_multiline_input(prompt: str, default: str = "") -> str:
+    """Get multi-line input from user with proper line editing support.
+
+    Args:
+        prompt: The prompt to display
+        default: Default value if user enters empty string
+
+    Returns:
+        User input as string
+    """
+    console.print(f"{prompt} (press Enter twice to finish):")
+
+    lines = []
+    while True:
+        try:
+            # Use typer.prompt for better line editing support
+            line = typer.prompt("", default="", show_default=False)
+            if not line.strip():  # Empty line
+                if lines:  # We have content, empty line means finish
+                    break
+                # No content yet, check if we should use default
+                if default and not lines:
+                    return default
+                # No content and no default, continue waiting for input
+            else:
+                lines.append(line)
+        except (EOFError, KeyboardInterrupt):
+            break
+
+    return "\n".join(lines) if lines else default
+
+
 def validate_api_config(api_key: str, base_url: str) -> bool:
-    """Validate API configuration by testing connection."""
+    """Validate API configuration by testing credentials."""
+    # Store credentials temporarily for testing
+    from .api_key import store_credentials, delete_credentials
+
+    # Backup existing credentials if any
+    from .api_key import has_credentials, get_credentials
+
+    had_existing = has_credentials()
+    existing_api_key = None
+    existing_base_url = None
+    if had_existing:
+        existing_api_key, existing_base_url = get_credentials()
+
    try:
+        # Store the new credentials temporarily
+        store_credentials(api_key, base_url)
+
        # Create a minimal config for testing
        test_config = {
            "models": {
@@ -39,16 +86,31 @@ def validate_api_config(api_key: str, base_url: str) -> bool:
            "api": {
                "max_retries": 1,
                "timeout": 10,
-                "temperature": 0.7,
-                "max_tokens": 100,
+                "max_input_chars": 1000,
            },
        }

        client = AIClient(test_config)
-        return client.validate_model_availability()
+        # First check model availability
+        if not client.validate_model_availability():
+            console.print("[red]API validation failed: Models not available[/red]")
+            return False
+
+        # Then test actual API access with credentials
+        if not client.validate_api_access():
+            console.print("[red]API validation failed: Invalid credentials[/red]")
+            return False
+
+        return True
    except Exception as e:
-        console.print(f"❌ Connection failed: {str(e)[:100]}...")
+        console.print(f"[red]API validation failed: {str(e)[:100]}...[/red]")
        return False
+    finally:
+        # Clean up: remove temporary credentials
+        delete_credentials()
+        # Restore existing credentials if there were any
+        if had_existing and existing_api_key and existing_base_url:
+            store_credentials(existing_api_key, existing_base_url)


@app.command()
@@ -74,7 +136,9 @@ def config(

    if api_key is None:
        if has_credentials():
-            console.print("ℹ️ [yellow]API credentials already configured[/yellow]")
+            console.print(
+                "[yellow]Info:[/yellow] [yellow]API credentials already configured[/yellow]"
+            )
            console.print(
                "Use [cyan]taletorrent info[/cyan] to view current configuration"
            )
@@ -84,29 +148,33 @@ def config(
            raise typer.Exit(0)
        else:
            console.print(
-                "❌ [red]No API key provided and no existing credentials found[/red]"
+                "[red]Error:[/red] [red]No API key provided and no existing credentials found[/red]"
            )
            console.print(
                "Use [cyan]--api-key[/cyan] option or [cyan]--interactive[/cyan] flag"
            )
            raise typer.Exit(1)

-    console.print("🔄 [blue]Validating API configuration...[/blue]")
+    console.print(
+        "[blue]Processing...[/blue] [blue]Validating API configuration...[/blue]"
+    )

    if validate_api_config(api_key, base_url):
        store_credentials(api_key, base_url)
-        console.print("✅ [green]API configuration stored successfully[/green]")
+        console.print(
+            "[green]Success:[/red] [green]API configuration stored successfully[/green]"
+        )

        # Create default config if it doesn't exist
        config_path = get_default_config_path()
        if not config_path.exists():
            create_default_config(config_path)
            console.print(
-                f"✅ [green]Default configuration created at {config_path}[/green]"
+                f"[green]Success:[/red] [green]Default configuration created at {config_path}[/green]"
            )
    else:
        console.print(
-            "❌ [red]API validation failed. Check your credentials and URL.[/red]"
+            "[red]Error:[/red] [red]API validation failed. Check your credentials and URL.[/red]"
        )
        raise typer.Exit(1)

@@ -126,27 +194,31 @@ def setup_wizard(enable_logging: bool = False):
    api_key = Prompt.ask("Enter API key", password=True)

    if not api_key.strip():
-        console.print("❌ [red]API key cannot be empty[/red]")
+        console.print("[red]Error:[/red] [red]API key cannot be empty[/red]")
        raise typer.Exit(1)

-    console.print("🔄 [blue]Validating API configuration...[/blue]")
+    console.print(
+        "[blue]Processing...[/blue] [blue]Validating API configuration...[/blue]"
+    )

    if validate_api_config(api_key, base_url):
        store_credentials(api_key, base_url)
-        console.print("✅ [green]API configuration stored successfully[/green]")
+        console.print(
+            "[green]Success:[/red] [green]API configuration stored successfully[/green]"
+        )

        # Create default config
        config_path = get_default_config_path()
        create_default_config(config_path)
        console.print(
-            f"✅ [green]Default configuration created at {config_path}[/green]"
+            f"[green]Success:[/red] [green]Default configuration created at {config_path}[/green]"
        )

        if enable_logging:
            console.print("📝 [yellow]Logging enabled (will output to stderr)[/yellow]")
    else:
        console.print(
-            "❌ [red]API validation failed. Check your credentials and URL.[/red]"
+            "[red]Error:[/red] [red]API validation failed. Check your credentials and URL.[/red]"
        )

        choice = Prompt.ask(
@@ -157,12 +229,14 @@ def setup_wizard(enable_logging: bool = False):

        if choice == "y":
            store_credentials(api_key, base_url)
-            console.print("⚠️ [yellow]Configuration stored without validation[/yellow]")
+            console.print(
+                "[yellow]Warning:[/yellow] [yellow]Configuration stored without validation[/yellow]"
+            )

            config_path = get_default_config_path()
            create_default_config(config_path)
            console.print(
-                f"✅ [green]Default configuration created at {config_path}[/green]"
+                f"[green]Success:[/red] [green]Default configuration created at {config_path}[/green]"
            )
        else:
            console.print("[yellow]Setup cancelled.[/yellow]")
@@ -174,7 +248,7 @@ def info():
    """Display current API and configuration info."""
    # API info
    if not has_credentials():
-        console.print("❌ [red]No API configuration found[/red]")
+        console.print("[red]Error:[/red] [red]No API configuration found[/red]")
        console.print("Run [cyan]taletorrent config --interactive[/cyan] to set up")
        raise typer.Exit(1)

@@ -193,7 +267,7 @@ def info():
        console.print(api_panel)

    except Exception as e:
-        console.print(f"❌ [red]Failed to get API info: {e}[/red]")
+        console.print(f"[red]Error:[/red] [red]Failed to get API info: {e}[/red]")
        raise typer.Exit(1)

    # Config info
@@ -219,9 +293,13 @@ def info():
            console.print(config_table)

        except Exception as e:
-            console.print(f"⚠️ [yellow]Config exists but invalid: {e}[/yellow]")
+            console.print(
+                f"[yellow]Warning:[/yellow] [yellow]Config exists but invalid: {e}[/yellow]"
+            )
    else:
-        console.print("ℹ️ [yellow]No game configuration found[/yellow]")
+        console.print(
+            "[yellow]Info:[/yellow] [yellow]No game configuration found[/yellow]"
+        )
        console.print(
            f"Run [cyan]taletorrent config[/cyan] to create default config at {config_path}"
        )
@@ -231,7 +309,9 @@ def info():
 def clear():
    """Clear stored API credentials."""
    if not has_credentials():
-        console.print("ℹ️ [yellow]No API credentials to clear[/yellow]")
+        console.print(
+            "[yellow]Info:[/yellow] [yellow]No API credentials to clear[/yellow]"
+        )
        return

    choice = Prompt.ask(
@@ -242,7 +322,7 @@ def clear():

    if choice == "y":
        delete_credentials()
-        console.print("✅ [green]API credentials cleared[/green]")
+        console.print("[green]Success:[/red] [green]API credentials cleared[/green]")
    else:
        console.print("[yellow]Operation cancelled.[/yellow]")

@@ -260,7 +340,7 @@ def play(
    try:
        # Check API credentials
        if not has_credentials():
-            console.print("❌ [red]No API configuration found[/red]")
+            console.print("[red]Error:[/red] [red]No API configuration found[/red]")
            console.print("Run [cyan]taletorrent config --interactive[/cyan] to set up")
            raise typer.Exit(1)

@@ -269,28 +349,38 @@ def play(
            config_path = get_default_config_path()
            if not config_path.exists():
                console.print(
-                    "ℹ️ [yellow]No configuration found, creating default...[/yellow]"
+                    "[yellow]Info:[/yellow] [yellow]No configuration found, creating default...[/yellow]"
                )
                create_default_config(config_path)

-        console.print(f"📁 [blue]Loading configuration from {config_path}[/blue]")
+        console.print(
+            f"[blue]Config:[/blue] [blue]Loading configuration from {config_path}[/blue]"
+        )
        config = GameConfig(config_path, enable_logging=enable_logging)
        config.validate()

        # Initialize components
-        console.print("🔄 [blue]Initializing game engine...[/blue]")
+        console.print(
+            "[blue]Processing...[/blue] [blue]Initializing game engine...[/blue]"
+        )
        ai_client = AIClient(config)
        prompt_manager = PromptManager(config)

-        # Test API connection
-        console.print("🔗 [blue]Testing API connection...[/blue]")
+        # Test API access
+        console.print("[blue]API:[/blue] [blue]Testing API access...[/blue]")
        if not ai_client.validate_model_availability():
            console.print(
-                "❌ [red]API models not available. Check your configuration.[/red]"
+                "[red]Error:[/red] [red]API models not available. Check your configuration.[/red]"
            )
            raise typer.Exit(1)

-        console.print("✅ [green]API connection successful[/green]")
+        if not ai_client.validate_api_access():
+            console.print(
+                "[red]Error:[/red] [red]API credentials invalid. Check your API key and configuration.[/red]"
+            )
+            raise typer.Exit(1)
+
+        console.print("[green]Success:[/red] [green]API access successful[/green]")

        # Interactive game setup
        console.print(
@@ -303,11 +393,13 @@ def play(

        # Get world setting
        console.print("\n[bold]World Setting:[/bold]")
-        setting = Prompt.ask("Enter the world/setting", default="A dark fantasy world")
-        writing_style = Prompt.ask(
+        setting = get_multiline_input(
+            "Enter the world/setting", default="A dark fantasy world"
+        )
+        writing_style = get_multiline_input(
            "Enter desired writing style", default="Gritty, descriptive, atmospheric"
        )
-        plot = Prompt.ask(
+        plot = get_multiline_input(
            "Enter initial plot",
            default="A mysterious artifact has been discovered in ancient ruins",
        )
@@ -321,8 +413,10 @@ def play(
        while True:
            console.print(f"\nCharacter #{len(characters) + 1}:")
            name = Prompt.ask("Character name")
-            external = Prompt.ask("External description (appearance, etc.)")
-            internal = Prompt.ask("Internal description (personality, motivations)")
+            external = get_multiline_input("External description (appearance, etc.)")
+            internal = get_multiline_input(
+                "Internal description (personality, motivations)"
+            )
            bsm = Prompt.ask("Initial body state", default="Healthy, alert")
            position = Prompt.ask(
                "Initial position", default="Standing in the entrance"
@@ -355,10 +449,10 @@ def play(
        game_loop(chat_history, characters, world, ai_client, prompt_manager, config)

    except GameError as e:
-        console.print(f"❌ [red]Game error: {e}[/red]")
+        console.print(f"[red]Failed to initialize game session: {e}[/red]")
        raise typer.Exit(1)
    except Exception as e:
-        console.print(f"❌ [red]Unexpected error: {e}[/red]")
+        console.print(f"[red]Unexpected error during game initialization: {e}[/red]")
        raise typer.Exit(1)


@@ -381,7 +475,7 @@ def game_loop(
        chat_history.append(narrator_turn)
        display_turn(narrator_turn)
    except GameError as e:
-        console.print(f"❌ [red]Failed to start game: {e}[/red]")
+        console.print(f"[red]Failed to generate initial narrator turn: {e}[/red]")
        return

    while True:
@@ -415,7 +509,9 @@ def game_loop(
                    chat_history, character, ai_client, prompt_manager
                )
            else:
-                console.print(f"❌ [red]Character not found: {character_name}[/red]")
+                console.print(
+                    f"[red]Error:[/red] [red]Character not found: {character_name}[/red]"
+                )
            continue
        elif command == "narrator":
            handle_narrator_turn(
@@ -424,7 +520,7 @@ def game_loop(
            continue
        else:
            console.print(
-                "❌ [red]Unknown command. Type 'help' for available commands.[/red]"
+                "[red]Error:[/red] [red]Unknown command. Type 'help' for available commands.[/red]"
            )


@@ -436,7 +532,9 @@ def handle_character_turn(
 ):
    """Handle a character turn."""
    try:
-        console.print(f"🎭 [blue]Generating turn for {character.name}...[/blue]")
+        console.print(
+            f"[blue]Character:[/blue] [blue]Generating turn for {character.name}...[/blue]"
+        )
        turn = char_turn(chat_history, character, ai_client, prompt_manager)
        chat_history.append(turn)
        display_turn(turn)
@@ -447,7 +545,9 @@ def handle_character_turn(
        character.position = updated_character.position

    except GameError as e:
-        console.print(f"❌ [red]Failed to generate character turn: {e}[/red]")
+        console.print(
+            f"[red]Error:[/red] [red]Failed to generate character turn: {e}[/red]"
+        )


 def handle_narrator_turn(
@@ -459,8 +559,8 @@ def handle_narrator_turn(
 ):
    """Handle a narrator turn."""
    try:
-        console.print("📖 [blue]Generating narrator turn...[/blue]")
-        guide = Prompt.ask(
+        console.print("[blue]Narrator:[/blue] [blue]Generating narrator turn...[/blue]")
+        guide = get_multiline_input(
            "Optional guide for narrator (press Enter to skip)", default=""
        )
        guide = guide if guide.strip() else None
@@ -472,7 +572,9 @@ def handle_narrator_turn(
        display_turn(turn)

    except GameError as e:
-        console.print(f"❌ [red]Failed to generate narrator turn: {e}[/red]")
+        console.print(
+            f"[red]Error:[/red] [red]Failed to generate narrator turn: {e}[/red]"
+        )


 def display_turn(turn: Turn):
@@ -533,15 +635,19 @@ def validate_config(
        config_path = get_default_config_path()

    if not config_path.exists():
-        console.print(f"❌ [red]Configuration file not found: {config_path}[/red]")
+        console.print(
+            f"[red]Error:[/red] [red]Configuration file not found: {config_path}[/red]"
+        )
        raise typer.Exit(1)

    try:
-        console.print(f"🔍 [blue]Validating configuration: {config_path}[/blue]")
+        console.print(
+            f"[blue]Validation:[/blue] [blue]Validating configuration: {config_path}[/blue]"
+        )
        config = GameConfig(config_path)
        config.validate()

-        console.print("✅ [green]Configuration is valid![/green]")
+        console.print("[green]Success:[/red] [green]Configuration is valid![/green]")

        # Show config summary
        console.print("\n[bold]Configuration Summary:[/bold]")
@@ -551,10 +657,10 @@ def validate_config(
        console.print(f"  Max Chat History: {config.get('game.max_chat_history')}")

    except ConfigError as e:
-        console.print(f"❌ [red]Configuration error: {e}[/red]")
+        console.print(f"[red]Error:[/red] [red]Configuration error: {e}[/red]")
        raise typer.Exit(1)
    except Exception as e:
-        console.print(f"❌ [red]Unexpected error: {e}[/red]")
+        console.print(f"[red]Error:[/red] [red]Unexpected error: {e}[/red]")
        raise typer.Exit(1)


--- a/taletorrent/config.py
+++ b/taletorrent/config.py
@@ -22,8 +22,7 @@ class GameConfig:
        "api": {
            "max_retries": 3,
            "timeout": 30,
-            "temperature": 0.7,
-            "max_tokens": 1000,
+            "max_input_chars": 4000,
        },
        "game": {"max_chat_history": 20, "enable_logging": False, "log_level": "INFO"},
        "prompts": {
@@ -139,7 +138,7 @@ Remember: you update character descriptions solely based on events described in
        except Exception as e:
            raise wrap_error(f"Failed to load configuration from {config_path}", e)

-    def _merge_configs(self, base: dict, override: dict) -> None:
+    def _merge_configs(self, base: dict[str, Any], override: dict[str, Any]) -> None:
        """Recursively merge two dictionaries."""
        for key, value in override.items():
            if key in base and isinstance(base[key], dict) and isinstance(value, dict):
@@ -212,10 +211,16 @@ Remember: you update character descriptions solely based on events described in
                raise ValidationError("max_retries must be non-negative")
            if api["timeout"] <= 0:
                raise ValidationError("timeout must be positive")
-            if not 0 <= api["temperature"] <= 2:
-                raise ValidationError("temperature must be between 0 and 2")
-            if api["max_tokens"] <= 0:
-                raise ValidationError("max_tokens must be positive")
+            if api["max_input_chars"] <= 0:
+                raise ValidationError("max_input_chars must be positive")
+
+            # Validate optional parameters if they exist
+            if "temperature" in api and api["temperature"] is not None:
+                if not 0 <= api["temperature"] <= 2:
+                    raise ValidationError("temperature must be between 0 and 2")
+            if "max_tokens" in api and api["max_tokens"] is not None:
+                if api["max_tokens"] <= 0:
+                    raise ValidationError("max_tokens must be positive")

            # Validate game settings
            game = self.get("game")