From e6663596ce5001bdae2d26e377b7a3976a6dda05 Mon Sep 17 00:00:00 2001
From: Nathan Esquenazi <nesquena@gmail.com>
Date: Fri, 3 Apr 2026 20:52:37 +0000
Subject: [PATCH] fix(review): 4 issues found in agent review of PR #45
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

BUG-1 (medium): _validate_profile_name() used re.match() with a $ anchor.
re.match() with $ is truthy for 'name\n' because match() allows trailing
content after the $ in multiline mode. Changed to re.fullmatch() which
requires the entire string to match — trailing newlines now correctly rejected.

BUG-2 (medium/defense-in-depth): create_profile_api() validated 'name' via
_validate_profile_name() but passed clone_from directly to hermes_cli and
_create_profile_fallback() without validation. Added clone_from validation
inside create_profile_api() (skipping 'default' which is a valid clone source).
routes.py already validates it at the HTTP layer; this adds API-layer defense.

BUG-3 (low): When hermes_cli is not importable (the exact Docker case this PR
targets), list_profiles_api() also returns only the stub default dict and
can't find the newly created profile by name. The fallback return was a
2-key dict {name, path} — incomplete vs the 9-key schema everywhere else.
Expanded to the full profile dict with all fields so API clients get
consistent data regardless of hermes_cli availability.

OBS-4 (low/TOCTOU): _create_profile_fallback() checked profile_dir.exists()
then called mkdir(exist_ok=True). If a concurrent request created the dir
between those two calls, mkdir silently succeeded — defeating the
FileExistsError guard. Changed to mkdir(exist_ok=False) so the OS raises
FileExistsError atomically if the dir appears in the race window.

Tests: 423 passed, 0 failed.
---
 api/profiles.py | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/api/profiles.py b/api/profiles.py
index 579ad78..3f393a0 100644
--- a/api/profiles.py
+++ b/api/profiles.py
@@ -259,7 +259,8 @@ def _validate_profile_name(name: str):
     """Validate profile name format (matches hermes_cli.profiles upstream)."""
     if name == 'default':
         raise ValueError("Cannot create a profile named 'default' -- it is the built-in profile.")
-    if not _PROFILE_ID_RE.match(name):
+    # Use fullmatch (not match) so a trailing newline can't sneak past the $ anchor
+    if not _PROFILE_ID_RE.fullmatch(name):
         raise ValueError(
             f"Invalid profile name {name!r}. "
             "Must match [a-z0-9][a-z0-9_-]{0,63}"
@@ -273,8 +274,8 @@ def _create_profile_fallback(name: str, clone_from: str = None,
     if profile_dir.exists():
         raise FileExistsError(f"Profile '{name}' already exists.")
 
-    # Bootstrap directory structure
-    profile_dir.mkdir(parents=True, exist_ok=True)
+    # Bootstrap directory structure (exist_ok=False so a concurrent create raises)
+    profile_dir.mkdir(parents=True, exist_ok=False)
     for subdir in _PROFILE_DIRS:
         (profile_dir / subdir).mkdir(parents=True, exist_ok=True)
 
@@ -297,6 +298,10 @@ def create_profile_api(name: str, clone_from: str = None,
                        clone_config: bool = False) -> dict:
     """Create a new profile. Returns the new profile info dict."""
     _validate_profile_name(name)
+    # Defense-in-depth: validate clone_from here too, even though routes.py
+    # also validates it. Any caller that bypasses the HTTP layer gets protection.
+    if clone_from is not None and clone_from != 'default':
+        _validate_profile_name(clone_from)
 
     try:
         from hermes_cli.profiles import create_profile
@@ -310,11 +315,25 @@ def create_profile_api(name: str, clone_from: str = None,
     except ImportError:
         _create_profile_fallback(name, clone_from, clone_config)
 
-    # Find and return the newly created profile info
+    # Find and return the newly created profile info.
+    # When hermes_cli is not importable, list_profiles_api() also falls back
+    # to the stub default-only list and won't find the new profile by name.
+    # In that case, return a complete profile dict directly.
+    profile_path = _DEFAULT_HERMES_HOME / 'profiles' / name
     for p in list_profiles_api():
         if p['name'] == name:
             return p
-    return {'name': name, 'path': str(_DEFAULT_HERMES_HOME / 'profiles' / name)}
+    return {
+        'name': name,
+        'path': str(profile_path),
+        'is_default': False,
+        'is_active': _active_profile == name,
+        'gateway_running': False,
+        'model': None,
+        'provider': None,
+        'has_env': (profile_path / '.env').exists(),
+        'skill_count': 0,
+    }
 
 
 def delete_profile_api(name: str) -> dict: