Fix code review issues: P0 bugs, style, and features

**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 <noreply@anthropic.com>
This commit is contained in:
Danny Stocker 2025-11-30 20:51:50 +01:00
parent fbe6832d3e
commit 80510a7082
9 changed files with 100 additions and 42 deletions

View file

@ -6,8 +6,8 @@ import typer
from rich.console import Console from rich.console import Console
from rich.table import Table from rich.table import Table
from ..http import create_client, handle_response, handle_request_error
from ..errors import AuthError from ..errors import AuthError
from ..http import create_client, handle_request_error, handle_response
app = typer.Typer(no_args_is_help=True) app = typer.Typer(no_args_is_help=True)
console = Console() console = Console()
@ -36,9 +36,11 @@ def stats(
user_data = handle_response(response) user_data = handle_response(response)
if user_data.get("role") != "admin": if user_data.get("role") != "admin":
user_name = user_data.get("name")
user_role = user_data.get("role")
raise AuthError( raise AuthError(
f"Admin command requires admin privileges; " 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 # Build basic stats

View file

@ -5,8 +5,15 @@ from rich.console import Console
from rich.prompt import Prompt from rich.prompt import Prompt
from ..config import get_effective_config 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 ..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) app = typer.Typer(no_args_is_help=True)
console = Console() console = Console()
@ -16,7 +23,9 @@ console = Console()
def login( def login(
ctx: typer.Context, ctx: typer.Context,
username: str | None = typer.Option(None, "--username", "-u", help="Username or email"), 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: ) -> None:
"""Login to OpenWebUI instance.""" """Login to OpenWebUI instance."""
obj = ctx.obj or {} obj = ctx.obj or {}
@ -93,7 +102,9 @@ def token(
if show: if show:
console.print(f"[bold]Token:[/bold] {stored_token}") console.print(f"[bold]Token:[/bold] {stored_token}")
else: 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]Token:[/bold] {masked}")
console.print(f"[bold]Profile:[/bold] {profile}") console.print(f"[bold]Profile:[/bold] {profile}")
console.print(f"[bold]URI:[/bold] {uri}") console.print(f"[bold]URI:[/bold] {uri}")

View file

@ -5,11 +5,9 @@ import sys
import typer import typer
from rich.console import Console from rich.console import Console
from rich.live import Live
from rich.text import Text
from ..config import load_config 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) app = typer.Typer(no_args_is_help=True)
console = Console() console = Console()
@ -18,22 +16,37 @@ console = Console()
@app.command() @app.command()
def send( def send(
ctx: typer.Context, 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)"), 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"), system: str | None = typer.Option(None, "--system", "-s", help="System prompt"),
chat_id: str | None = typer.Option(None, "--chat-id", help="Continue existing conversation"), 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"), 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"), 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"), max_tokens: int | None = typer.Option(None, "--max-tokens", help="Max response tokens"),
json_output: bool = typer.Option(False, "--json", help="Output as JSON"), 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: ) -> None:
"""Send a chat message.""" """Send a chat message."""
obj = ctx.obj or {} obj = ctx.obj or {}
config = load_config() 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 # Get prompt from stdin if not provided
if prompt is None: if prompt is None:
if not sys.stdin.isatty(): if not sys.stdin.isatty():
@ -47,6 +60,7 @@ def send(
if history_file: if history_file:
try: try:
from pathlib import Path from pathlib import Path
history_path = Path(history_file) history_path = Path(history_file)
if not history_path.exists(): if not history_path.exists():
console.print(f"[red]Error: History file not found: {history_file}[/red]") 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: elif isinstance(history_data, dict) and "messages" in history_data:
messages = history_data["messages"] messages = history_data["messages"]
else: 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) raise typer.Exit(2)
except json.JSONDecodeError as e: except json.JSONDecodeError as e:
console.print(f"[red]Error: Invalid JSON in history file: {e}[/red]") console.print(f"[red]Error: Invalid JSON in history file: {e}[/red]")
@ -78,7 +95,7 @@ def send(
# Build request body # Build request body
body: dict = { body: dict = {
"model": model, "model": effective_model,
"messages": messages, "messages": messages,
"stream": not no_stream and config.defaults.stream, "stream": not no_stream and config.defaults.stream,
} }
@ -99,6 +116,10 @@ def send(
if files_context: if files_context:
body["files"] = files_context body["files"] = files_context
# Add chat_id if continuing conversation
if chat_id:
body["chat_id"] = chat_id
try: try:
with create_client( with create_client(
profile=obj.get("profile"), profile=obj.get("profile"),
@ -138,17 +159,23 @@ def send(
print() # Newline after partial output print() # Newline after partial output
console.print("\n[yellow]Stream interrupted by user[/yellow]") console.print("\n[yellow]Stream interrupted by user[/yellow]")
if full_content and json_output: 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) raise typer.Exit(0)
print() # Final newline print() # Final newline
if json_output: if json_output or obj.get("format") == "json":
console.print(json.dumps({"content": full_content}, indent=2)) console.print(json.dumps({"content": full_content}, indent=2))
except (ConnectionError, TimeoutError) as e: except (ConnectionError, TimeoutError) as e:
console.print(f"\n[red]Connection error during streaming: {e}[/red]") 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) raise typer.Exit(4)
else: else:
# Non-streaming response # Non-streaming response

View file

@ -6,7 +6,7 @@ import typer
from rich.console import Console from rich.console import Console
from rich.table import Table 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) app = typer.Typer(no_args_is_help=True)
console = Console() console = Console()

View file

@ -7,7 +7,7 @@ import typer
from rich.console import Console from rich.console import Console
from rich.table import Table 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) app = typer.Typer(no_args_is_help=True)
console = Console() console = Console()
@ -95,14 +95,16 @@ def upload(
) )
console.print(f" Added to collection: {collection}") console.print(f" Added to collection: {collection}")
except Exception as e: 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: except Exception as e:
handle_request_error(e) handle_request_error(e)
@files_app.command() @files_app.command("delete")
def delete( def delete_file(
ctx: typer.Context, ctx: typer.Context,
file_id: str = typer.Argument(..., help="File ID to delete"), file_id: str = typer.Argument(..., help="File ID to delete"),
force: bool = typer.Option(False, "--force", "-f", help="Skip confirmation"), force: bool = typer.Option(False, "--force", "-f", help="Skip confirmation"),
@ -190,8 +192,8 @@ def create(
handle_request_error(e) handle_request_error(e)
@collections_app.command() @collections_app.command("delete")
def delete( def delete_collection(
ctx: typer.Context, ctx: typer.Context,
collection_id: str = typer.Argument(..., help="Collection ID to delete"), collection_id: str = typer.Argument(..., help="Collection ID to delete"),
force: bool = typer.Option(False, "--force", "-f", help="Skip confirmation"), force: bool = typer.Option(False, "--force", "-f", help="Skip confirmation"),

View file

@ -2,11 +2,10 @@
import os import os
from pathlib import Path from pathlib import Path
from typing import Any
import yaml import yaml
from pydantic import BaseModel, ConfigDict, Field from pydantic import BaseModel, Field
from pydantic_settings import BaseSettings from pydantic_settings import BaseSettings, SettingsConfigDict
def get_config_dir() -> Path: def get_config_dir() -> Path:
@ -60,7 +59,7 @@ class Config(BaseModel):
class Settings(BaseSettings): class Settings(BaseSettings):
"""Environment-based settings that override config file.""" """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_uri: str | None = None
openwebui_token: str | None = None openwebui_token: str | None = None

View file

@ -1,6 +1,6 @@
"""HTTP client wrapper for OpenWebUI API.""" """HTTP client wrapper for OpenWebUI API."""
from typing import Any, AsyncIterator from typing import Any
import httpx import httpx
import keyring import keyring
@ -8,7 +8,6 @@ import keyring
from .config import get_effective_config, load_config from .config import get_effective_config, load_config
from .errors import AuthError, NetworkError, ServerError from .errors import AuthError, NetworkError, ServerError
KEYRING_SERVICE = "openwebui-cli" KEYRING_SERVICE = "openwebui-cli"
@ -45,7 +44,7 @@ def create_client(
Args: Args:
profile: Profile name to use profile: Profile name to use
uri: Override server URI uri: Override server URI
token: Override token (otherwise uses keyring) token: Override token (otherwise uses env var or keyring)
timeout: Request timeout in seconds timeout: Request timeout in seconds
Returns: Returns:
@ -54,9 +53,12 @@ def create_client(
effective_uri, effective_profile = get_effective_config(profile, uri) effective_uri, effective_profile = get_effective_config(profile, uri)
config = load_config() config = load_config()
# Get token from keyring if not provided # Get token with precedence: param > env var > keyring
if token is None: 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 # Build headers
headers = { headers = {
@ -87,8 +89,12 @@ def create_async_client(
effective_uri, effective_profile = get_effective_config(profile, uri) effective_uri, effective_profile = get_effective_config(profile, uri)
config = load_config() config = load_config()
# Get token with precedence: param > env var > keyring
if token is None: 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 = { headers = {
"Content-Type": "application/json", "Content-Type": "application/json",
@ -139,8 +145,7 @@ def handle_response(response: httpx.Response) -> dict[str, Any]:
except Exception: except Exception:
message = "Resource not found" message = "Resource not found"
raise ServerError( raise ServerError(
f"Not found: {message}\n" f"Not found: {message}\nCheck that the resource ID, model name, or endpoint is correct."
"Check that the resource ID, model name, or endpoint is correct."
) )
elif response.status_code >= 500: elif response.status_code >= 500:
raise ServerError( raise ServerError(
@ -185,8 +190,7 @@ def handle_request_error(error: Exception) -> None:
) )
elif isinstance(error, httpx.RequestError): elif isinstance(error, httpx.RequestError):
raise NetworkError( raise NetworkError(
f"Request failed: {error}\n" f"Request failed: {error}\nCheck your network connection and server configuration."
"Check your network connection and server configuration."
) )
else: else:
raise error raise error

View file

@ -4,7 +4,8 @@ import typer
from rich.console import Console from rich.console import Console
from . import __version__ 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 # Create main app
app = typer.Typer( app = typer.Typer(
@ -32,7 +33,9 @@ def main(
version: bool = typer.Option(False, "--version", "-v", help="Show version"), version: bool = typer.Option(False, "--version", "-v", help="Show version"),
profile: str | None = typer.Option(None, "--profile", "-P", help="Use named profile"), profile: str | None = typer.Option(None, "--profile", "-P", help="Use named profile"),
uri: str | None = typer.Option(None, "--uri", "-U", help="Server URI"), 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"), quiet: bool = typer.Option(False, "--quiet", "-q", help="Suppress non-essential output"),
verbose: bool = typer.Option(False, "--verbose", "--debug", help="Enable debug logging"), verbose: bool = typer.Option(False, "--verbose", "--debug", help="Enable debug logging"),
timeout: int | None = typer.Option(None, "--timeout", "-t", help="Request timeout in seconds"), timeout: int | None = typer.Option(None, "--timeout", "-t", help="Request timeout in seconds"),
@ -52,5 +55,14 @@ def main(
ctx.obj["timeout"] = timeout 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__": if __name__ == "__main__":
app() cli()

View file

@ -42,10 +42,11 @@ dev = [
"pytest-cov>=4.0.0", "pytest-cov>=4.0.0",
"ruff>=0.1.0", "ruff>=0.1.0",
"mypy>=1.0.0", "mypy>=1.0.0",
"types-PyYAML>=6.0.0",
] ]
[project.scripts] [project.scripts]
openwebui = "openwebui_cli.main:app" openwebui = "openwebui_cli.main:cli"
[project.urls] [project.urls]
Homepage = "https://github.com/dannystocker/openwebui-cli" Homepage = "https://github.com/dannystocker/openwebui-cli"