fix: address CodeRabbit review issues for PR #141

- Cache ui_config in client.py to avoid 3 redundant file reads
- Fix env:None in MCP config by omitting key when empty
- Add integration tests for generate_design_tokens_from_spec
- Fix Phase 3b → 3d reference in create-spec.md
- Remove duplicate ROOT_DIR in spec_chat_session.py
- Add ARIA listbox role to UILibrarySelector and VisualStyleSelector
- Remove redundant isCompatible check in UILibrarySelector
- Add OSError handling in design_tokens.py
- Move constants to top of app_spec_parser.py
- Improve XML comment filtering with regex pattern

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: cabana8471-arch

.claude/commands/create-spec.md

@@ -293,7 +293,7 @@ These are just reference points - your actual count should come from the require
 
 **MANDATORY: Infrastructure Features**
 
-If the app requires a database (Phase 3b answer was "Yes" or "Not sure"), you MUST include 5 Infrastructure features (indices 0-4):
+If the app requires a database (Phase 3d answer was "Yes" or "Not sure"), you MUST include 5 Infrastructure features (indices 0-4):
 1. Database connection established
 2. Database schema applied correctly
 3. Data persists across server restart

app_spec_parser.py

@@ -4,12 +4,41 @@
 
 Shared utilities for parsing app_spec.txt sections.
 Used by client.py, prompts.py, and design_tokens.py to avoid code duplication.
+
+This module provides functions to:
+- Extract XML sections from app_spec.txt
+- Parse UI component library configuration
+- Parse visual style configuration
+- Combine configurations for the coding agent
 """
 
 import re
 from pathlib import Path
 from typing import TypedDict
 
+# =============================================================================
+# Constants
+# =============================================================================
+
+# Valid UI library options
+VALID_UI_LIBRARIES = {"shadcn-ui", "ark-ui", "radix-ui", "none"}
+
+# Libraries that have MCP server support
+MCP_SUPPORTED_LIBRARIES = {"shadcn-ui", "ark-ui"}
+
+# Valid visual style options
+VALID_VISUAL_STYLES = {"default", "neobrutalism", "glassmorphism", "retro", "custom"}
+
+# Valid frameworks
+VALID_FRAMEWORKS = {"react", "vue", "solid", "svelte"}
+
+# Regex pattern to match XML comments (<!-- ... -->)
+XML_COMMENT_PATTERN = re.compile(r"<!--.*?-->", re.DOTALL)
+
+# =============================================================================
+# Type Definitions
+# =============================================================================
+
 
 class UIComponentsConfig(TypedDict, total=False):
     """Configuration for UI component library."""
@@ -33,6 +62,11 @@ class UIConfig(TypedDict, total=False):
     design_tokens_path: str
 
 
+# =============================================================================
+# Parsing Functions
+# =============================================================================
+
+
 def parse_section(content: str, section_name: str) -> str | None:
     """
     Parse an XML section from app_spec.txt content.
@@ -53,19 +87,29 @@ def parse_xml_value(content: str, tag_name: str) -> str | None:
     """
     Parse a single XML value from content.
 
+    Extracts the text content from an XML tag, filtering out any XML comments.
+    Returns None if the tag is not found or contains only whitespace/comments.
+
     Args:
         content: XML content to search
         tag_name: The tag name to extract value from
 
     Returns:
-        The value inside the tags, or None if not found.
+        The value inside the tags, or None if not found or empty.
+
+    Example:
+        >>> parse_xml_value("<library>shadcn-ui</library>", "library")
+        'shadcn-ui'
+        >>> parse_xml_value("<library><!-- comment --></library>", "library")
+        None
     """
     pattern = rf"<{tag_name}[^>]*>(.*?)</{tag_name}>"
     match = re.search(pattern, content, re.DOTALL)
     if match:
-        value = match.group(1).strip()
-        # Filter out XML comments
-        if value and not value.startswith("<!--"):
+        value = match.group(1)
+        # Remove XML comments using regex pattern
+        value = XML_COMMENT_PATTERN.sub("", value).strip()
+        if value:
             return value
     return None
 
@@ -169,16 +213,3 @@ def get_ui_config_from_spec(project_dir: Path) -> UIConfig | None:
         return parse_ui_config(content)
     except (OSError, UnicodeDecodeError):
         return None
-
-
-# Valid UI library options
-VALID_UI_LIBRARIES = {"shadcn-ui", "ark-ui", "radix-ui", "none"}
-
-# Libraries that have MCP server support
-MCP_SUPPORTED_LIBRARIES = {"shadcn-ui", "ark-ui"}
-
-# Valid visual style options
-VALID_VISUAL_STYLES = {"default", "neobrutalism", "glassmorphism", "retro", "custom"}
-
-# Valid frameworks
-VALID_FRAMEWORKS = {"react", "vue", "solid", "svelte"}

client.py

@@ -355,6 +355,12 @@ def create_client(
     Note: Authentication is handled by start.bat/start.sh before this runs.
     The Claude SDK auto-detects credentials from the Claude CLI configuration
     """
+    # Cache UI config once to avoid repeated file reads
+    # This is used for both allowed_tools and MCP server configuration
+    ui_mcp_disabled = os.getenv("DISABLE_UI_MCP", "").lower() == "true"
+    ui_config = None if ui_mcp_disabled else get_ui_config_from_spec(project_dir)
+    has_ui_mcp = ui_config is not None and ui_config.get("has_mcp", False)
+
     # Select the feature MCP tools appropriate for this agent type
     feature_tools_map = {
         "coding": CODING_AGENT_TOOLS,
@@ -381,10 +387,8 @@ def create_client(
 
     # Add UI MCP tools if the project uses a library with MCP support
     # UI MCP is available in both standard and YOLO mode
-    if os.getenv("DISABLE_UI_MCP", "").lower() != "true":
-        ui_config = get_ui_config_from_spec(project_dir)
-        if ui_config and ui_config.get("has_mcp"):
-            allowed_tools.extend(UI_MCP_TOOLS)
+    if has_ui_mcp:
+        allowed_tools.extend(UI_MCP_TOOLS)
 
     # Build permissions list.
     # We permit ALL feature MCP tools at the security layer (so the MCP server
@@ -421,10 +425,8 @@ def create_client(
         permissions_list.extend(PLAYWRIGHT_TOOLS)
 
     # Add UI MCP tools to permissions if available
-    if os.getenv("DISABLE_UI_MCP", "").lower() != "true":
-        ui_config = get_ui_config_from_spec(project_dir)
-        if ui_config and ui_config.get("has_mcp"):
-            permissions_list.extend(UI_MCP_TOOLS)
+    if has_ui_mcp:
+        permissions_list.extend(UI_MCP_TOOLS)
 
     # Create comprehensive security settings
     # Note: Using relative paths ("./**") restricts access to project directory
@@ -509,52 +511,50 @@ def create_client(
 
     # UI Components MCP server (available in both standard and YOLO mode)
     # Only added for libraries with MCP support (shadcn-ui, ark-ui)
-    if os.getenv("DISABLE_UI_MCP", "").lower() != "true":
-        ui_config = get_ui_config_from_spec(project_dir)
-        if ui_config and ui_config.get("has_mcp"):
-            library = ui_config.get("library", "")
-            framework = ui_config.get("framework", "react")
+    if has_ui_mcp and ui_config:
+        library = ui_config.get("library", "")
+        framework = ui_config.get("framework", "react")
 
-            try:
-                npx_cmd = get_npx_command()
-
-                if library == "shadcn-ui":
-                    # shadcn/ui MCP server for React components
-                    # Uses GitHub API - benefits from GITHUB_PERSONAL_ACCESS_TOKEN for rate limits
-                    ui_mcp_args = [
-                        "-y",
-                        "--prefer-offline",
-                        f"@jpisnice/shadcn-ui-mcp-server@{SHADCN_MCP_VERSION}",
-                        "--framework", framework,
-                    ]
-                    ui_mcp_env = {}
-                    github_token = os.getenv("GITHUB_PERSONAL_ACCESS_TOKEN")
-                    if github_token:
-                        ui_mcp_env["GITHUB_TOKEN"] = github_token
-
-                    mcp_servers["ui_components"] = {
-                        "command": npx_cmd,
-                        "args": ui_mcp_args,
-                        "env": ui_mcp_env if ui_mcp_env else None,
-                    }
-                    print(f"   - UI MCP: shadcn/ui ({framework})")
-
-                elif library == "ark-ui":
-                    # Ark UI MCP server for multi-framework headless components
-                    ui_mcp_args = [
-                        "-y",
-                        "--prefer-offline",
-                        f"@ark-ui/mcp@{ARK_MCP_VERSION}",
-                    ]
-                    mcp_servers["ui_components"] = {
-                        "command": npx_cmd,
-                        "args": ui_mcp_args,
-                    }
-                    print(f"   - UI MCP: Ark UI ({framework})")
-
-            except RuntimeError as e:
-                # npx not found - graceful degradation
-                print(f"   - Warning: UI MCP disabled - {e}")
+        try:
+            npx_cmd = get_npx_command()
+
+            if library == "shadcn-ui":
+                # shadcn/ui MCP server for React components
+                # Uses GitHub API - benefits from GITHUB_PERSONAL_ACCESS_TOKEN for rate limits
+                ui_mcp_args = [
+                    "-y",
+                    "--prefer-offline",
+                    f"@jpisnice/shadcn-ui-mcp-server@{SHADCN_MCP_VERSION}",
+                    "--framework", framework,
+                ]
+                ui_mcp_config: dict = {
+                    "command": npx_cmd,
+                    "args": ui_mcp_args,
+                }
+                # Only add env if there are environment variables to pass
+                github_token = os.getenv("GITHUB_PERSONAL_ACCESS_TOKEN")
+                if github_token:
+                    ui_mcp_config["env"] = {"GITHUB_TOKEN": github_token}
+
+                mcp_servers["ui_components"] = ui_mcp_config
+                print(f"   - UI MCP: shadcn/ui ({framework})")
+
+            elif library == "ark-ui":
+                # Ark UI MCP server for multi-framework headless components
+                ui_mcp_args = [
+                    "-y",
+                    "--prefer-offline",
+                    f"@ark-ui/mcp@{ARK_MCP_VERSION}",
+                ]
+                mcp_servers["ui_components"] = {
+                    "command": npx_cmd,
+                    "args": ui_mcp_args,
+                }
+                print(f"   - UI MCP: Ark UI ({framework})")
+
+        except RuntimeError as e:
+            # npx not found - graceful degradation
+            print(f"   - Warning: UI MCP disabled - {e}")
 
     # Build environment overrides for API endpoint configuration
     # Uses get_effective_sdk_env() which reads provider settings from the database,

design_tokens.py

@@ -158,8 +158,11 @@ def generate_design_tokens(project_dir: Path, style: str) -> Path | None:
         style: The visual style to use
 
     Returns:
-        Path to the generated tokens file, or None if style is default.
+        Path to the generated tokens file, or None if style is default/custom
+        or if file write fails.
     """
+    # "default" uses library defaults, no tokens needed
+    # "custom" means user will define their own tokens manually
     if style == "default" or style == "custom":
         return None
 
@@ -173,7 +176,11 @@ def generate_design_tokens(project_dir: Path, style: str) -> Path | None:
 
     # Write design tokens
     tokens_path = autocoder_dir / "design-tokens.json"
-    tokens_path.write_text(json.dumps(preset, indent=2), encoding="utf-8")
+    try:
+        tokens_path.write_text(json.dumps(preset, indent=2), encoding="utf-8")
+    except OSError:
+        # File write failed (permissions, disk full, etc.)
+        return None
 
     return tokens_path
 

test_ui_config.py

@@ -23,6 +23,7 @@
 from design_tokens import (
     STYLE_PRESETS,
     generate_design_tokens,
+    generate_design_tokens_from_spec,
     get_style_preset,
     validate_visual_style,
 )
@@ -343,6 +344,67 @@ def test_generate_custom_tokens(self):
             assert result is None
 
 
+# =============================================================================
+# Test: generate_design_tokens_from_spec
+# =============================================================================
+
+class TestGenerateDesignTokensFromSpec:
+    """Tests for generate_design_tokens_from_spec function."""
+
+    def test_generate_tokens_from_neobrutalism_spec(self):
+        """Integration test: generate tokens from a spec file with neobrutalism style."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            project_dir = Path(tmpdir)
+            prompts_dir = project_dir / "prompts"
+            prompts_dir.mkdir()
+
+            spec_content = """
+            <project_specification>
+                <visual_style>
+                    <style>neobrutalism</style>
+                    <design_tokens_path>.autocoder/design-tokens.json</design_tokens_path>
+                </visual_style>
+            </project_specification>
+            """
+            (prompts_dir / "app_spec.txt").write_text(spec_content)
+
+            result = generate_design_tokens_from_spec(project_dir)
+
+            assert result is not None
+            assert result.exists()
+            assert result.name == "design-tokens.json"
+
+            tokens = json.loads(result.read_text())
+            assert tokens["borders"]["width"] == "4px"
+            assert tokens["borders"]["radius"] == "0"
+
+    def test_generate_tokens_from_default_spec(self):
+        """No tokens generated for default style."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            project_dir = Path(tmpdir)
+            prompts_dir = project_dir / "prompts"
+            prompts_dir.mkdir()
+
+            spec_content = """
+            <project_specification>
+                <visual_style>
+                    <style>default</style>
+                </visual_style>
+            </project_specification>
+            """
+            (prompts_dir / "app_spec.txt").write_text(spec_content)
+
+            result = generate_design_tokens_from_spec(project_dir)
+            assert result is None
+
+    def test_generate_tokens_missing_spec(self):
+        """Returns None when spec file doesn't exist."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            project_dir = Path(tmpdir)
+            result = generate_design_tokens_from_spec(project_dir)
+            assert result is None
+
+
 # =============================================================================
 # Test: validate_visual_style
 # =============================================================================

ui/src/components/UILibrarySelector.tsx

@@ -84,11 +84,16 @@ export function UILibrarySelector({
   }
 
   const isCompatible = (lib: UILibrary) => {
-    return lib.frameworks.includes(framework) || lib.id === 'none'
+    // 'none' is always compatible as it's a catch-all for any framework
+    return lib.frameworks.includes(framework)
   }
 
   return (
-    <div className={cn('grid grid-cols-1 gap-3 sm:grid-cols-2', className)}>
+    <div
+      className={cn('grid grid-cols-1 gap-3 sm:grid-cols-2', className)}
+      role="listbox"
+      aria-label="Select UI component library"
+    >
       {UI_LIBRARIES.map((lib) => {
         const selected = value === lib.id
         const compatible = isCompatible(lib)

ui/src/components/VisualStyleSelector.tsx

@@ -163,7 +163,11 @@ export function VisualStyleSelector({
   }
 
   return (
-    <div className={cn('grid grid-cols-1 gap-3 sm:grid-cols-2 lg:grid-cols-3', className)}>
+    <div
+      className={cn('grid grid-cols-1 gap-3 sm:grid-cols-2 lg:grid-cols-3', className)}
+      role="listbox"
+      aria-label="Select visual style"
+    >
       {VISUAL_STYLES.map((style) => {
         const selected = value === style.id
         // hoveredId is tracked for potential future hover preview effects