From 0583cb53407196e297f8079585ac67f0c1908c13 Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah <22363102+codeforester@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:28:11 -0700 Subject: [PATCH] fix review findings --- README.md | 16 ++++-- docs/cache-ownership-and-layout.md | 18 +++++++ docs/local-config.md | 11 ++++ lib/python/base_cli/README.md | 2 +- lib/python/base_cli/__init__.py | 29 +++++++++-- lib/python/base_cli/_dependencies.py | 15 ++++++ lib/python/base_cli/_private_files.py | 38 ++++++++++++++ lib/python/base_cli/_runtime.py | 67 ++++++++++++++++++++----- lib/python/base_cli/app.py | 54 ++++++++++++-------- lib/python/base_cli/command_protocol.py | 37 ++++++++++++++ lib/python/base_cli/config.py | 6 +-- lib/python/base_cli/history.py | 33 +++++++++--- lib/python/base_cli/logging.py | 2 +- lib/python/base_cli/output.py | 12 ++--- lib/python/base_cli/testing.py | 22 +++++--- tests/test_app_log_retention.py | 22 ++++++++ tests/test_app_run.py | 24 +++++++++ tests/test_app_runtime_boundary.py | 20 ++++++++ tests/test_command_protocol.py | 31 ++++++++++++ tests/test_history.py | 27 ++++++++++ tests/test_logging.py | 11 +++- tests/test_public_api.py | 28 ++++++++++- tests/test_testing.py | 14 +++--- 23 files changed, 460 insertions(+), 79 deletions(-) create mode 100644 docs/cache-ownership-and-layout.md create mode 100644 docs/local-config.md create mode 100644 lib/python/base_cli/_dependencies.py create mode 100644 lib/python/base_cli/_private_files.py diff --git a/README.md b/README.md index 4004f49..4ab3f5a 100644 --- a/README.md +++ b/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///`. Each invocation is a run bundle containing private (`0600`) `run.json`, diff --git a/docs/cache-ownership-and-layout.md b/docs/cache-ownership-and-layout.md new file mode 100644 index 0000000..476d200 --- /dev/null +++ b/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///` 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`). diff --git a/docs/local-config.md b/docs/local-config.md new file mode 100644 index 0000000..b4e48d2 --- /dev/null +++ b/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 `/.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. diff --git a/lib/python/base_cli/README.md b/lib/python/base_cli/README.md index 1913358..c687bc5 100644 --- a/lib/python/base_cli/README.md +++ b/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 diff --git a/lib/python/base_cli/__init__.py b/lib/python/base_cli/__init__.py index d6440fc..6847601 100644 --- a/lib/python/base_cli/__init__.py +++ b/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", ] diff --git a/lib/python/base_cli/_dependencies.py b/lib/python/base_cli/_dependencies.py new file mode 100644 index 0000000..ea970a8 --- /dev/null +++ b/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 diff --git a/lib/python/base_cli/_private_files.py b/lib/python/base_cli/_private_files.py new file mode 100644 index 0000000..f7bbacb --- /dev/null +++ b/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) diff --git a/lib/python/base_cli/_runtime.py b/lib/python/base_cli/_runtime.py index a8cf838..8f82414 100644 --- a/lib/python/base_cli/_runtime.py +++ b/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: diff --git a/lib/python/base_cli/app.py b/lib/python/base_cli/app.py index c9e869c..e1fc857 100644 --- a/lib/python/base_cli/app.py +++ b/lib/python/base_cli/app.py @@ -1,7 +1,6 @@ from __future__ import annotations import functools -import json import os import sys from contextvars import ContextVar @@ -9,6 +8,7 @@ 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,11 +256,7 @@ 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: @@ -264,21 +264,16 @@ def _create_context(self, standard: dict[str, Any], sensitive_options: set[str], 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( diff --git a/lib/python/base_cli/command_protocol.py b/lib/python/base_cli/command_protocol.py index 988d0bd..5851533 100644 --- a/lib/python/base_cli/command_protocol.py +++ b/lib/python/base_cli/command_protocol.py @@ -6,10 +6,15 @@ __all__ = [ + "BOOLEAN", "CommandProtocolError", + "FieldSpec", + "NULLABLE_STRING", + "STRING", "dumps_record", "dumps_records", "loads_records", + "register_record_schema", ] @@ -88,6 +93,38 @@ class FieldSpec: Record = Mapping[str, RecordValue] +def register_record_schema(record_type: str, fields: Mapping[str, FieldSpec]) -> None: + """Register an application-specific record schema for the wire protocol. + + Schema names and field names are intentionally constrained to the same + framing-safe characters used by the built-in protocol. Registration is + additive and rejects replacement of an existing schema so that a process + cannot silently change the meaning of an established record type. + """ + + if not isinstance(record_type, str) or re.fullmatch(r"[A-Za-z][A-Za-z0-9-]*", record_type) is None: + raise CommandProtocolError( + "record_type must start with a letter and contain only letters, digits, and hyphens" + ) + if record_type in RECORD_SCHEMAS: + raise CommandProtocolError(f"record_type '{record_type}' is already registered") + if not isinstance(fields, Mapping) or not fields: + raise CommandProtocolError("record schema fields must be a non-empty mapping") + + normalized: dict[str, FieldSpec] = {} + for field_name, spec in fields.items(): + if not isinstance(field_name, str) or re.fullmatch(r"[A-Za-z][A-Za-z0-9_]*", field_name) is None: + raise CommandProtocolError( + f"field name '{field_name}' must start with a letter and contain only letters, digits, and underscores" + ) + if not isinstance(spec, FieldSpec) or spec.value_type not in {"string", "boolean"}: + raise CommandProtocolError( + f"field '{field_name}' must use a FieldSpec with value_type 'string' or 'boolean'" + ) + normalized[field_name] = spec + RECORD_SCHEMAS[record_type] = normalized + + def dumps_record(record_type: str, record: Record) -> str: return dumps_records(record_type, (record,)) diff --git a/lib/python/base_cli/config.py b/lib/python/base_cli/config.py index 68aabe5..07a76d5 100644 --- a/lib/python/base_cli/config.py +++ b/lib/python/base_cli/config.py @@ -6,6 +6,7 @@ import re from typing import Any +from ._dependencies import require_yaml from .ide_schema import SUPPORTED_IDES from .ide_schema import parse_ide_extensions from .ide_schema import parse_ide_settings @@ -64,10 +65,7 @@ def load_yaml_file(path: Path) -> dict[str, Any]: if not path.is_file(): return {} - try: - import yaml - except ImportError as exc: - raise RuntimeError("PyYAML is required to load base_cli configuration.") from exc + yaml = require_yaml("PyYAML is required to load base_cli configuration.") try: data = yaml.safe_load(path.read_text(encoding="utf-8")) diff --git a/lib/python/base_cli/history.py b/lib/python/base_cli/history.py index 66775d4..1a8d11f 100644 --- a/lib/python/base_cli/history.py +++ b/lib/python/base_cli/history.py @@ -12,6 +12,12 @@ except ImportError: # pragma: no cover - fcntl is unavailable on Windows. _fcntl = None # type: ignore[assignment] +try: + import msvcrt as _msvcrt +except ImportError: # pragma: no cover - msvcrt is unavailable outside Windows. + _msvcrt = None # type: ignore[assignment] + +from ._private_files import restrict_file, write_private_json from .config import load_yaml_file from .context import Context from .paths import base_cache_root @@ -211,7 +217,7 @@ def write_history_record(record: dict[str, Any]) -> None: path = base_cache_root() / HISTORY_PATH path.parent.mkdir(parents=True, exist_ok=True) append_history_line(path, f"{json.dumps(record, sort_keys=True)}\n") - path.chmod(0o600) + restrict_file(path) def runtime_bundle_path() -> Path | None: @@ -251,33 +257,48 @@ def update_run_metadata(run_root: Path, record: dict[str, Any]) -> None: ): if key in record and record[key] is not None: metadata[key] = record[key] - metadata_path.parent.mkdir(parents=True, exist_ok=True) - metadata_path.write_text(json.dumps(metadata, sort_keys=True) + "\n", encoding="utf-8") - metadata_path.chmod(0o600) + write_private_json(metadata_path, metadata) except (OSError, TypeError, ValueError): pass def append_history_line(path: Path, line: str) -> None: fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_APPEND, 0o600) + lock_fd = fd + sidecar_fd: int | None = None try: - lock_history_file(fd) + if _fcntl is None and _msvcrt is not None: + sidecar_path = path.with_name(f".{path.name}.lock") + sidecar_fd = os.open(sidecar_path, os.O_RDWR | os.O_CREAT, 0o600) + if os.fstat(sidecar_fd).st_size == 0: + os.write(sidecar_fd, b"0") + restrict_file(sidecar_path) + lock_fd = sidecar_fd + lock_history_file(lock_fd) try: write_all(fd, line.encode("utf-8")) finally: - unlock_history_file(fd) + unlock_history_file(lock_fd) finally: + if sidecar_fd is not None: + os.close(sidecar_fd) os.close(fd) def lock_history_file(fd: int) -> None: if _fcntl is not None: _fcntl.flock(fd, _fcntl.LOCK_EX) + elif _msvcrt is not None: + os.lseek(fd, 0, os.SEEK_SET) + _msvcrt.locking(fd, _msvcrt.LK_LOCK, 1) def unlock_history_file(fd: int) -> None: if _fcntl is not None: _fcntl.flock(fd, _fcntl.LOCK_UN) + elif _msvcrt is not None: + os.lseek(fd, 0, os.SEEK_SET) + _msvcrt.locking(fd, _msvcrt.LK_UNLCK, 1) def write_all(fd: int, data: bytes) -> None: diff --git a/lib/python/base_cli/logging.py b/lib/python/base_cli/logging.py index fb1468d..4b49b35 100644 --- a/lib/python/base_cli/logging.py +++ b/lib/python/base_cli/logging.py @@ -69,7 +69,7 @@ def _handler_formatter(formatter: logging.Formatter | None, *, use_color: bool) def _use_color(stream: TextIO) -> bool: return ( - os.environ.get("BASE_CLI_COLOR") == "1" + os.environ.get("BASE_CLI_COLOR") != "0" and "NO_COLOR" not in os.environ and hasattr(stream, "isatty") and stream.isatty() diff --git a/lib/python/base_cli/output.py b/lib/python/base_cli/output.py index 92882bd..5ca4d61 100644 --- a/lib/python/base_cli/output.py +++ b/lib/python/base_cli/output.py @@ -8,6 +8,8 @@ from collections.abc import Iterable, Mapping, Sequence from typing import Any, TextIO +from ._dependencies import require_yaml + PUBLIC_OUTPUT_FORMATS = ("text", "csv", "tsv", "yaml", "json") @@ -90,10 +92,7 @@ def render_records( return resolved if resolved == "yaml": - try: - import yaml - except ImportError as exc: # pragma: no cover - environment guard - raise RuntimeError("PyYAML is required for YAML output.") from exc + yaml = require_yaml("PyYAML is required for YAML output.") target.write(yaml.safe_dump(record_list, sort_keys=False, allow_unicode=True)) return resolved @@ -126,10 +125,7 @@ def render_document( target.write("\n") return resolved if resolved == "yaml": - try: - import yaml - except ImportError as exc: # pragma: no cover - environment guard - raise RuntimeError("PyYAML is required for YAML output.") from exc + yaml = require_yaml("PyYAML is required for YAML output.") target.write(yaml.safe_dump(dict(document), sort_keys=False, allow_unicode=True)) return resolved diff --git a/lib/python/base_cli/testing.py b/lib/python/base_cli/testing.py index 3faa329..693fdee 100644 --- a/lib/python/base_cli/testing.py +++ b/lib/python/base_cli/testing.py @@ -1,11 +1,13 @@ from __future__ import annotations import inspect +import os from collections.abc import Mapping from pathlib import Path from typing import TYPE_CHECKING, Any from .paths import use_working_dir +from ._dependencies import require_yaml if TYPE_CHECKING: from click.testing import Result @@ -41,17 +43,21 @@ def invoke( runner_kwargs["mix_stderr"] = False runner = CliRunner(**runner_kwargs) with use_working_dir(cwd_path): - return runner.invoke(app.click_command, args or [], env=invoke_env) + if cwd_path is None: + return runner.invoke(app.click_command, args or [], env=invoke_env) + original_cwd = Path.cwd() + os.chdir(cwd_path) + try: + return runner.invoke(app.click_command, args or [], env=invoke_env) + finally: + os.chdir(original_cwd) def _write_manifest_fixture(cwd: Path, manifest: Mapping[str, Any]) -> None: - try: - import yaml - except ImportError as exc: - raise RuntimeError( - "PyYAML is required to write base_cli.testing manifest fixtures. " - "Install it with 'pip install PyYAML'." - ) from exc + yaml = require_yaml( + "PyYAML is required to write base_cli.testing manifest fixtures. " + "Install it with 'pip install PyYAML'." + ) (cwd / "base_manifest.yaml").write_text( yaml.safe_dump(dict(manifest), sort_keys=False), diff --git a/tests/test_app_log_retention.py b/tests/test_app_log_retention.py index 2e4da0c..9d24228 100644 --- a/tests/test_app_log_retention.py +++ b/tests/test_app_log_retention.py @@ -5,6 +5,7 @@ import tempfile import unittest from pathlib import Path +from unittest import mock import base_cli from base_cli.testing import invoke @@ -17,6 +18,27 @@ def write_log_file(path: Path, mtime: int) -> None: class AppLogRetentionTests(unittest.TestCase): + @unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") + def test_uses_retention_index_after_initial_discovery(self) -> None: + app = base_cli.App(name="retention-index", max_log_files=2) + + @app.command() + def main(ctx: base_cli.Context) -> None: + ctx.log.info("indexed retention") + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + first = invoke(app, home=home) + self.assertEqual(first.exit_code, 0, first.output) + + with mock.patch( + "base_cli._runtime.Path.glob", + side_effect=AssertionError("retention should use its index after initial discovery"), + ): + second = invoke(app, home=home) + + self.assertEqual(second.exit_code, 0, second.output) + @unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") def test_prunes_oldest_default_logs(self) -> None: app = base_cli.App(name="retention-demo", max_log_files=2) diff --git a/tests/test_app_run.py b/tests/test_app_run.py index def4217..b03522e 100644 --- a/tests/test_app_run.py +++ b/tests/test_app_run.py @@ -65,6 +65,30 @@ def main(ctx: base_cli.Context) -> None: with self.assertRaisesRegex(RuntimeError, "boom"): base_cli.run_app(app, []) + @unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") + def test_run_app_reports_invalid_command_return_values(self) -> None: + app = base_cli.App(name="invalid-return", log_to_file=False) + + @app.command() + def main(ctx: base_cli.Context) -> dict[str, str]: + del ctx + return {"status": "bad"} + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + stderr = io.StringIO() + with mock.patch.dict( + os.environ, + { + "HOME": str(home), + "BASE_CACHE_DIR": str(home / ".cache" / "base"), + }, + ), redirect_stderr(stderr): + status = base_cli.run_app(app, []) + + self.assertEqual(status, 1) + self.assertIn("Commands must return None or an int exit code", stderr.getvalue()) + @unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") def test_run_app_rejects_equals_form_long_option_values(self) -> None: app = base_cli.App(name="space-options", log_to_file=False) diff --git a/tests/test_app_runtime_boundary.py b/tests/test_app_runtime_boundary.py index a78d6bc..ffb2c53 100644 --- a/tests/test_app_runtime_boundary.py +++ b/tests/test_app_runtime_boundary.py @@ -2,6 +2,8 @@ import importlib import importlib.util +import os +import tempfile from pathlib import Path import base_cli @@ -64,3 +66,21 @@ def test_runtime_directory_helpers_are_split_from_app_module() -> None: assert "def runtime_layout" not in app_source assert "def create_runtime_directory" not in app_source assert "def prune_log_files" not in app_source + + +def test_runtime_directories_are_owner_only_even_with_permissive_umask() -> None: + if os.name == "nt": + return + runtime = importlib.import_module("base_cli._runtime") + with tempfile.TemporaryDirectory() as tmpdir: + original_umask = os.umask(0o022) + try: + path = Path(tmpdir) / "runtime" / "nested" + runtime.create_runtime_directory(path, Path(tmpdir)) + path.chmod(0o755) + runtime.create_runtime_directory(path, Path(tmpdir)) + finally: + os.umask(original_umask) + + assert path.stat().st_mode & 0o777 == 0o700 + assert path.parent.stat().st_mode & 0o777 == 0o700 diff --git a/tests/test_command_protocol.py b/tests/test_command_protocol.py index 3b5b1ce..f8c330f 100644 --- a/tests/test_command_protocol.py +++ b/tests/test_command_protocol.py @@ -3,11 +3,15 @@ import unittest from unittest.mock import patch +from base_cli.command_protocol import BOOLEAN from base_cli.command_protocol import CommandProtocolError +from base_cli.command_protocol import NULLABLE_STRING +from base_cli.command_protocol import STRING from base_cli.command_protocol import dumps_record from base_cli.command_protocol import dumps_records from base_cli.command_protocol import loads_records from base_cli.command_protocol import RECORD_SCHEMAS +from base_cli.command_protocol import register_record_schema def project_command_record(**overrides: object) -> dict[str, object]: @@ -26,6 +30,33 @@ def project_command_record(**overrides: object) -> dict[str, object]: class CommandProtocolTests(unittest.TestCase): + def test_downstream_code_can_register_a_framing_safe_record_schema(self) -> None: + record_type = "test-record" + self.addCleanup(RECORD_SCHEMAS.pop, record_type, None) + register_record_schema( + record_type, + { + "name": STRING, + "enabled": BOOLEAN, + "note": NULLABLE_STRING, + }, + ) + + payload = dumps_record(record_type, {"name": "demo", "enabled": True, "note": None}) + + self.assertEqual( + loads_records(payload, expected_record_type=record_type), + (record_type, ({"name": "demo", "enabled": True, "note": None},)), + ) + + def test_record_schema_registration_rejects_invalid_or_duplicate_schemas(self) -> None: + with self.assertRaisesRegex(CommandProtocolError, "already registered"): + register_record_schema("demo", {"name": STRING}) + with self.assertRaisesRegex(CommandProtocolError, "non-empty mapping"): + register_record_schema("custom", {}) + with self.assertRaisesRegex(CommandProtocolError, "field name"): + register_record_schema("custom", {"bad-name": STRING}) + def test_project_python_requirement_is_scoped_to_project_setup_route_records(self) -> None: self.assertIn("requires_project_python", RECORD_SCHEMAS["project-setup-route"]) for record_type in ("project-route", "project-command", "build-target", "demo"): diff --git a/tests/test_history.py b/tests/test_history.py index ee2b423..7ffe7e4 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -112,6 +112,33 @@ def test_write_history_record_appends_without_fcntl(self) -> None: self.assertEqual(records, [record]) self.assertEqual(history_mode, 0o600) + def test_windows_lock_fallback_uses_a_private_sidecar(self) -> None: + record = { + "schema_version": 1, + "event": "finished", + "run_id": "run-windows", + "command": "check", + "status": "ok", + "exit_code": 0, + } + + with tempfile.TemporaryDirectory() as tmpdir: + cache_root = Path(tmpdir) / "cache" + fake_msvcrt = mock.Mock(LK_LOCK=1, LK_UNLCK=2) + with mock.patch.dict(os.environ, {"BASE_CACHE_DIR": str(cache_root)}): + with mock.patch("base_cli.history._fcntl", None), mock.patch( + "base_cli.history._msvcrt", fake_msvcrt + ): + history_helpers.write_history_record(record) + + history_path = cache_root / "base" / "history" / "runs.jsonl" + sidecar_path = history_path.with_name(f".{history_path.name}.lock") + self.assertEqual(read_history_records(cache_root), [record]) + self.assertEqual(sidecar_path.read_text(encoding="utf-8"), "0") + fake_msvcrt.locking.assert_has_calls( + [mock.call(mock.ANY, fake_msvcrt.LK_LOCK, 1), mock.call(mock.ANY, fake_msvcrt.LK_UNLCK, 1)] + ) + def test_write_primary_record_preserves_user_command_and_project_metadata(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: cache_root = Path(tmpdir) / "cache" diff --git a/tests/test_logging.py b/tests/test_logging.py index b42833b..92ef375 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -62,7 +62,7 @@ def test_configure_logger_honors_log_utc(self) -> None: def test_configure_logger_colors_python_user_stream_when_requested(self) -> None: stream = self._TtyStream() - with mock.patch.dict(os.environ, {"BASE_CLI_COLOR": "1"}, clear=True): + with mock.patch.dict(os.environ, {}, clear=True): logger = base_cli.configure_logger("color-stream", None, debug=False, stream=stream) logger.info("hello color") @@ -96,6 +96,15 @@ def test_configure_logger_honors_no_color_for_requested_user_stream(self) -> Non self.assertNotIn("\033[", stream.getvalue()) self.assertIn("hello plain", stream.getvalue()) + def test_configure_logger_honors_explicit_color_disable(self) -> None: + stream = self._TtyStream() + + with mock.patch.dict(os.environ, {"BASE_CLI_COLOR": "0"}, clear=True): + logger = base_cli.configure_logger("explicit-no-color-stream", None, debug=False, stream=stream) + logger.info("hello plain") + + self.assertNotIn("\033[", stream.getvalue()) + def test_configure_logger_uses_custom_formatter_for_file_handler(self) -> None: formatter = logging.Formatter("%(levelname)s:%(message)s") user_stream = io.StringIO() diff --git a/tests/test_public_api.py b/tests/test_public_api.py index acdca96..975f32b 100644 --- a/tests/test_public_api.py +++ b/tests/test_public_api.py @@ -1,6 +1,9 @@ from __future__ import annotations +import tempfile import unittest +from pathlib import Path +from unittest import mock import base_cli from base_cli import command_filters, command_protocol, history @@ -14,6 +17,19 @@ def test_version_matches_repository_contract(self) -> None: version_file = Path(__file__).resolve().parents[1] / "VERSION" self.assertEqual(base_cli.__version__, version_file.read_text(encoding="utf-8").splitlines()[0].strip()) + def test_version_resolution_ignores_unrelated_ancestor_version_files(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + checkout = root / "project" / "checkout" + package_file = checkout / "lib" / "python" / "base_cli" / "__init__.py" + (root / "VERSION").write_text("unrelated\n", encoding="utf-8") + package_file.parent.mkdir(parents=True) + (checkout / "pyproject.toml").write_text("[project]\nname = 'other'\n", encoding="utf-8") + + with mock.patch.object(base_cli, "__file__", str(package_file)): + with mock.patch.object(base_cli, "distribution_version", return_value="installed"): + self.assertEqual(base_cli._resolve_version(), "installed") + def test_facade_exports_supported_modules_functions_and_types(self) -> None: expected = { "CommandProtocolError", @@ -49,7 +65,17 @@ def test_module_all_surfaces_are_explicit(self) -> None: ) self.assertEqual( set(command_protocol.__all__), - {"CommandProtocolError", "dumps_record", "dumps_records", "loads_records"}, + { + "BOOLEAN", + "CommandProtocolError", + "FieldSpec", + "NULLABLE_STRING", + "STRING", + "dumps_record", + "dumps_records", + "loads_records", + "register_record_schema", + }, ) self.assertIn("write_primary_record", history.__all__) self.assertNotIn("lock_history_file", history.__all__) diff --git a/tests/test_testing.py b/tests/test_testing.py index 099a8aa..b5f7a7a 100644 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -8,8 +8,6 @@ import tempfile import unittest from pathlib import Path -from unittest import mock - import base_cli from base_cli.testing import invoke @@ -84,14 +82,16 @@ def test_invoke_rejects_manifest_without_cwd(self) -> None: with self.assertRaisesRegex(ValueError, "manifest requires cwd"): invoke(app, [], manifest={"project": {"name": "demo"}}) - def test_invoke_with_cwd_does_not_mutate_process_cwd(self) -> None: + def test_invoke_with_cwd_exposes_process_cwd_and_restores_it(self) -> None: app = base_cli.App(name="testing-cwd-isolation", log_to_file=False) - seen: dict[str, Path | None] = {} + seen: dict[str, Path | None | str] = {} @app.command() def main(ctx: base_cli.Context) -> None: seen["project_root"] = ctx.project_root seen["manifest_path"] = ctx.manifest_path + seen["cwd"] = str(Path.cwd()) + seen["relative_content"] = Path("relative.txt").read_text(encoding="utf-8") with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) @@ -101,14 +101,16 @@ def main(ctx: base_cli.Context) -> None: project.mkdir() manifest_path = project / "base_manifest.yaml" manifest_path.write_text("project:\n name: demo\n", encoding="utf-8") + (project / "relative.txt").write_text("cwd works\n", encoding="utf-8") original_cwd = Path.cwd() - with mock.patch("os.chdir", side_effect=AssertionError("process-global cwd mutation")): - result = invoke(app, [], home=home, cwd=project) + result = invoke(app, [], home=home, cwd=project) self.assertEqual(result.exit_code, 0, result.output) self.assertEqual(seen["project_root"], project.resolve()) self.assertEqual(seen["manifest_path"], manifest_path.resolve()) + self.assertEqual(seen["cwd"], str(project.resolve())) + self.assertEqual(seen["relative_content"], "cwd works\n") self.assertEqual(Path.cwd(), original_cwd) def test_invoke_with_cwd_without_manifest_preserves_no_manifest_behavior(self) -> None: