fix: address review feedback for PR #922

packages/browseros/build/cli/dev.py

@@ -166,9 +166,13 @@ def extract_commit(
         True, "--interactive/--no-interactive", "-i/-n", help="Interactive mode"
     ),
     force: bool = Option(False, "--force", "-f", help="Overwrite existing patches"),
-    include_binary: bool = Option(False, "--include-binary", help="Include binary files"),
+    include_binary: bool = Option(
+        False, "--include-binary", help="Include binary files"
+    ),
     base: Optional[str] = Option(
-        None, "--base", help="Extract full diff from base commit for files in COMMIT"
+        None,
+        "--base",
+        help="Base commit to diff from for BASE_COMMIT-relative extraction (defaults to BASE_COMMIT)",
     ),
     feature: bool = Option(
         False, "--feature", help="Add extracted files to a feature in features.yaml"
@@ -202,9 +206,18 @@ def extract_commit(
 
 @extract_app.command(name="patch")
 def extract_patch_cmd(
-    chromium_path: str = Argument(..., help="Chromium file path (e.g., chrome/common/foo.h)"),
-    base: str = Option(..., "--base", "-b", help="Base commit to diff against"),
-    force: bool = Option(False, "--force", "-f", help="Overwrite existing patch without prompting"),
+    chromium_path: str = Argument(
+        ..., help="Chromium file path (e.g., chrome/common/foo.h)"
+    ),
+    base: Optional[str] = Option(
+        None,
+        "--base",
+        "-b",
+        help="Base commit to diff against (defaults to BASE_COMMIT)",
+    ),
+    force: bool = Option(
+        False, "--force", "-f", help="Overwrite existing patch without prompting"
+    ),
     feature: bool = Option(
         False, "--feature", help="Add extracted file to a feature in features.yaml"
     ),
@@ -224,9 +237,17 @@ def extract_patch_cmd(
 
     # Handle --feature flag
     if feature:
+        from ..modules.extract.common import resolve_base_commit
+        from ..modules.extract.utils import GitError
         from ..modules.feature import prompt_feature_selection, add_files_to_feature
 
-        result = prompt_feature_selection(ctx, base[:12], None)
+        try:
+            resolved_base = resolve_base_commit(ctx, base)
+        except GitError as e:
+            log_error(str(e))
+            raise typer.Exit(1)
+
+        result = prompt_feature_selection(ctx, resolved_base[:12], None)
         if result is None:
             log_warning("Skipped adding file to feature")
         else:
@@ -243,12 +264,16 @@ def extract_range(
         True, "--interactive/--no-interactive", "-i/-n", help="Interactive mode"
     ),
     force: bool = Option(False, "--force", "-f", help="Overwrite existing patches"),
-    include_binary: bool = Option(False, "--include-binary", help="Include binary files"),
-    squash: bool = Option(False, "--squash", help="Squash all commits into single patches"),
+    include_binary: bool = Option(
+        False, "--include-binary", help="Include binary files"
+    ),
+    squash: bool = Option(
+        False, "--squash", help="Squash all commits into single patches"
+    ),
     base: Optional[str] = Option(
         None,
         "--base",
-        help="Use different base for diff (full diff from base for files in range)",
+        help="Base commit to diff from (defaults to BASE_COMMIT)",
     ),
     feature: bool = Option(
         False, "--feature", help="Add extracted files to a feature in features.yaml"

packages/browseros/build/modules/extract/common.py

@@ -13,6 +13,7 @@ from ...common.utils import log_info, log_error, log_warning
 from .utils import (
     FilePatch,
     FileOperation,
+    GitError,
     run_git_command,
     parse_diff_output,
     write_patch_file,
@@ -23,6 +24,22 @@ from .utils import (
 )
 
 
+def resolve_base_commit(ctx: Context, base: Optional[str]) -> str:
+    """Return an explicit base or the package BASE_COMMIT used for Chromium patches."""
+    if base:
+        return base
+
+    base_path = ctx.root_dir / "BASE_COMMIT"
+    try:
+        resolved = base_path.read_text(encoding="utf-8").strip()
+    except FileNotFoundError as exc:
+        raise GitError(f"BASE_COMMIT not found: {base_path}") from exc
+
+    if not resolved:
+        raise GitError(f"BASE_COMMIT is empty: {base_path}")
+    return resolved
+
+
 def check_overwrite(ctx: Context, file_patches: Dict, verbose: bool) -> bool:
     """Check for existing patches and prompt for overwrite"""
     existing_patches = []
@@ -137,45 +154,6 @@ def write_patches(
     return success_count, extracted_files
 
 
-def extract_normal(
-    ctx: Context,
-    commit_hash: str,
-    verbose: bool,
-    force: bool,
-    include_binary: bool,
-) -> Tuple[int, List[str]]:
-    """Extract patches normally (diff against parent).
-
-    Returns:
-        Tuple of (count, list of extracted file paths)
-    """
-    from .utils import GitError
-
-    # Get diff against parent
-    diff_cmd = ["git", "diff", f"{commit_hash}^..{commit_hash}"]
-    if include_binary:
-        diff_cmd.append("--binary")
-
-    result = run_git_command(diff_cmd, cwd=ctx.chromium_src)
-
-    if result.returncode != 0:
-        raise GitError(f"Failed to get diff for commit {commit_hash}: {result.stderr}")
-
-    # Parse diff into file patches
-    file_patches = parse_diff_output(result.stdout)
-
-    if not file_patches:
-        log_warning("No changes found in commit")
-        return 0, []
-
-    # Check for existing patches
-    if not force and not check_overwrite(ctx, file_patches, verbose):
-        return 0, []
-
-    # Write patches
-    return write_patches(ctx, file_patches, verbose, include_binary)
-
-
 def extract_with_base(
     ctx: Context,
     commit_hash: str,

packages/browseros/build/modules/extract/extract_base_default_test.py

@@ -0,0 +1,153 @@
+#!/usr/bin/env python3
+"""Tests for extract command default base commit handling."""
+
+import tempfile
+import unittest
+from contextlib import nullcontext
+from pathlib import Path
+from types import SimpleNamespace
+from unittest.mock import patch
+
+from .common import resolve_base_commit
+from .extract_commit import extract_single_commit
+from .extract_patch import extract_single_file_patch
+from .extract_range import extract_commits_individually
+from .utils import FileOperation, FilePatch
+
+
+def make_context(root_dir: Path) -> SimpleNamespace:
+    return SimpleNamespace(
+        root_dir=root_dir,
+        chromium_src=Path("/tmp/chromium"),
+        get_patch_path_for_file=lambda rel: root_dir / "chromium_patches" / rel,
+    )
+
+
+class ExtractBaseDefaultTest(unittest.TestCase):
+    def test_resolve_base_commit_reads_base_commit_when_base_missing(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            root = Path(tmp)
+            (root / "BASE_COMMIT").write_text("base123\n", encoding="utf-8")
+
+            self.assertEqual(resolve_base_commit(make_context(root), None), "base123")
+
+    def test_resolve_base_commit_preserves_explicit_base(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            root = Path(tmp)
+            (root / "BASE_COMMIT").write_text("base123\n", encoding="utf-8")
+
+            self.assertEqual(
+                resolve_base_commit(make_context(root), "explicit456"),
+                "explicit456",
+            )
+
+    def test_extract_single_commit_uses_base_commit_by_default(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            root = Path(tmp)
+            (root / "BASE_COMMIT").write_text("base123\n", encoding="utf-8")
+            ctx = make_context(root)
+
+            with (
+                patch(
+                    "build.modules.extract.extract_commit.validate_commit_exists",
+                    return_value=True,
+                ),
+                patch(
+                    "build.modules.extract.extract_commit.get_commit_info",
+                    return_value=None,
+                ),
+                patch(
+                    "build.modules.extract.extract_commit.extract_with_base",
+                    return_value=(1, ["chrome/foo.cc"]),
+                ) as extract_with_base_mock,
+            ):
+                result = extract_single_commit(ctx, "HEAD", force=True)
+
+            self.assertEqual(result, (1, ["chrome/foo.cc"]))
+            extract_with_base_mock.assert_called_once_with(
+                ctx, "HEAD", "base123", False, True, False
+            )
+
+    def test_extract_single_file_patch_uses_base_commit_by_default(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            root = Path(tmp)
+            (root / "BASE_COMMIT").write_text("base123\n", encoding="utf-8")
+            ctx = make_context(root)
+            diff_result = SimpleNamespace(returncode=0, stdout="diff", stderr="")
+            patch_file = FilePatch(
+                file_path="chrome/foo.cc",
+                operation=FileOperation.MODIFY,
+                patch_content="diff",
+                is_binary=False,
+            )
+
+            with (
+                patch(
+                    "build.modules.extract.extract_patch.validate_commit_exists",
+                    return_value=True,
+                ) as validate_mock,
+                patch(
+                    "build.modules.extract.extract_patch.run_git_command",
+                    return_value=diff_result,
+                ) as git_mock,
+                patch(
+                    "build.modules.extract.extract_patch.parse_diff_output",
+                    return_value={"chrome/foo.cc": patch_file},
+                ),
+                patch(
+                    "build.modules.extract.extract_patch.write_patch_file",
+                    return_value=True,
+                ),
+            ):
+                success, error = extract_single_file_patch(
+                    ctx, "chrome/foo.cc", None, force=True
+                )
+
+            self.assertTrue(success)
+            self.assertIsNone(error)
+            validate_mock.assert_called_once_with("base123", ctx.chromium_src)
+            git_mock.assert_called_once_with(
+                ["git", "diff", "base123", "--", "chrome/foo.cc"],
+                cwd=ctx.chromium_src,
+            )
+
+    def test_extract_commits_individually_uses_base_commit_by_default(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            root = Path(tmp)
+            (root / "BASE_COMMIT").write_text("base123\n", encoding="utf-8")
+            ctx = make_context(root)
+            rev_list = SimpleNamespace(returncode=0, stdout="commit1\n", stderr="")
+
+            with (
+                patch(
+                    "build.modules.extract.extract_range.validate_commit_exists",
+                    return_value=True,
+                ),
+                patch(
+                    "build.modules.extract.extract_range.run_git_command",
+                    return_value=rev_list,
+                ),
+                patch(
+                    "build.modules.extract.extract_range.extract_with_base",
+                    return_value=(1, ["chrome/foo.cc"]),
+                ) as extract_with_base_mock,
+                patch(
+                    "click.progressbar",
+                    side_effect=lambda items, **_: nullcontext(items),
+                ),
+            ):
+                result = extract_commits_individually(ctx, "START", "END", force=True)
+
+            self.assertEqual(result, (1, ["chrome/foo.cc"]))
+            extract_with_base_mock.assert_called_once_with(
+                ctx,
+                "commit1",
+                "base123",
+                verbose=False,
+                force=True,
+                include_binary=False,
+            )
+
+
+if __name__ == "__main__":
+    unittest.main()

packages/browseros/build/modules/extract/extract_commit.py

@@ -14,7 +14,7 @@ from .utils import (
     validate_commit_exists,
     get_commit_info,
 )
-from .common import extract_normal, extract_with_base
+from .common import extract_with_base, resolve_base_commit
 
 
 def extract_single_commit(
@@ -33,7 +33,7 @@ def extract_single_commit(
         verbose: Show detailed output
         force: Overwrite existing patches
         include_binary: Include binary files
-        base: If provided, extract full diff from base for files in commit
+        base: Base commit to diff from. Defaults to BASE_COMMIT.
 
     Returns:
         Tuple of (count, list of extracted file paths)
@@ -50,16 +50,15 @@ def extract_single_commit(
         )
         log_info(f"  Subject: {commit_info['subject']}")
 
-    if base:
-        # With --base: Get files from commit, but diff from base
-        return extract_with_base(ctx, commit_hash, base, verbose, force, include_binary)
-    else:
-        # Normal behavior: diff against parent
-        return extract_normal(ctx, commit_hash, verbose, force, include_binary)
+    base_commit = resolve_base_commit(ctx, base)
+    return extract_with_base(
+        ctx, commit_hash, base_commit, verbose, force, include_binary
+    )
 
 
 class ExtractCommitModule(CommandModule):
     """Extract patches from a single commit"""
+
     produces = []
     requires = []
     description = "Extract patches from a single commit"
@@ -67,6 +66,7 @@ class ExtractCommitModule(CommandModule):
     def validate(self, ctx: Context) -> None:
         """Validate git repository"""
         import shutil
+
         if not shutil.which("git"):
             raise ValidationError("Git is not available in PATH")
         if not validate_git_repository(ctx.chromium_src):
@@ -93,7 +93,7 @@ class ExtractCommitModule(CommandModule):
             verbose: Show detailed output
             force: Overwrite existing patches
             include_binary: Include binary files
-            base: Extract full diff from base commit for files in COMMIT
+            base: Base commit to diff from. Defaults to BASE_COMMIT.
             feature: Prompt to add extracted files to a feature in features.yaml
         """
         try:

packages/browseros/build/modules/extract/extract_patch.py

@@ -2,7 +2,7 @@
 Extract Patch - Extract patch for a single chromium file.
 """
 
-from typing import Tuple, Optional
+from typing import Optional, Tuple
 
 from ...common.context import Context
 from ...common.utils import log_info, log_warning
@@ -15,12 +15,13 @@ from .utils import (
     FileOperation,
     GitError,
 )
+from .common import resolve_base_commit
 
 
 def extract_single_file_patch(
     build_ctx: Context,
     chromium_path: str,
-    base: str,
+    base: Optional[str] = None,
     force: bool = False,
 ) -> Tuple[bool, Optional[str]]:
     """Extract patch for a single chromium file.
@@ -31,20 +32,25 @@ def extract_single_file_patch(
     Args:
         build_ctx: Build context
         chromium_path: Path to file in chromium (e.g., chrome/common/foo.h)
-        base: Base commit to diff against
+        base: Base commit to diff against. Defaults to BASE_COMMIT.
         force: If True, overwrite existing patch without prompting
 
     Returns:
         Tuple of (success: bool, error_message: Optional[str])
     """
-    if not validate_commit_exists(base, build_ctx.chromium_src):
-        return False, f"Base commit not found: {base}"
+    try:
+        base_commit = resolve_base_commit(build_ctx, base)
+    except GitError as e:
+        return False, str(e)
+
+    if not validate_commit_exists(base_commit, build_ctx.chromium_src):
+        return False, f"Base commit not found: {base_commit}"
 
     log_info(f"Extracting patch for: {chromium_path}")
-    log_info(f"  Base: {base[:12]}")
+    log_info(f"  Base: {base_commit[:12]}")
 
     # Get diff from base to working directory for this file
-    diff_cmd = ["git", "diff", base, "--", chromium_path]
+    diff_cmd = ["git", "diff", base_commit, "--", chromium_path]
     result = run_git_command(diff_cmd, cwd=build_ctx.chromium_src)
 
     if result.returncode != 0:
@@ -54,7 +60,7 @@ def extract_single_file_patch(
         # No diff - check if file exists in base vs working directory
         base_exists = (
             run_git_command(
-                ["git", "cat-file", "-e", f"{base}:{chromium_path}"],
+                ["git", "cat-file", "-e", f"{base_commit}:{chromium_path}"],
                 cwd=build_ctx.chromium_src,
             ).returncode
             == 0
@@ -64,7 +70,10 @@ def extract_single_file_patch(
         working_exists = working_file.exists()
 
         if not base_exists and not working_exists:
-            return False, f"File does not exist in base or working directory: {chromium_path}"
+            return (
+                False,
+                f"File does not exist in base or working directory: {chromium_path}",
+            )
 
         if base_exists and working_exists:
             return False, f"No changes found for: {chromium_path}"
@@ -97,7 +106,9 @@ def extract_single_file_patch(
     if patch_path.exists() and not force:
         import click
 
-        if not click.confirm(f"Patch already exists: {chromium_path}. Overwrite?", default=False):
+        if not click.confirm(
+            f"Patch already exists: {chromium_path}. Overwrite?", default=False
+        ):
             log_info("Extraction cancelled")
             return False, "Cancelled by user"
 

packages/browseros/build/modules/extract/extract_range.py

@@ -22,8 +22,7 @@ from .utils import (
     create_binary_marker,
     log_extraction_summary,
 )
-from .common import check_overwrite, extract_with_base
-from .extract_commit import extract_single_commit
+from .common import check_overwrite, extract_with_base, resolve_base_commit
 
 
 def get_range_changed_files_with_status(
@@ -78,8 +77,10 @@ def extract_commit_range(
         raise GitError(f"Base commit not found: {base_commit}")
     if not validate_commit_exists(head_commit, ctx.chromium_src):
         raise GitError(f"Head commit not found: {head_commit}")
-    if custom_base and not validate_commit_exists(custom_base, ctx.chromium_src):
-        raise GitError(f"Custom base commit not found: {custom_base}")
+    diff_base = resolve_base_commit(ctx, custom_base)
+    if not validate_commit_exists(diff_base, ctx.chromium_src):
+        label = "Custom base" if custom_base else "BASE_COMMIT"
+        raise GitError(f"{label} commit not found: {diff_base}")
 
     # Count commits in range for progress
     result = run_git_command(
@@ -94,63 +95,47 @@ def extract_commit_range(
 
     log_info(f"Processing {commit_count} commits")
 
-    # Step 2: Get diff based on whether we have a custom base
-    if custom_base:
-        # Get files changed in range WITH status to handle deletions correctly
-        changed_files = get_range_changed_files_with_status(
-            base_commit, head_commit, ctx.chromium_src
+    # Get files changed in range WITH status to handle deletions correctly
+    changed_files = get_range_changed_files_with_status(
+        base_commit, head_commit, ctx.chromium_src
+    )
+
+    if not changed_files:
+        log_warning("No files changed in range")
+        return 0, []
+
+    log_info(f"Found {len(changed_files)} files changed in range")
+
+    # Separate deleted files from others
+    deleted_files = [f for f, s in changed_files.items() if s == "D"]
+    non_deleted_files = [f for f, s in changed_files.items() if s != "D"]
+
+    file_patches = {}
+
+    # Handle deleted files directly
+    for file_path in deleted_files:
+        file_patches[file_path] = FilePatch(
+            file_path=file_path,
+            operation=FileOperation.DELETE,
+            patch_content=None,
+            is_binary=False,
         )
 
-        if not changed_files:
-            log_warning("No files changed in range")
-            return 0, []
-
-        log_info(f"Found {len(changed_files)} files changed in range")
-
-        # Separate deleted files from others
-        deleted_files = [f for f, s in changed_files.items() if s == "D"]
-        non_deleted_files = [f for f, s in changed_files.items() if s != "D"]
-
-        file_patches = {}
-
-        # Handle deleted files directly
-        for file_path in deleted_files:
-            file_patches[file_path] = FilePatch(
-                file_path=file_path,
-                operation=FileOperation.DELETE,
-                patch_content=None,
-                is_binary=False,
-            )
-
-        # Get diff from custom base for non-deleted files
-        if non_deleted_files:
-            diff_cmd = ["git", "diff", f"{custom_base}..{head_commit}"]
-            if include_binary:
-                diff_cmd.append("--binary")
-            diff_cmd.append("--")
-            diff_cmd.extend(non_deleted_files)
-
-            result = run_git_command(diff_cmd, cwd=ctx.chromium_src, timeout=120)
-
-            if result.returncode != 0:
-                raise GitError(f"Failed to get diff for range: {result.stderr}")
-
-            # Parse and merge with deleted files
-            parsed_patches = parse_diff_output(result.stdout)
-            file_patches.update(parsed_patches)
-    else:
-        # Regular diff from base_commit to head_commit
-        diff_cmd = ["git", "diff", f"{base_commit}..{head_commit}"]
+    # Get diff from BASE_COMMIT/custom base for non-deleted files.
+    if non_deleted_files:
+        diff_cmd = ["git", "diff", f"{diff_base}..{head_commit}"]
         if include_binary:
             diff_cmd.append("--binary")
+        diff_cmd.append("--")
+        diff_cmd.extend(non_deleted_files)
 
         result = run_git_command(diff_cmd, cwd=ctx.chromium_src, timeout=120)
 
         if result.returncode != 0:
             raise GitError(f"Failed to get diff for range: {result.stderr}")
 
-        # Parse diff into file patches
-        file_patches = parse_diff_output(result.stdout)
+        parsed_patches = parse_diff_output(result.stdout)
+        file_patches.update(parsed_patches)
 
     if not file_patches:
         log_warning("No changes found in commit range")
@@ -227,9 +212,10 @@ def extract_commits_individually(
     Returns:
         Tuple of (count, list of extracted file paths)
     """
-    # Validate custom base if provided
-    if custom_base and not validate_commit_exists(custom_base, ctx.chromium_src):
-        raise GitError(f"Custom base commit not found: {custom_base}")
+    diff_base = resolve_base_commit(ctx, custom_base)
+    if not validate_commit_exists(diff_base, ctx.chromium_src):
+        label = "Custom base" if custom_base else "BASE_COMMIT"
+        raise GitError(f"{label} commit not found: {diff_base}")
 
     # Get list of commits in range
     result = run_git_command(
@@ -247,8 +233,7 @@ def extract_commits_individually(
         return 0, []
 
     log_info(f"Extracting patches from {len(commits)} commits individually")
-    if custom_base:
-        log_info(f"Using custom base: {custom_base}")
+    log_info(f"Using base: {diff_base}")
 
     total_extracted = 0
     all_extracted_files: List[str] = []
@@ -259,25 +244,14 @@ def extract_commits_individually(
     ) as commits_bar:
         for commit in commits_bar:
             try:
-                if custom_base:
-                    # Use extract_with_base for full diff from custom base
-                    extracted, files = extract_with_base(
-                        ctx,
-                        commit,
-                        custom_base,
-                        verbose=False,
-                        force=force,
-                        include_binary=include_binary,
-                    )
-                else:
-                    # Normal extraction from parent
-                    extracted, files = extract_single_commit(
-                        ctx,
-                        commit,
-                        verbose=False,
-                        force=force,
-                        include_binary=include_binary,
-                    )
+                extracted, files = extract_with_base(
+                    ctx,
+                    commit,
+                    diff_base,
+                    verbose=False,
+                    force=force,
+                    include_binary=include_binary,
+                )
                 total_extracted += extracted
                 all_extracted_files.extend(files)
             except GitError as e:
@@ -299,6 +273,7 @@ def extract_commits_individually(
 
 class ExtractRangeModule(CommandModule):
     """Extract patches from a range of commits"""
+
     produces = []
     requires = []
     description = "Extract patches from a range of commits"
@@ -306,6 +281,7 @@ class ExtractRangeModule(CommandModule):
     def validate(self, ctx: Context) -> None:
         """Validate git repository"""
         import shutil
+
         if not shutil.which("git"):
             raise ValidationError("Git is not available in PATH")
         if not validate_git_repository(ctx.chromium_src):
@@ -336,7 +312,7 @@ class ExtractRangeModule(CommandModule):
             force: Overwrite existing patches
             include_binary: Include binary files
             squash: Squash all commits into single patches
-            base: Use different base for diff (full diff from base for files in range)
+            base: Base commit to diff from. Defaults to BASE_COMMIT.
             feature: Prompt to add extracted files to a feature in features.yaml
         """
         try:
@@ -363,7 +339,9 @@ class ExtractRangeModule(CommandModule):
             if count == 0:
                 log_warning(f"No patches extracted from range {start}..{end}")
             else:
-                log_success(f"Successfully extracted {count} patches from {start}..{end}")
+                log_success(
+                    f"Successfully extracted {count} patches from {start}..{end}"
+                )
 
                 # Handle --feature flag
                 if feature and extracted_files: