fix: cli improvements (#230)

shadowfax92 · web-flow · commit 14ce8dda2193 · 2025-12-11T11:00:15.000-08:00
* fix: feature support add-update

* feat: feature name validation
diff --git a/packages/browseros/build/cli/dev.py b/packages/browseros/build/cli/dev.py
@@ -403,29 +403,40 @@ def feature_show(
         raise typer.Exit(1)
 
 
-@feature_app.command(name="add")
-def feature_add(
-    feature_name: str = Argument(..., help="Feature name to add"),
-    commit: str = Argument(..., help="Git commit reference"),
-    description: Optional[str] = Option(
-        None, "--description", "-d", help="Feature description"
+@feature_app.command(name="add-update")
+def feature_add_update(
+    name: str = Option(
+        ..., "--name", "-n", help="Feature name (lowercase kebab-case, e.g., 'llm-chat')"
+    ),
+    commit: str = Option(..., "--commit", "-c", help="Git commit reference"),
+    description: str = Option(
+        ...,
+        "--description",
+        "-d",
+        help="Feature description with prefix (feat:, fix:, build:, chore:, series:)",
     ),
 ):
-    """Add a new feature from a commit"""
+    """Add or update a feature with files from a commit.
+
+    If the feature exists, merges new files into it.
+    If the feature is new, creates it.
+
+    Examples:
+        browseros dev feature add-update --name llm-chat --commit HEAD -d "feat: LLM chat panel"
+        browseros dev feature add-update -n api -c HEAD~3 -d "feat: browseros API updates"
+    """
     ctx = create_build_context(state.chromium_src)
     if not ctx:
         raise typer.Exit(1)
 
-    from ..modules.feature import AddFeatureModule
+    from ..modules.feature import AddUpdateFeatureModule
 
-    module = AddFeatureModule()
+    module = AddUpdateFeatureModule()
     try:
         module.validate(ctx)
-        module.execute(
-            ctx, feature_name=feature_name, commit=commit, description=description
-        )
+        module.execute(ctx, name=name, commit=commit, description=description)
     except Exception as e:
-        log_error(f"Failed to add feature: {e}")
+        log_error(f"Failed to add/update feature: {e}")
         raise typer.Exit(1)
 
 
diff --git a/packages/browseros/build/features.yaml b/packages/browseros/build/features.yaml
@@ -366,8 +366,8 @@ features:
       - chrome/browser/profiles/profile_shortcut_manager_win.cc
       - chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
       - chrome/browser/ui/views/frame/browser_window_property_manager_win.cc
-  "feat: chromium urls for browseros":
-    description: "feat: feat: chromium urls for browseros"
+  chromium-urls:
+    description: "feat: chromium urls for browseros"
     files:
       - chrome/browser/chrome_content_browser_client.cc
       - chrome/browser/extensions/browseros_extension_constants.h
diff --git a/packages/browseros/build/modules/feature/__init__.py b/packages/browseros/build/modules/feature/__init__.py
@@ -2,17 +2,26 @@
 Feature module - Manage feature-to-file mappings.
 
 Provides commands for managing features:
-- add_feature: Add files from a commit to a feature
+- add_or_update_feature: Add or update a feature with files from a commit
 - list_features: List all defined features
 - show_feature: Show details of a specific feature
 - prompt_feature_selection: Interactive feature selection for extract commands
 - add_files_to_feature: Add files to a feature (with duplicate handling)
 - classify_files: Classify unclassified patch files into features
+- validate_description: Validate description has required prefix
+- validate_feature_name: Validate feature name format
 """
 
+from .validation import (
+    validate_description,
+    validate_feature_name,
+    VALID_PREFIXES,
+)
 from .feature import (
     add_feature,
+    add_or_update_feature,
     AddFeatureModule,
+    AddUpdateFeatureModule,
     list_features,
     ListFeaturesModule,
     show_feature,
@@ -28,7 +37,12 @@
 
 __all__ = [
     "add_feature",
+    "add_or_update_feature",
+    "validate_description",
+    "validate_feature_name",
+    "VALID_PREFIXES",
     "AddFeatureModule",
+    "AddUpdateFeatureModule",
     "list_features",
     "ListFeaturesModule",
     "show_feature",
diff --git a/packages/browseros/build/modules/feature/feature.py b/packages/browseros/build/modules/feature/feature.py
@@ -5,49 +5,122 @@
 """
 
 import yaml
-from typing import Dict, Optional
+from typing import Dict, List, Optional, Tuple
 from ...common.context import Context
 from ...common.module import CommandModule, ValidationError
 from ..extract.utils import get_commit_changed_files
 from ...common.utils import log_info, log_error, log_success, log_warning
+from .validation import validate_description, validate_feature_name, VALID_PREFIXES
 
 
-def add_feature(ctx: Context, feature_name: str, commit: str, description: Optional[str] = None) -> bool:
-    """Add files from a commit to a feature
+def add_or_update_feature(
+    ctx: Context,
+    feature_name: str,
+    commit: str,
+    description: str,
+) -> Tuple[bool, str]:
+    """Add or update a feature with files from a commit.
+
+    If feature exists, merges files (appends new ones).
+    If feature is new, creates it.
+
+    Args:
+        ctx: Build context
+        feature_name: Feature key name (e.g., 'llm-chat')
+        commit: Git commit reference
+        description: Feature description with prefix (e.g., 'feat: LLM chat')
 
-    Examples:
-      dev feature add my-feature HEAD
-      dev feature add llm-chat HEAD~3 --description "LLM chat integration"
+    Returns:
+        Tuple of (success, error_message)
     """
+    # Validate inputs
+    valid, error = validate_feature_name(feature_name)
+    if not valid:
+        return False, error
+
+    valid, error = validate_description(description)
+    if not valid:
+        return False, error
+
     features_file = ctx.get_features_yaml_path()
 
     # Get changed files from commit
     changed_files = get_commit_changed_files(commit, ctx.chromium_src)
     if not changed_files:
-        log_error(f"No changed files found in commit {commit}")
-        return False
+        return False, f"No changed files found in commit {commit}"
 
     # Load existing features
-    features: Dict = {"features": {}}
+    features: Dict = {"version": "1.0", "features": {}}
     if features_file.exists():
         with open(features_file, "r") as f:
             content = yaml.safe_load(f)
-            if content and "features" in content:
+            if content:
                 features = content
+                if "features" not in features:
+                    features["features"] = {}
+
+    existing_feature = features["features"].get(feature_name)
+
+    if existing_feature:
+        # Update existing feature - merge files
+        existing_files = set(existing_feature.get("files", []))
+        new_files = set(changed_files)
+
+        added_files = new_files - existing_files
+        already_present = new_files & existing_files
+        merged_files = existing_files | new_files
+
+        log_info(f"Updating existing feature '{feature_name}'")
+        log_info(f"  Current files: {len(existing_files)}")
+        log_info(f"  Files from commit: {len(new_files)}")
+
+        if added_files:
+            log_success(f"  Adding {len(added_files)} new file(s):")
+            for f in sorted(added_files)[:10]:
+                log_info(f"    + {f}")
+            if len(added_files) > 10:
+                log_info(f"    ... and {len(added_files) - 10} more")
 
-    # Add or update feature
-    features["features"][feature_name] = {
-        "description": description or f"Feature: {feature_name}",
-        "files": sorted(changed_files),
-        "commit": commit,
-    }
+        if already_present:
+            log_warning(f"  Skipping {len(already_present)} file(s) already in feature")
+
+        features["features"][feature_name]["files"] = sorted(merged_files)
+        # Update description if provided (allows updating description)
+        features["features"][feature_name]["description"] = description
+
+    else:
+        # Create new feature
+        log_info(f"Creating new feature '{feature_name}'")
+        log_info(f"  Files from commit: {len(changed_files)}")
+
+        features["features"][feature_name] = {
+            "description": description,
+            "files": sorted(changed_files),
+        }
 
     # Save to file
     with open(features_file, "w") as f:
         yaml.safe_dump(features, f, sort_keys=False, default_flow_style=False)
 
-    log_success(f"✓ Added feature '{feature_name}' with {len(changed_files)} files")
-    return True
+    total_files = len(features["features"][feature_name]["files"])
+    if existing_feature:
+        log_success(f"✓ Updated feature '{feature_name}' - now has {total_files} files")
+    else:
+        log_success(f"✓ Created feature '{feature_name}' with {total_files} files")
+
+    return True, ""
+
+
+# Keep old function name for backwards compatibility but mark deprecated
+def add_feature(ctx: Context, feature_name: str, commit: str, description: Optional[str] = None) -> bool:
+    """Deprecated: Use add_or_update_feature instead."""
+    if description is None:
+        log_error("Description is required and must have a valid prefix")
+        return False
+    success, error = add_or_update_feature(ctx, feature_name, commit, description)
+    if not success:
+        log_error(error)
+    return success
 
 
 def list_features(ctx: Context):
@@ -134,11 +207,11 @@ def execute(self, ctx: Context, feature_name: str, **kwargs) -> None:
         show_feature(ctx, feature_name)
 
 
-class AddFeatureModule(CommandModule):
-    """Add files from a commit to a feature"""
+class AddUpdateFeatureModule(CommandModule):
+    """Add or update a feature with files from a commit"""
     produces = []
     requires = []
-    description = "Add files from a commit to a feature"
+    description = "Add or update a feature with files from a commit"
 
     def validate(self, ctx: Context) -> None:
         """Validate git is available"""
@@ -148,10 +221,21 @@ def validate(self, ctx: Context) -> None:
         if not ctx.chromium_src.exists():
             raise ValidationError(f"Chromium source not found: {ctx.chromium_src}")
 
-    def execute(self, ctx: Context, feature_name: str, commit: str, description: Optional[str] = None, **kwargs) -> None:
-        success = add_feature(ctx, feature_name, commit, description)
+    def execute(
+        self,
+        ctx: Context,
+        name: str,
+        commit: str,
+        description: str,
+        **kwargs,
+    ) -> None:
+        success, error = add_or_update_feature(ctx, name, commit, description)
         if not success:
-            raise RuntimeError(f"Failed to add feature '{feature_name}'")
+            raise RuntimeError(error)
+
+
+# Backwards compatibility alias
+AddFeatureModule = AddUpdateFeatureModule
 
 
 class ClassifyFeaturesModule(CommandModule):
diff --git a/packages/browseros/build/modules/feature/select.py b/packages/browseros/build/modules/feature/select.py
@@ -11,6 +11,7 @@
 
 from ...common.context import Context
 from ...common.utils import log_info, log_success, log_warning, log_error
+from .validation import validate_feature_name, validate_description, VALID_PREFIXES
 
 
 def load_features_yaml(features_file: Path) -> Dict:
@@ -120,29 +121,57 @@ def prompt_new_feature(default_description: Optional[str] = None) -> Optional[Tu
     log_info("")
     log_info("Creating new feature:")
     log_info("-" * 40)
+    log_info(f"  Valid prefixes: {', '.join(VALID_PREFIXES)}")
+    log_info("")
 
     try:
-        # Get feature name
-        feature_name = input("Feature name: ").strip()
-        if not feature_name:
-            log_warning("Cancelled - no feature name provided")
-            return None
-
-        # Sanitize feature name (lowercase, hyphens instead of spaces)
-        feature_name = feature_name.lower().replace(" ", "-")
-
-        # Get description
-        if default_description:
-            desc_prompt = f"Description [{default_description}]: "
-        else:
-            desc_prompt = "Description: "
+        # Get and validate feature name
+        while True:
+            feature_name = input("Feature name (kebab-case): ").strip()
+            if not feature_name:
+                log_warning("Cancelled - no feature name provided")
+                return None
 
-        description = input(desc_prompt).strip()
-        if not description and default_description:
-            description = default_description
+            # Sanitize feature name (lowercase, hyphens instead of spaces)
+            feature_name = feature_name.lower().replace(" ", "-")
+
+            # Validate
+            valid, error = validate_feature_name(feature_name)
+            if valid:
+                break
+            log_warning(f"Invalid name: {error}")
+
+        # Get and validate description
+        while True:
+            if default_description:
+                # Check if default already has valid prefix
+                valid, _ = validate_description(default_description)
+                if valid:
+                    desc_prompt = f"Description [{default_description}]: "
+                else:
+                    desc_prompt = f"Description (e.g., feat: {default_description}): "
+            else:
+                desc_prompt = "Description (e.g., feat: Add feature): "
+
+            description = input(desc_prompt).strip()
+            if not description and default_description:
+                # Check if default is valid
+                valid, _ = validate_description(default_description)
+                if valid:
+                    description = default_description
+                else:
+                    log_warning(f"Default description needs prefix. Valid: {', '.join(VALID_PREFIXES)}")
+                    continue
+
+            if not description:
+                log_warning(f"Description required. Must start with: {', '.join(VALID_PREFIXES)}")
+                continue
 
-        if not description:
-            description = f"Feature: {feature_name}"
+            # Validate
+            valid, error = validate_description(description)
+            if valid:
+                break
+            log_warning(f"Invalid description: {error}")
 
         return (feature_name, description)
 
