Fix base-cli review findings

README.md

@@ -306,9 +306,15 @@ def helper() -> None:
 `--quiet` suppresses INFO output on the user-facing stream but still shows
 warnings and errors. `--debug` and `--quiet` cannot be used together. Persistent
 log files still receive DEBUG-level detail, including INFO messages suppressed
-from stderr. When `basectl --color` is used on a terminal, the user-facing
-Python logs use the same level colors as Bash logs; persistent log files remain
-plain text. `NO_COLOR` disables colors.
+from stderr. User-facing logs use colors automatically on interactive terminals;
+persistent log files remain plain text. Set `NO_COLOR=1` or
+`BASE_CLI_COLOR=0` to disable colors. The Base wrapper's `--color` option
+remains compatible with this behavior.
+
+Click also provides shell completion. For an app named `hello`, request a
+completion script with `_HELLO_COMPLETE=bash_source hello`, replacing `bash`
+with `zsh` or `fish` as needed. `base_cli` leaves installation to the caller so
+shell startup files remain under user control.
 
 Advanced tests and CI wrappers can call `base_cli.configure_logger(...,
 stream=..., formatter=...)` to capture user-facing logs or apply a custom
@@ -406,7 +412,7 @@ for those structured values.
 The user config file is machine-local by default. Base owns the semantics of
 `~/.base.d/config.yaml`, while users own backup and sync choices such as iCloud,
 chezmoi, dotfiles repositories, Time Machine, or manual copy. See
-`docs/local-config.md` for the product-level boundary.
+[`docs/local-config.md`](docs/local-config.md) for the product-level boundary.
 
 ## Project Discovery
 
@@ -426,7 +432,7 @@ actionable message.
 
 Runtime state is rooted at `~/Library/Caches/base` on macOS and `~/.cache/base`
 elsewhere. `BASE_CACHE_DIR` overrides the root. See
-[`docs/cache-ownership-and-layout.md`](../../../docs/cache-ownership-and-layout.md)
+[`docs/cache-ownership-and-layout.md`](docs/cache-ownership-and-layout.md)
 for the owner-aware layout. Base control-plane commands use `base/`; a
 Base-compliant project's own commands use `projects/<project>/<checkout-id>/`.
 Each invocation is a run bundle containing private (`0600`) `run.json`,

docs/cache-ownership-and-layout.md

@@ -0,0 +1,18 @@
+# Cache ownership and layout
+
+Runtime state is rooted at `~/Library/Caches/base` on macOS and `~/.cache/base`
+elsewhere. Set `BASE_CACHE_DIR` to override the root.
+
+The `base` owner stores Base control-plane runs directly below `base/`. A
+project-owned runtime uses `projects/<project>/<checkout-id>/` so separate
+checkouts do not share mutable run state accidentally.
+
+Each invocation has a private run bundle containing:
+
+- `run.json` for lifecycle metadata;
+- `logs/` for diagnostic logs; and
+- `tmp/` for temporary command data.
+
+Persistent component caches live under the owner's `cache/components/` path.
+Runtime directories are owner-only (`0700`), and runtime files are owner-only
+(`0600`).

docs/local-config.md

@@ -0,0 +1,11 @@
+# Local configuration
+
+`base-cli` reads machine-local configuration from `~/.base.d/config.yaml`.
+Project configuration is read from `<project>/.base/config.yaml`, and an
+explicit `--config` file can provide the final project-specific override.
+
+The package owns the configuration schema and merge semantics. Users own the
+operational choice of whether to back up or synchronize the machine-local file,
+using tools such as iCloud, chezmoi, a dotfiles repository, Time Machine, or a
+manual copy. The file can contain paths and other machine-specific values and
+should not be synchronized blindly across incompatible machines.

lib/python/base_cli/README.md

@@ -391,7 +391,7 @@ for those structured values.
 The user config file is machine-local by default. Base owns the semantics of
 `~/.base.d/config.yaml`, while users own backup and sync choices such as iCloud,
 chezmoi, dotfiles repositories, Time Machine, or manual copy. See
-`docs/local-config.md` for the product-level boundary.
+[`docs/local-config.md`](../../../docs/local-config.md) for the product-level boundary.
 
 ## Project Discovery
 

lib/python/base_cli/__init__.py

@@ -7,8 +7,16 @@
 def _resolve_version() -> str:
     """Return the checkout version or the installed distribution version."""
 
-    for parent in Path(__file__).resolve().parents:
-        version_file = parent / "VERSION"
+    package_dir = Path(__file__).resolve().parent
+    python_dir = package_dir.parent
+    lib_dir = python_dir.parent
+    checkout_root = lib_dir.parent
+    if (
+        python_dir.name == "python"
+        and lib_dir.name == "lib"
+        and (checkout_root / "pyproject.toml").is_file()
+    ):
+        version_file = checkout_root / "VERSION"
         if version_file.is_file():
             value = version_file.read_text(encoding="utf-8").splitlines()[0].strip()
             if value:
@@ -25,7 +33,17 @@ def _resolve_version() -> str:
 from . import command_filters, command_protocol, history, testing
 from .app import App, argument, command, delegated_display_command, option, run_app
 from .command_filters import command_matches, normalize_command_filter, normalize_command_filters
-from .command_protocol import CommandProtocolError, dumps_record, dumps_records, loads_records
+from .command_protocol import (
+    BOOLEAN,
+    NULLABLE_STRING,
+    STRING,
+    CommandProtocolError,
+    FieldSpec,
+    dumps_record,
+    dumps_records,
+    loads_records,
+    register_record_schema,
+)
 from .config import UserConfig, UserGithubConfig, UserIdeConfig, UserIdePreference, UserWorkspaceConfig
 from .context import Context, get_current_context
 from .exit_codes import ExitCode
@@ -44,9 +62,13 @@ def _resolve_version() -> str:
 __all__ = [
     "App",
     "__version__",
+    "BOOLEAN",
     "CommandProtocolError",
     "Context",
     "ExitCode",
+    "FieldSpec",
+    "NULLABLE_STRING",
+    "STRING",
     "UserConfig",
     "UserGithubConfig",
     "UserIdeConfig",
@@ -81,6 +103,7 @@ def _resolve_version() -> str:
     "option",
     "render_document",
     "render_records",
+    "register_record_schema",
     "resolve_output_format",
     "run_app",
 ]

lib/python/base_cli/_dependencies.py

@@ -0,0 +1,15 @@
+"""Optional dependency loaders shared by base_cli modules."""
+
+from __future__ import annotations
+
+from typing import Any
+
+
+def require_yaml(error_message: str) -> Any:
+    """Import PyYAML or raise the caller's feature-specific error."""
+
+    try:
+        import yaml
+    except ImportError as exc:
+        raise RuntimeError(error_message) from exc
+    return yaml

lib/python/base_cli/_private_files.py

@@ -0,0 +1,38 @@
+"""Helpers for writing private runtime files."""
+
+from __future__ import annotations
+
+import json
+import os
+from collections.abc import Mapping
+from pathlib import Path
+from typing import Any
+
+
+PRIVATE_FILE_MODE = 0o600
+
+
+def restrict_file(path: Path) -> None:
+    """Ensure an existing runtime file is readable and writable only by its owner."""
+
+    path.chmod(PRIVATE_FILE_MODE)
+
+
+def write_private_json(path: Path, value: Mapping[str, Any]) -> None:
+    """Write a JSON mapping with owner-only permissions from the moment it is created."""
+
+    path.parent.mkdir(parents=True, exist_ok=True)
+    fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, PRIVATE_FILE_MODE)
+    try:
+        fchmod = getattr(os, "fchmod", None)
+        if fchmod is not None:
+            fchmod(fd, PRIVATE_FILE_MODE)
+        stream = os.fdopen(fd, "w", encoding="utf-8")
+        fd = -1
+        with stream:
+            json.dump(value, stream, sort_keys=True)
+            stream.write("\n")
+    finally:
+        if fd != -1:
+            os.close(fd)
+    restrict_file(path)

