From 81e14724fa51faba8e064c78cdaaa72bb2eac660 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:23:35 -0700 Subject: [PATCH] refactor(hermes_cli/web_routers): fold repeated 403/500 I/O error mapping into files._io_errors; compact call sites --- hermes_cli/web_routers/cron.py | 19 +---- hermes_cli/web_routers/files.py | 130 ++++++++++---------------------- hermes_cli/web_routers/mcp.py | 29 ++----- hermes_cli/web_routers/ops.py | 77 +++++-------------- 4 files changed, 67 insertions(+), 188 deletions(-) diff --git a/hermes_cli/web_routers/cron.py b/hermes_cli/web_routers/cron.py index da7b15d053..a9784e199a 100644 --- a/hermes_cli/web_routers/cron.py +++ b/hermes_cli/web_routers/cron.py @@ -15,11 +15,7 @@ from fastapi import APIRouter, HTTPException, Request from fastapi.responses import JSONResponse from hermes_cli.web_deps import late -from hermes_cli.web_models import ( - AutomationBlueprintInstantiate, - CronJobCreate, - CronJobUpdate, -) +from hermes_cli.web_models import AutomationBlueprintInstantiate, CronJobCreate, CronJobUpdate from hermes_cli.web_routers._common import log as _log router = APIRouter() @@ -138,10 +134,7 @@ def _list_cron_job_runs_sync(job_id: str, profile: Optional[str] = None, limit: runs = db.list_cron_job_runs(canonical, limit=limit_n, offset=0) now = time.time() for s in runs: - s["is_active"] = ( - s.get("ended_at") is None - and (now - s.get("last_active", s.get("started_at", 0))) < 300 - ) + s["is_active"] = s.get("ended_at") is None and (now - s.get("last_active", s.get("started_at", 0))) < 300 s["archived"] = bool(s.get("archived")) if selected: s["profile"] = selected @@ -195,10 +188,7 @@ def _trigger_cron_job_sync(job_id: str, profile: Optional[str] = None): if refreshed and refreshed.get("last_run_at") != job.get("last_run_at"): return refreshed if not ran: - raise HTTPException( - status_code=409, - detail="Job is already running or was claimed by another scheduler", - ) + raise HTTPException(status_code=409, detail="Job is already running or was claimed by another scheduler") if refreshed: return refreshed # A one-shot may remove itself after exhausting repeat=1: keep the response @@ -356,8 +346,7 @@ async def cron_fire_webhook(request: Request): return JSONResponse( { "status": "gateway_stopped", - "detail": "gateway deliberately stopped; fire dropped, " - "jobs re-arm on next gateway start", + "detail": "gateway deliberately stopped; fire dropped, jobs re-arm on next gateway start", "job_id": job_id, "profile": profile, }, diff --git a/hermes_cli/web_routers/files.py b/hermes_cli/web_routers/files.py index 681c8d3c2b..0daa58fb4e 100644 --- a/hermes_cli/web_routers/files.py +++ b/hermes_cli/web_routers/files.py @@ -7,6 +7,7 @@ reached through the late-binding seam (cycle-safe). import asyncio import base64 import binascii +import contextlib import mimetypes import os import re @@ -26,11 +27,7 @@ from fastapi.responses import FileResponse from hermes_cli._subprocess_compat import windows_hide_flags from hermes_cli.web_deps import late from hermes_cli.web_models import ( - ChatImageUpload, - FsWriteText, - ManagedDirectoryCreate, - ManagedFileDelete, - ManagedFileUpload, + ChatImageUpload, FsWriteText, ManagedDirectoryCreate, ManagedFileDelete, ManagedFileUpload, ) router = APIRouter() @@ -48,14 +45,8 @@ load_config = late("load_config") # Image types GET /api/media serves — extension-allowlisted so an authenticated # caller can't pull non-image files through it. _MEDIA_CONTENT_TYPES = { - ".png": "image/png", - ".jpg": "image/jpeg", - ".jpeg": "image/jpeg", - ".gif": "image/gif", - ".webp": "image/webp", - ".svg": "image/svg+xml", - ".bmp": "image/bmp", - ".ico": "image/x-icon", + ".png": "image/png", ".jpg": "image/jpeg", ".jpeg": "image/jpeg", ".gif": "image/gif", + ".webp": "image/webp", ".svg": "image/svg+xml", ".bmp": "image/bmp", ".ico": "image/x-icon", } _MEDIA_MAX_BYTES = 25 * 1024 * 1024 @@ -74,17 +65,9 @@ _FS_READDIR_HIDDEN = { # (agent.file_safety.get_read_block_error, gateway.platforms.base # ._ROOT_CREDENTIAL_FILES) so the Files tab never lags behind them. _SENSITIVE_MANAGED_FILE_BASENAMES = frozenset({ - "auth.json", - "auth.lock", - "credentials", - "config.yaml", - ".anthropic_oauth.json", - "google_token.json", - "google_oauth_pending.json", - "google_oauth.json", - "webhook_subscriptions.json", - "bws_cache.json", - "bws_cache.enc.json", + "auth.json", "auth.lock", "credentials", "config.yaml", ".anthropic_oauth.json", + "google_token.json", "google_oauth_pending.json", "google_oauth.json", + "webhook_subscriptions.json", "bws_cache.json", "bws_cache.enc.json", ".git-credentials", # git's credential-store cache (file_safety blocks it too) }) @@ -132,24 +115,11 @@ _FS_PREVIEW_LANGUAGE_BY_EXT = { } _FS_MIME_TYPES = { - ".avi": "video/x-msvideo", - ".bmp": "image/bmp", - ".flac": "audio/flac", - ".gif": "image/gif", - ".jpeg": "image/jpeg", - ".jpg": "image/jpeg", - ".m4a": "audio/mp4", - ".mkv": "video/x-matroska", - ".mov": "video/quicktime", - ".mp3": "audio/mpeg", - ".mp4": "video/mp4", - ".ogg": "audio/ogg", - ".opus": "audio/ogg; codecs=opus", - ".png": "image/png", - ".svg": "image/svg+xml", - ".wav": "audio/wav", - ".webm": "video/webm", - ".webp": "image/webp", + ".avi": "video/x-msvideo", ".bmp": "image/bmp", ".flac": "audio/flac", ".gif": "image/gif", + ".jpeg": "image/jpeg", ".jpg": "image/jpeg", ".m4a": "audio/mp4", ".mkv": "video/x-matroska", + ".mov": "video/quicktime", ".mp3": "audio/mpeg", ".mp4": "video/mp4", ".ogg": "audio/ogg", + ".opus": "audio/ogg; codecs=opus", ".png": "image/png", ".svg": "image/svg+xml", + ".wav": "audio/wav", ".webm": "video/webm", ".webp": "image/webp", } @@ -170,6 +140,17 @@ def _fs_looks_binary(data: bytes) -> bool: return suspicious / len(data) > 0.12 +@contextlib.contextmanager +def _io_errors(denied: str, failed: str): + """PermissionError -> 403 ``denied``; other OSError -> 500 ``": "``.""" + try: + yield + except PermissionError: + raise HTTPException(status_code=403, detail=denied) + except OSError as exc: + raise HTTPException(status_code=500, detail=f"{failed}: {exc}") + + def _fs_regular_file(path: Path) -> tuple[Path, os.stat_result]: target = _fs_path(str(path)) try: @@ -298,11 +279,8 @@ def _decode_data_url(data_url: str) -> tuple[bytes, str]: _CHAT_IMAGE_UPLOAD_MAX_BYTES = 25 * 1024 * 1024 _CHAT_IMAGE_ALLOWED_EXTENSIONS = frozenset({".png", ".jpg", ".jpeg", ".gif", ".webp", ".bmp"}) _CHAT_IMAGE_MAGIC: tuple[tuple[bytes, str], ...] = ( - (b"\x89PNG\r\n\x1a\n", ".png"), - (b"\xff\xd8\xff", ".jpg"), - (b"GIF87a", ".gif"), - (b"GIF89a", ".gif"), - (b"BM", ".bmp"), + (b"\x89PNG\r\n\x1a\n", ".png"), (b"\xff\xd8\xff", ".jpg"), + (b"GIF87a", ".gif"), (b"GIF89a", ".gif"), (b"BM", ".bmp"), ) @@ -348,23 +326,15 @@ async def upload_chat_image(payload: ChatImageUpload, profile: Optional[str] = N data, mime_type, ext = _decode_chat_image_upload(payload) with _profile_scope(profile) as scoped_home: img_dir = Path(scoped_home or get_hermes_home()) / "images" - try: + with _io_errors("Image directory is not writable", "Could not create image directory"): img_dir.mkdir(parents=True, exist_ok=True) - except PermissionError: - raise HTTPException(status_code=403, detail="Image directory is not writable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not create image directory: {exc}") stem = Path(_sanitize_chat_image_filename(payload.filename)).stem or "pasted-image" stem = re.sub(r"[^A-Za-z0-9_.-]+", "_", stem).strip("._-") or "pasted-image" ts = datetime.now().strftime("%Y%m%d_%H%M%S") target = img_dir / f"dashboard_{ts}_{secrets.token_hex(4)}_{stem}{ext}" - try: + with _io_errors("Image directory is not writable", "Could not write image"): target.write_bytes(data) - except PermissionError: - raise HTTPException(status_code=403, detail="Image directory is not writable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not write image: {exc}") return { "ok": True, @@ -388,17 +358,12 @@ async def list_managed_files(request: Request, path: Optional[str] = None): if not target.is_dir(): raise HTTPException(status_code=400, detail="Path is not a directory") - try: - with os.scandir(target) as scan: - entries = [ - _managed_file_entry(policy, Path(entry.path)) - for entry in scan - if not _is_sensitive_path(Path(entry.path)) - ] - except PermissionError: - raise HTTPException(status_code=403, detail="Directory is not readable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not read directory: {exc}") + with _io_errors("Directory is not readable", "Could not read directory"), os.scandir(target) as scan: + entries = [ + _managed_file_entry(policy, Path(entry.path)) + for entry in scan + if not _is_sensitive_path(Path(entry.path)) + ] entries.sort(key=lambda item: (not item["is_directory"], str(item["name"]).lower())) locked_root = policy.locked_root @@ -420,7 +385,8 @@ def _managed_readable_file(request: Request, path: str) -> tuple[Any, Path, str, raise HTTPException(status_code=400, detail="Path is not a file") if _is_sensitive_path(target): raise HTTPException(status_code=403, detail="Access to sensitive files is not allowed") - return policy, target, display_path, _MANAGED_FILE_MAX_BYTES, mimetypes.guess_type(target.name)[0] or "application/octet-stream" + mime_type = mimetypes.guess_type(target.name)[0] or "application/octet-stream" + return policy, target, display_path, _MANAGED_FILE_MAX_BYTES, mime_type def _managed_file_size(target: Path, max_bytes: int) -> int: @@ -437,12 +403,8 @@ def _managed_file_size(target: Path, max_bytes: int) -> int: async def read_managed_file(request: Request, path: str): policy, target, display_path, max_bytes, mime_type = _managed_readable_file(request, path) size = _managed_file_size(target, max_bytes) - try: + with _io_errors("File is not readable", "Could not read file"): encoded = base64.b64encode(target.read_bytes()).decode("ascii") - except PermissionError: - raise HTTPException(status_code=403, detail="File is not readable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not read file: {exc}") return { "name": target.name, "path": display_path, @@ -526,13 +488,9 @@ def _managed_write_result(policy, target: Path, display_path: str) -> dict: async def upload_managed_file(payload: ManagedFileUpload, request: Request): policy, target, display_path = _managed_write_target(payload.path, request, payload.overwrite) data, _mime_type = _decode_data_url(payload.data_url) - try: + with _io_errors("File is not writable", "Could not write file"): target.parent.mkdir(parents=True, exist_ok=True) target.write_bytes(data) - except PermissionError: - raise HTTPException(status_code=403, detail="File is not writable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not write file: {exc}") return _managed_write_result(policy, target, display_path) @@ -569,8 +527,6 @@ async def stream_upload_to_path( out.write(chunk) os.replace(tmp_path, target) renamed = True - except HTTPException: - raise except PermissionError: raise HTTPException(status_code=403, detail=not_writable) except OSError as exc: @@ -592,12 +548,8 @@ async def upload_managed_file_stream( """Chunked multipart upload: constant memory and no base64 inflation, unlike the JSON data-URL endpoint that trips proxy body-size limits on large archives.""" policy, target, display_path = _managed_write_target(path, request, overwrite) - try: + with _io_errors("File is not writable", "Could not create parent directory"): target.parent.mkdir(parents=True, exist_ok=True) - except PermissionError: - raise HTTPException(status_code=403, detail="File is not writable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not create parent directory: {exc}") await stream_upload_to_path( file, target, too_large="File is too large", @@ -612,12 +564,8 @@ async def create_managed_directory(payload: ManagedDirectoryCreate, request: Req policy, target, display_path = _resolve_managed_path(payload.path, request, for_write=True) if target.exists() and not target.is_dir(): raise HTTPException(status_code=409, detail="A file already exists at that path") - try: + with _io_errors("Directory is not writable", "Could not create directory"): target.mkdir(parents=True, exist_ok=True) - except PermissionError: - raise HTTPException(status_code=403, detail="Directory is not writable") - except OSError as exc: - raise HTTPException(status_code=500, detail=f"Could not create directory: {exc}") return _managed_write_result(policy, target, display_path) diff --git a/hermes_cli/web_routers/mcp.py b/hermes_cli/web_routers/mcp.py index cda360ad37..5211678483 100644 --- a/hermes_cli/web_routers/mcp.py +++ b/hermes_cli/web_routers/mcp.py @@ -17,20 +17,10 @@ from fastapi import APIRouter, HTTPException, Request from fastapi.responses import HTMLResponse from hermes_cli.web_deps import LateState, late -from hermes_cli.web_models import ( - MCPCatalogInstall, - MCPEnabledToggle, - MCPServerCreate, - MCPServersReplace, -) +from hermes_cli.web_models import MCPCatalogInstall, MCPEnabledToggle, MCPServerCreate, MCPServersReplace from hermes_cli.web_routers._common import ( - _profile_cli_args, - _profile_scope, - _spawn_hermes_action, - config_write_scope, - http_failure, - log as _log, - scoped_to_thread, + _profile_cli_args, _profile_scope, _spawn_hermes_action, config_write_scope, http_failure, + log as _log, scoped_to_thread, ) router = APIRouter() @@ -113,8 +103,7 @@ async def add_mcp_server(body: MCPServerCreate, profile: Optional[str] = None): server_config["headers"] = _save_bearer_auth_token(name, bearer_token) if not _save_mcp_server(name, server_config): raise HTTPException( - status_code=400, - detail=f"Server '{name}' rejected: suspicious command/args configuration", + status_code=400, detail=f"Server '{name}' rejected: suspicious command/args configuration", ) try: @@ -249,9 +238,7 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = if any(f.server_name == name and f.hermes_home == flow_home for f in live): raise HTTPException(status_code=409, detail=f"MCP OAuth for '{name}' is already in progress") _mcp_oauth_flows[flow_id] = flow - threading.Thread( - target=_run_dashboard_mcp_oauth, args=(flow, cfg), daemon=True, name=f"mcp-oauth-{name}", - ).start() + threading.Thread(target=_run_dashboard_mcp_oauth, args=(flow, cfg), daemon=True, name=f"mcp-oauth-{name}").start() try: await flow.wait_for_authorization_url(timeout=30) except Exception as exc: @@ -299,10 +286,8 @@ async def mcp_oauth_callback( if flow.server_name == server_name and flow.status == "authorization_required" ] flow = next( - ( - c for c in candidates - if c.expected_state is not None and state is not None and secrets.compare_digest(c.expected_state, state) - ), + (c for c in candidates + if c.expected_state is not None and state is not None and secrets.compare_digest(c.expected_state, state)), None, ) if flow is None: diff --git a/hermes_cli/web_routers/ops.py b/hermes_cli/web_routers/ops.py index 125e793474..668d302c9a 100644 --- a/hermes_cli/web_routers/ops.py +++ b/hermes_cli/web_routers/ops.py @@ -23,17 +23,8 @@ from fastapi.responses import FileResponse from hermes_cli.config import redact_key from hermes_cli.web_deps import late from hermes_cli.web_models import ( - BackupRequest, - CredentialPoolAdd, - HookCreate, - HookDelete, - ImportRequest, - MemoryProviderSelect, - MemoryReset, - PairingApprove, - PairingRevoke, - WebhookCreate, - WebhookEnabledToggle, + BackupRequest, CredentialPoolAdd, HookCreate, HookDelete, ImportRequest, MemoryProviderSelect, + MemoryReset, PairingApprove, PairingRevoke, WebhookCreate, WebhookEnabledToggle, ) from hermes_cli.web_routers._common import _CONFIG_MUTATION_LOCK, http_failure, spawn_profile_action from hermes_cli.web_routers.files import stream_upload_to_path @@ -107,12 +98,10 @@ async def approve_pairing(body: PairingApprove): # as a bogus 429 while the platform is locked out for an unrelated reason. if not by_request_id and store._is_locked_out(platform): raise HTTPException( - status_code=429, - detail=f"Platform '{platform}' is locked out after too many failed approvals.", + status_code=429, detail=f"Platform '{platform}' is locked out after too many failed approvals.", ) raise HTTPException( - status_code=404, - detail=f"Pairing request or code not found or expired for platform '{platform}'.", + status_code=404, detail=f"Pairing request or code not found or expired for platform '{platform}'.", ) @@ -124,10 +113,7 @@ async def revoke_pairing(body: PairingRevoke): raise HTTPException(status_code=400, detail="platform and user_id are required") if store.revoke(platform, body.user_id): return {"ok": True} - raise HTTPException( - status_code=404, - detail=f"User {body.user_id} not found in approved list for {platform}.", - ) + raise HTTPException(status_code=404, detail=f"User {body.user_id} not found in approved list for {platform}.") @router.post("/api/pairing/clear-pending") @@ -174,10 +160,7 @@ async def list_webhooks(): @router.post("/api/webhooks/enable") async def enable_webhooks(): - with http_failure( - "Failed to enable webhook platform from dashboard", 500, - detail="Failed to enable webhook platform.", - ): + with http_failure("Failed to enable webhook platform from dashboard", 500, detail="Failed to enable webhook platform."): _write_platform_enabled("webhook", True) restart_result = _restart_gateway_after(None, what="enabling webhooks", label="Webhook enable") return { @@ -195,19 +178,16 @@ async def create_webhook(body: WebhookCreate): if not wh._is_webhook_enabled(): raise HTTPException( - status_code=400, - detail="Webhook platform is not enabled. Enable it from the Webhooks page first.", + status_code=400, detail="Webhook platform is not enabled. Enable it from the Webhooks page first.", ) name = (body.name or "").strip().lower().replace(" ", "-") if not re.match(r"^[a-z0-9][a-z0-9_-]*$", name): raise HTTPException( - status_code=400, - detail="Invalid name. Use lowercase alphanumeric with hyphens/underscores.", + status_code=400, detail="Invalid name. Use lowercase alphanumeric with hyphens/underscores.", ) if body.deliver_only and body.deliver == "log": raise HTTPException( - status_code=400, - detail="Direct delivery requires a real target (telegram, discord, …), not 'log'.", + status_code=400, detail="Direct delivery requires a real target (telegram, discord, …), not 'log'.", ) secret = body.secret or secrets.token_urlsafe(32) @@ -426,21 +406,12 @@ async def remove_credential_pool_entry(provider: str, index: int): except Exception: # Cleanup is best-effort, but suppression is the actual fix — # without it the entry resurrects on the next load_pool(). - _log.exception( - "credential source cleanup failed for %s/%s; suppressing anyway", - provider, removed.source, - ) + _log.exception("credential source cleanup failed for %s/%s; suppressing anyway", provider, removed.source) try: suppress_credential_source(provider, removed.source) except Exception: _log.exception("suppress_credential_source failed") - return { - "ok": True, - "provider": provider, - "count": len(pool.entries()), - "cleaned": cleaned, - "hints": hints, - } + return {"ok": True, "provider": provider, "count": len(pool.entries()), "cleaned": cleaned, "hints": hints} return await asyncio.to_thread(_run) @@ -463,11 +434,7 @@ async def get_memory_status(): for fname, key in _MEMORY_FILES: path = mem_dir / fname files[key] = path.stat().st_size if path.exists() else 0 - return { - "active": active, - "providers": _discover_memory_provider_statuses(), - "builtin_files": files, - } + return {"active": active, "providers": _discover_memory_provider_statuses(), "builtin_files": files} return await asyncio.to_thread(_run) @@ -542,9 +509,7 @@ async def run_backup(body: BackupRequest): except OSError as exc: raise HTTPException(status_code=500, detail=f"Could not create backup directory: {exc}") output = str(archive) - response = _spawn_action( - ["backup", "-o", output], "backup", log_msg="Failed to spawn backup", prefix="Failed to run backup", - ) + response = _spawn_action(["backup", "-o", output], "backup", log_msg="Failed to spawn backup", prefix="Failed to run backup") if archive is not None: response["archive"] = str(archive) return response @@ -565,10 +530,7 @@ async def download_dashboard_backup(archive: str): if not target.is_file(): raise HTTPException(status_code=404, detail="Backup not found") return FileResponse( - path=str(target), - media_type="application/zip", - filename=target.name, - content_disposition_type="attachment", + path=str(target), media_type="application/zip", filename=target.name, content_disposition_type="attachment", ) @@ -611,10 +573,8 @@ async def run_import_upload( stamp = datetime.now(timezone.utc).strftime("%Y%m%d-%H%M%S") target = staging_dir / f"dashboard-import-{stamp}-{secrets.token_hex(4)}-{_safe_backup_upload_name(file.filename)}" total = await stream_upload_to_path( - file, target, - too_large="Archive is too large", - not_writable="Import staging directory is not writable", - write_failed="Could not write uploaded archive", + file, target, too_large="Archive is too large", + not_writable="Import staging directory is not writable", write_failed="Could not write uploaded archive", ) if not zipfile.is_zipfile(target): target.unlink(missing_ok=True) @@ -686,10 +646,7 @@ async def create_hook(body: HookCreate): with contextlib.suppress(Exception): from hermes_cli.plugins import VALID_HOOKS as valid_hooks if valid_hooks is not None and event not in valid_hooks: - raise HTTPException( - status_code=400, - detail=f"Unknown event '{event}'. Valid: {', '.join(sorted(valid_hooks))}", - ) + raise HTTPException(status_code=400, detail=f"Unknown event '{event}'. Valid: {', '.join(sorted(valid_hooks))}") def _run(): with _CONFIG_MUTATION_LOCK: