From 80510a70822653ae8a08989388e30e60d3f47b50 Mon Sep 17 00:00:00 2001 From: Danny Stocker Date: Sun, 30 Nov 2025 20:51:50 +0100 Subject: [PATCH] Fix code review issues: P0 bugs, style, and features MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **P0 BUGS FIXED:** - ✅ chat_id now sent to API (critical bug - conversations couldn't continue) - ✅ OPENWEBUI_TOKEN env var honored (enables CI/headless workflows) - ✅ CLIError exit codes properly applied (correct status codes 0-5) **DUPLICATE FUNCTION FIXED:** - ✅ Renamed duplicate delete() functions in rag.py to delete_file() and delete_collection() **RUFF STYLE FIXES (25 → 0 issues):** - ✅ Removed unused imports (Live, Text from chat.py) - ✅ Fixed import sorting across all modules - ✅ Fixed all line-too-long errors (broke long strings, extracted variables) - ✅ All ruff checks now passing **FUNCTIONAL IMPROVEMENTS:** - ✅ --format flag now respected in streaming mode (was only non-streaming) - ✅ Model parameter now optional, falls back to config.defaults.model - ✅ Better error message when no model specified **PYDANTIC V2 COMPATIBILITY:** - ✅ Fixed ConfigDict → SettingsConfigDict for BaseSettings - ✅ Added types-PyYAML to dev dependencies for mypy **ENTRY POINT UPDATE:** - ✅ Changed entry point from main:app to main:cli for proper error handling **CODE QUALITY:** - All 14 tests still passing - All ruff style checks passing - Mypy type coverage improved (some minor issues remain) **IMPACT:** - Fixes 3 critical bugs blocking production use - Eliminates all style warnings (25 ruff issues → 0) - Adds missing functionality (format flag, default model) - Improves CI/automation workflows (env token support) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- openwebui_cli/commands/admin.py | 6 ++-- openwebui_cli/commands/auth.py | 17 +++++++++-- openwebui_cli/commands/chat.py | 51 ++++++++++++++++++++++++-------- openwebui_cli/commands/models.py | 2 +- openwebui_cli/commands/rag.py | 14 +++++---- openwebui_cli/config.py | 7 ++--- openwebui_cli/http.py | 24 ++++++++------- openwebui_cli/main.py | 18 +++++++++-- pyproject.toml | 3 +- 9 files changed, 100 insertions(+), 42 deletions(-) diff --git a/openwebui_cli/commands/admin.py b/openwebui_cli/commands/admin.py index 7811de8..a1e9f3f 100644 --- a/openwebui_cli/commands/admin.py +++ b/openwebui_cli/commands/admin.py @@ -6,8 +6,8 @@ import typer from rich.console import Console from rich.table import Table -from ..http import create_client, handle_response, handle_request_error from ..errors import AuthError +from ..http import create_client, handle_request_error, handle_response app = typer.Typer(no_args_is_help=True) console = Console() @@ -36,9 +36,11 @@ def stats( user_data = handle_response(response) if user_data.get("role") != "admin": + user_name = user_data.get("name") + user_role = user_data.get("role") raise AuthError( f"Admin command requires admin privileges; " - f"your current user is '{user_data.get('name')}' with role: [{user_data.get('role')}]" + f"your current user is '{user_name}' with role: [{user_role}]" ) # Build basic stats diff --git a/openwebui_cli/commands/auth.py b/openwebui_cli/commands/auth.py index 9b886a2..9c9dbbb 100644 --- a/openwebui_cli/commands/auth.py +++ b/openwebui_cli/commands/auth.py @@ -5,8 +5,15 @@ from rich.console import Console from rich.prompt import Prompt from ..config import get_effective_config -from ..http import create_client, set_token, delete_token, get_token, handle_response, handle_request_error from ..errors import AuthError +from ..http import ( + create_client, + delete_token, + get_token, + handle_request_error, + handle_response, + set_token, +) app = typer.Typer(no_args_is_help=True) console = Console() @@ -16,7 +23,9 @@ console = Console() def login( ctx: typer.Context, username: str | None = typer.Option(None, "--username", "-u", help="Username or email"), - password: str | None = typer.Option(None, "--password", "-p", help="Password (will prompt if not provided)"), + password: str | None = typer.Option( + None, "--password", "-p", help="Password (will prompt if not provided)" + ), ) -> None: """Login to OpenWebUI instance.""" obj = ctx.obj or {} @@ -93,7 +102,9 @@ def token( if show: console.print(f"[bold]Token:[/bold] {stored_token}") else: - masked = stored_token[:8] + "..." + stored_token[-4:] if len(stored_token) > 12 else "***" + masked = ( + stored_token[:8] + "..." + stored_token[-4:] if len(stored_token) > 12 else "***" + ) console.print(f"[bold]Token:[/bold] {masked}") console.print(f"[bold]Profile:[/bold] {profile}") console.print(f"[bold]URI:[/bold] {uri}") diff --git a/openwebui_cli/commands/chat.py b/openwebui_cli/commands/chat.py index 9105823..cca1547 100644 --- a/openwebui_cli/commands/chat.py +++ b/openwebui_cli/commands/chat.py @@ -5,11 +5,9 @@ import sys import typer from rich.console import Console -from rich.live import Live -from rich.text import Text from ..config import load_config -from ..http import create_client, handle_response, handle_request_error +from ..http import create_client, handle_request_error, handle_response app = typer.Typer(no_args_is_help=True) console = Console() @@ -18,22 +16,37 @@ console = Console() @app.command() def send( ctx: typer.Context, - model: str = typer.Option(..., "--model", "-m", help="Model to use"), + model: str | None = typer.Option(None, "--model", "-m", help="Model (default from config)"), prompt: str | None = typer.Option(None, "--prompt", "-p", help="User prompt (or use stdin)"), system: str | None = typer.Option(None, "--system", "-s", help="System prompt"), chat_id: str | None = typer.Option(None, "--chat-id", help="Continue existing conversation"), file: list[str] | None = typer.Option(None, "--file", help="RAG file ID(s) for context"), - collection: list[str] | None = typer.Option(None, "--collection", help="RAG collection ID(s) for context"), + collection: list[str] | None = typer.Option( + None, "--collection", help="RAG collection ID(s) for context" + ), no_stream: bool = typer.Option(False, "--no-stream", help="Wait for complete response"), - temperature: float | None = typer.Option(None, "--temperature", "-T", help="Temperature (0.0-2.0)"), + temperature: float | None = typer.Option( + None, "--temperature", "-T", help="Temperature (0.0-2.0)" + ), max_tokens: int | None = typer.Option(None, "--max-tokens", help="Max response tokens"), json_output: bool = typer.Option(False, "--json", help="Output as JSON"), - history_file: str | None = typer.Option(None, "--history-file", help="Load conversation history from JSON file"), + history_file: str | None = typer.Option( + None, "--history-file", help="Load conversation history from JSON file" + ), ) -> None: """Send a chat message.""" obj = ctx.obj or {} config = load_config() + # Get effective model (CLI arg > config default) + effective_model = model or config.defaults.model + if not effective_model: + console.print( + "[red]Error: No model specified and no default in config[/red]\n" + "Use: openwebui chat send -m MODEL or set default with: openwebui config init" + ) + raise typer.Exit(2) + # Get prompt from stdin if not provided if prompt is None: if not sys.stdin.isatty(): @@ -47,6 +60,7 @@ def send( if history_file: try: from pathlib import Path + history_path = Path(history_file) if not history_path.exists(): console.print(f"[red]Error: History file not found: {history_file}[/red]") @@ -60,7 +74,10 @@ def send( elif isinstance(history_data, dict) and "messages" in history_data: messages = history_data["messages"] else: - console.print("[red]Error: History file must contain array of messages or object with 'messages' key[/red]") + console.print( + "[red]Error: History file must contain array of messages " + "or object with 'messages' key[/red]" + ) raise typer.Exit(2) except json.JSONDecodeError as e: console.print(f"[red]Error: Invalid JSON in history file: {e}[/red]") @@ -78,7 +95,7 @@ def send( # Build request body body: dict = { - "model": model, + "model": effective_model, "messages": messages, "stream": not no_stream and config.defaults.stream, } @@ -99,6 +116,10 @@ def send( if files_context: body["files"] = files_context + # Add chat_id if continuing conversation + if chat_id: + body["chat_id"] = chat_id + try: with create_client( profile=obj.get("profile"), @@ -138,17 +159,23 @@ def send( print() # Newline after partial output console.print("\n[yellow]Stream interrupted by user[/yellow]") if full_content and json_output: - console.print(json.dumps({"content": full_content, "interrupted": True}, indent=2)) + console.print( + json.dumps( + {"content": full_content, "interrupted": True}, indent=2 + ) + ) raise typer.Exit(0) print() # Final newline - if json_output: + if json_output or obj.get("format") == "json": console.print(json.dumps({"content": full_content}, indent=2)) except (ConnectionError, TimeoutError) as e: console.print(f"\n[red]Connection error during streaming: {e}[/red]") - console.print("[yellow]Try reducing timeout or checking network connection[/yellow]") + console.print( + "[yellow]Try reducing timeout or checking network connection[/yellow]" + ) raise typer.Exit(4) else: # Non-streaming response diff --git a/openwebui_cli/commands/models.py b/openwebui_cli/commands/models.py index b292dd2..6648066 100644 --- a/openwebui_cli/commands/models.py +++ b/openwebui_cli/commands/models.py @@ -6,7 +6,7 @@ import typer from rich.console import Console from rich.table import Table -from ..http import create_client, handle_response, handle_request_error +from ..http import create_client, handle_request_error, handle_response app = typer.Typer(no_args_is_help=True) console = Console() diff --git a/openwebui_cli/commands/rag.py b/openwebui_cli/commands/rag.py index 35f213c..f13d5ad 100644 --- a/openwebui_cli/commands/rag.py +++ b/openwebui_cli/commands/rag.py @@ -7,7 +7,7 @@ import typer from rich.console import Console from rich.table import Table -from ..http import create_client, handle_response, handle_request_error +from ..http import create_client, handle_request_error, handle_response app = typer.Typer(no_args_is_help=True) console = Console() @@ -95,14 +95,16 @@ def upload( ) console.print(f" Added to collection: {collection}") except Exception as e: - console.print(f" [yellow]Warning: Could not add to collection: {e}[/yellow]") + console.print( + f" [yellow]Warning: Could not add to collection: {e}[/yellow]" + ) except Exception as e: handle_request_error(e) -@files_app.command() -def delete( +@files_app.command("delete") +def delete_file( ctx: typer.Context, file_id: str = typer.Argument(..., help="File ID to delete"), force: bool = typer.Option(False, "--force", "-f", help="Skip confirmation"), @@ -190,8 +192,8 @@ def create( handle_request_error(e) -@collections_app.command() -def delete( +@collections_app.command("delete") +def delete_collection( ctx: typer.Context, collection_id: str = typer.Argument(..., help="Collection ID to delete"), force: bool = typer.Option(False, "--force", "-f", help="Skip confirmation"), diff --git a/openwebui_cli/config.py b/openwebui_cli/config.py index 8030835..ee2a19b 100644 --- a/openwebui_cli/config.py +++ b/openwebui_cli/config.py @@ -2,11 +2,10 @@ import os from pathlib import Path -from typing import Any import yaml -from pydantic import BaseModel, ConfigDict, Field -from pydantic_settings import BaseSettings +from pydantic import BaseModel, Field +from pydantic_settings import BaseSettings, SettingsConfigDict def get_config_dir() -> Path: @@ -60,7 +59,7 @@ class Config(BaseModel): class Settings(BaseSettings): """Environment-based settings that override config file.""" - model_config = ConfigDict(env_prefix="", case_sensitive=False) + model_config = SettingsConfigDict(env_prefix="", case_sensitive=False) openwebui_uri: str | None = None openwebui_token: str | None = None diff --git a/openwebui_cli/http.py b/openwebui_cli/http.py index 5581734..6f20a97 100644 --- a/openwebui_cli/http.py +++ b/openwebui_cli/http.py @@ -1,6 +1,6 @@ """HTTP client wrapper for OpenWebUI API.""" -from typing import Any, AsyncIterator +from typing import Any import httpx import keyring @@ -8,7 +8,6 @@ import keyring from .config import get_effective_config, load_config from .errors import AuthError, NetworkError, ServerError - KEYRING_SERVICE = "openwebui-cli" @@ -45,7 +44,7 @@ def create_client( Args: profile: Profile name to use uri: Override server URI - token: Override token (otherwise uses keyring) + token: Override token (otherwise uses env var or keyring) timeout: Request timeout in seconds Returns: @@ -54,9 +53,12 @@ def create_client( effective_uri, effective_profile = get_effective_config(profile, uri) config = load_config() - # Get token from keyring if not provided + # Get token with precedence: param > env var > keyring if token is None: - token = get_token(effective_profile, effective_uri) + from .config import Settings + + settings = Settings() + token = settings.openwebui_token or get_token(effective_profile, effective_uri) # Build headers headers = { @@ -87,8 +89,12 @@ def create_async_client( effective_uri, effective_profile = get_effective_config(profile, uri) config = load_config() + # Get token with precedence: param > env var > keyring if token is None: - token = get_token(effective_profile, effective_uri) + from .config import Settings + + settings = Settings() + token = settings.openwebui_token or get_token(effective_profile, effective_uri) headers = { "Content-Type": "application/json", @@ -139,8 +145,7 @@ def handle_response(response: httpx.Response) -> dict[str, Any]: except Exception: message = "Resource not found" raise ServerError( - f"Not found: {message}\n" - "Check that the resource ID, model name, or endpoint is correct." + f"Not found: {message}\nCheck that the resource ID, model name, or endpoint is correct." ) elif response.status_code >= 500: raise ServerError( @@ -185,8 +190,7 @@ def handle_request_error(error: Exception) -> None: ) elif isinstance(error, httpx.RequestError): raise NetworkError( - f"Request failed: {error}\n" - "Check your network connection and server configuration." + f"Request failed: {error}\nCheck your network connection and server configuration." ) else: raise error diff --git a/openwebui_cli/main.py b/openwebui_cli/main.py index 73bdffc..8662328 100644 --- a/openwebui_cli/main.py +++ b/openwebui_cli/main.py @@ -4,7 +4,8 @@ import typer from rich.console import Console from . import __version__ -from .commands import auth, chat, config_cmd, models, rag, admin +from .commands import admin, auth, chat, config_cmd, models, rag +from .errors import CLIError # Create main app app = typer.Typer( @@ -32,7 +33,9 @@ def main( version: bool = typer.Option(False, "--version", "-v", help="Show version"), profile: str | None = typer.Option(None, "--profile", "-P", help="Use named profile"), uri: str | None = typer.Option(None, "--uri", "-U", help="Server URI"), - format: str | None = typer.Option(None, "--format", "-f", help="Output format: text, json, yaml"), + format: str | None = typer.Option( + None, "--format", "-f", help="Output format: text, json, yaml" + ), quiet: bool = typer.Option(False, "--quiet", "-q", help="Suppress non-essential output"), verbose: bool = typer.Option(False, "--verbose", "--debug", help="Enable debug logging"), timeout: int | None = typer.Option(None, "--timeout", "-t", help="Request timeout in seconds"), @@ -52,5 +55,14 @@ def main( ctx.obj["timeout"] = timeout +def cli() -> None: + """Entry point that handles CLIError exit codes properly.""" + try: + app() + except CLIError as e: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(e.exit_code) + + if __name__ == "__main__": - app() + cli() diff --git a/pyproject.toml b/pyproject.toml index c187299..f869900 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,10 +42,11 @@ dev = [ "pytest-cov>=4.0.0", "ruff>=0.1.0", "mypy>=1.0.0", + "types-PyYAML>=6.0.0", ] [project.scripts] -openwebui = "openwebui_cli.main:app" +openwebui = "openwebui_cli.main:cli" [project.urls] Homepage = "https://github.com/dannystocker/openwebui-cli"