lib/python/base_cli/_runtime.py

@@ -1,9 +1,11 @@
 from __future__ import annotations
 
+import json
 import logging
 from dataclasses import dataclass
 from pathlib import Path
 
+from ._private_files import write_private_json
 from .paths import runtime_owner_root, runtime_run_directory_name
 
 
@@ -17,6 +19,9 @@ class RuntimeLayout:
     temp_dir: Path
 
 
+_LOG_INDEX_NAME = ".base-cli-log-index.json"
+
+
 # pylint: disable=too-many-arguments
 def runtime_layout(
     cache_root: Path,
@@ -45,8 +50,17 @@ def runtime_layout(
 
 
 def create_runtime_directory(path: Path, cache_root: Path) -> None:
+    missing: list[Path] = []
+    candidate = path
+    while not candidate.exists():
+        missing.append(candidate)
+        candidate = candidate.parent
+    restrict_permissions = _is_within(path, cache_root)
     try:
         path.mkdir(parents=True, exist_ok=True)
+        if restrict_permissions:
+            for directory in [path, *missing]:
+                directory.chmod(0o700)
     except OSError as exc:
         raise RuntimeError(_runtime_directory_error(path, cache_root, exc)) from exc
 
@@ -57,21 +71,48 @@ def prune_log_files(
     max_log_files: int,
     logger: logging.Logger,
 ) -> None:
-    candidates: list[tuple[str, Path]] = []
-    for path in log_dir.rglob("*.log"):
-        if _same_path(path, current_log_file):
-            continue
-        candidates.append((path.name, path))
+    index_path = log_dir / _LOG_INDEX_NAME
+    tracked = _read_log_index(index_path)
+    if tracked is None:
+        tracked = {path.resolve() for path in log_dir.glob("*/logs/*.log")}
+    tracked.add(current_log_file.resolve())
+    candidates = [(path.name, path) for path in tracked if not _same_path(path, current_log_file)]
 
     excess_count = len(candidates) + 1 - max_log_files
-    if excess_count <= 0:
-        return
-
-    for _, path in sorted(candidates)[:excess_count]:
-        try:
-            path.unlink()
-        except OSError as exc:
-            logger.warning("Could not prune log file '%s': %s", path, exc)
+    if excess_count > 0:
+        for _, path in sorted(candidates)[:excess_count]:
+            try:
+                path.unlink()
+                tracked.discard(path)
+            except OSError as exc:
+                logger.warning("Could not prune log file '%s': %s", path, exc)
+
+    tracked = {path for path in tracked if path.exists() or _same_path(path, current_log_file)}
+    try:
+        write_private_json(index_path, {"version": 1, "logs": sorted(str(path) for path in tracked)})
+    except (OSError, TypeError, ValueError) as exc:
+        logger.debug("Could not update log retention index '%s': %s", index_path, exc)
+
+
+def _read_log_index(path: Path) -> set[Path] | None:
+    try:
+        payload = json.loads(path.read_text(encoding="utf-8"))
+    except (OSError, json.JSONDecodeError):
+        return None
+    if not isinstance(payload, dict) or payload.get("version") != 1:
+        return None
+    logs = payload.get("logs")
+    if not isinstance(logs, list) or not all(isinstance(value, str) for value in logs):
+        return None
+    return {Path(value) for value in logs}
+
+
+def _is_within(path: Path, root: Path) -> bool:
+    try:
+        path.resolve().relative_to(root.resolve())
+    except ValueError:
+        return False
+    return True
 
 
 def _runtime_directory_error(path: Path, cache_root: Path, exc: OSError) -> str:

lib/python/base_cli/app.py

@@ -1,14 +1,14 @@
 from __future__ import annotations
 
 import functools
-import json
 import os
 import sys
 from contextvars import ContextVar
 from pathlib import Path
 from typing import Any, Callable
 
 from ._runtime import create_runtime_directory, prune_log_files, runtime_layout
+from ._private_files import write_private_json
 from .config import load_config, read_user_config
 from .context import Context, reset_current_context, set_current_context
 from .exit_codes import ExitCode
@@ -170,7 +170,10 @@ def wrapper(**kwargs: Any):
                 if context.manifest_path is not None:
                     context.log.debug("manifest_path=%s", context.manifest_path)
                 result = func(context, **kwargs)
-                exit_code = int(result or ExitCode.SUCCESS)
+                try:
+                    exit_code = _normalize_command_result(result)
+                except TypeError as exc:
+                    raise click.ClickException(str(exc)) from exc
                 return result
             except Exception:
                 exit_code = ExitCode.FAILURE
@@ -238,6 +241,7 @@ def _create_context(self, standard: dict[str, Any], sensitive_options: set[str],
                 log_file = _default_log_file(layout, inherited_path)
             create_runtime_directory(log_file.parent, cache_root)
         if inherited_path is None and not dry_run and self.log_to_file:
+            create_runtime_directory(layout.owner_root, cache_root)
             create_runtime_directory(layout.run_root, cache_root)
             try:
                 run_metadata = {
@@ -252,33 +256,24 @@ def _create_context(self, standard: dict[str, Any], sensitive_options: set[str],
                     "workspace_root": str(user_config.workspace.root) if user_config.workspace.root else None,
                 }
                 run_metadata_path = layout.run_root / "run.json"
-                run_metadata_path.write_text(
-                    json.dumps(run_metadata, sort_keys=True) + "\n",
-                    encoding="utf-8",
-                )
-                run_metadata_path.chmod(0o600)
+                write_private_json(run_metadata_path, run_metadata)
             except OSError:
                 pass
         if runtime_owner == "project" and selected_project_root is not None and not dry_run and self.log_to_file:
             try:
                 create_runtime_directory(layout.owner_root, cache_root)
                 identity_path = layout.owner_root / "identity.json"
                 if not identity_path.exists():
-                    identity_path.write_text(
-                        json.dumps(
-                            {
-                                "schema_version": 1,
-                                "project": selected_project_name,
-                                "project_root": str(selected_project_root),
-                                "manifest": str(manifest_path) if manifest_path is not None else None,
-                                "checkout_id": layout.owner_root.name,
-                            },
-                            sort_keys=True,
-                        )
-                        + "\n",
-                        encoding="utf-8",
+                    write_private_json(
+                        identity_path,
+                        {
+                            "schema_version": 1,
+                            "project": selected_project_name,
+                            "project_root": str(selected_project_root),
+                            "manifest": str(manifest_path) if manifest_path is not None else None,
+                            "checkout_id": layout.owner_root.name,
+                        },
                     )
-                identity_path.chmod(0o600)
             except OSError:
                 pass
         logger = configure_logger(self.name, log_file, debug, quiet=quiet)
@@ -341,7 +336,22 @@ def run_app(app: App, argv: list[str] | None = None) -> int:
     except click.ClickException as exc:
         exc.show()
         return int(exc.exit_code)
-    return int(result or 0)
+    try:
+        return _normalize_command_result(result)
+    except TypeError as exc:
+        print(f"ERROR: {exc}", file=sys.stderr)
+        return ExitCode.FAILURE
+
+
+def _normalize_command_result(result: Any) -> int:
+    if result is None:
+        return ExitCode.SUCCESS
+    if isinstance(result, int):
+        return result
+    raise TypeError(
+        "Commands must return None or an int exit code; "
+        f"got {type(result).__name__}."
+    )
 
 
 def _effective_invocation_argv(