From 7c0fa43a98149cc222ffd8a5609d973fad0e1f6e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 16:49:54 +0000 Subject: [PATCH] fix(cli): tolerate safe leftovers in smpy new dest (#148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `smpy new --dest .` was refusing any non-empty destination, so the common `gh repo create myapp && cd myapp && smpy new myapp --dest .` flow blew up the moment `.git/`, `.gitignore`, or `README.md` was present. Closes #148. Scaffolding now tolerates a small allowlist (`SAFE_PRESERVED_NAMES`: `.git/`, `.gitignore`, `.gitattributes`, `README*`, `LICENSE*`, `COPYING`, `CHANGELOG.md`, `CONTRIBUTING.md`, `CODE_OF_CONDUCT.md`, `.claude/`, `.vscode/`, `.idea/`, `.editorconfig`, `.DS_Store`) at the scaffold target. When the scaffold would write over a tolerated pre-existing file, the scaffold's copy is skipped — the user's file wins. Any other pre-existing entry still raises `FileExistsError`, naming the colliding files so the user knows what to move aside. `create_workspace` / `create_host` accept a `preserve_existing` set and return the list of preserved paths; `create_app_project` returns `(host_dir, preserved)`; `new.py` surfaces a `Preserved existing files` notice listing what was skipped so users can merge content in by hand. Three regression tests in test_cli_new_dest_tolerance.py cover: workspace mode tolerates a fresh `git init` layout (and user files survive), flat mode does the same, and unrelated files still hard-fail without being touched. https://claude.ai/code/session_01K84RjsX1ToXorMyuaxNBT6 --- .../cli/simple_module_cli/app_project.py | 48 +++++----- framework/cli/simple_module_cli/new.py | 10 +- .../cli/simple_module_cli/scaffolding.py | 81 ++++++++++++---- .../cli/tests/test_cli_new_dest_tolerance.py | 96 +++++++++++++++++++ 4 files changed, 194 insertions(+), 41 deletions(-) create mode 100644 framework/cli/tests/test_cli_new_dest_tolerance.py diff --git a/framework/cli/simple_module_cli/app_project.py b/framework/cli/simple_module_cli/app_project.py index 2c874f95..52192082 100644 --- a/framework/cli/simple_module_cli/app_project.py +++ b/framework/cli/simple_module_cli/app_project.py @@ -27,6 +27,7 @@ from simple_module_cli.catalog import CATALOG, PRESETS, expand_deps from simple_module_cli.recipes import RECIPES, ScaffoldCtx from simple_module_cli.scaffolding import ( + SAFE_PRESERVED_NAMES, _module_to_pypi_name, create_host, create_module, @@ -97,38 +98,39 @@ def create_app_project( tenancy: bool = False, selected: Sequence[str] | None = None, flat: bool = False, -) -> Path: +) -> tuple[Path, list[Path]]: """Greenfield ``simple-module new`` scaffold. - In workspace mode (the default), lays down a uv + npm workspace at - ``target/`` with the host under ``target/host/`` and a sample module - under ``target/modules/hello/``. In flat mode (``flat=True``), keeps - the legacy single-host layout: host files at ``target/`` with no - ``modules/`` directory or workspace plumbing. + Workspace mode (default) lays down a uv + npm workspace at ``target/`` + with the host under ``target/host/`` and a sample module under + ``target/modules/hello/``. Flat mode keeps the legacy single-host layout. + Tolerates ``SAFE_PRESERVED_NAMES`` at ``target`` (leftovers from + ``git init`` / ``gh repo create`` / IDE setup); other pre-existing + entries raise ``FileExistsError``. - Generates a secret, picks a DB URL, rewrites the host's - ``pyproject.toml`` / the relevant ``package.json`` to pin exact - framework versions, and applies any matching post-scaffold recipes - (e.g. the ``background_tasks`` recipe drops a Celery worker stack). - - Returns the host directory — ``target`` in flat mode, ``target/host`` - in workspace mode. + Returns ``(host_dir, preserved)`` — the host directory (``target`` in + flat mode, ``target/host`` in workspace mode) and the paths whose + scaffold copy was skipped because the user already had one. """ - if target.exists() and any(target.iterdir()): - raise FileExistsError( - f"Destination {target} already exists and is non-empty; " - "choose a new path or remove its contents first." - ) - chosen = list(selected) if selected is not None else list(PRESETS["standard"]) resolved, _added = expand_deps(chosen) display_names = [to_pascal_case(CATALOG[m].display) for m in resolved] host_dir = target if flat else target / "host" + preserved: list[Path] = [] if not flat: - target.mkdir(parents=True, exist_ok=True) - create_workspace(target, name=name) - create_host(host_dir, name=name, modules=display_names, framework_version=_FRAMEWORK_VERSION) + preserved.extend( + create_workspace(target, name=name, preserve_existing=SAFE_PRESERVED_NAMES) + ) + preserved.extend( + create_host( + host_dir, + name=name, + modules=display_names, + framework_version=_FRAMEWORK_VERSION, + preserve_existing=SAFE_PRESERVED_NAMES if flat else frozenset(), + ) + ) if not flat: _strip_workspace_owned_files(host_dir) @@ -180,7 +182,7 @@ def create_app_project( if recipe_key is not None and recipe_key in RECIPES: RECIPES[recipe_key].apply(target, ctx) - return host_dir + return host_dir, preserved def _strip_workspace_owned_files(host_dir: Path) -> None: diff --git a/framework/cli/simple_module_cli/new.py b/framework/cli/simple_module_cli/new.py index 2c56c6d2..d230d0ac 100644 --- a/framework/cli/simple_module_cli/new.py +++ b/framework/cli/simple_module_cli/new.py @@ -111,7 +111,7 @@ def new_project( raise typer.Exit(code=1) from None try: - host_dir = create_app_project( + host_dir, preserved = create_app_project( target, name=name, db=db_final, @@ -125,6 +125,14 @@ def new_project( typer.echo(f"Created app '{name}' at {target}") typer.echo(f"Modules: {', '.join(resolved)}") + if preserved: + typer.echo( + "\nPreserved existing files (scaffold's versions were skipped — " + "merge by hand if you want their contents):" + ) + for path in preserved: + rel = path.relative_to(target) if path.is_relative_to(target) else path + typer.echo(f" {rel}") typer.echo("\nNext steps:") typer.echo(f" cd {target}") if no_install: diff --git a/framework/cli/simple_module_cli/scaffolding.py b/framework/cli/simple_module_cli/scaffolding.py index 17e27810..dbea7b57 100644 --- a/framework/cli/simple_module_cli/scaffolding.py +++ b/framework/cli/simple_module_cli/scaffolding.py @@ -28,13 +28,28 @@ validate_scaffold_name, ) -__all__ = ["create_host", "create_module", "create_workspace"] +__all__ = [ + "SAFE_PRESERVED_NAMES", + "create_host", + "create_module", + "create_workspace", +] logger = logging.getLogger(__name__) _TEMPLATES_PACKAGE = "simple_module_cli.templates" _PACKAGE_PATH_TOKEN = "__PACKAGE__" +# Pre-existing entries we tolerate at a scaffold target — typical leftovers +# from ``git init`` / ``gh repo create`` / IDE setup. +SAFE_PRESERVED_NAMES = frozenset( + {".git", ".gitignore", ".gitattributes", ".editorconfig", ".DS_Store"} + | {".claude", ".vscode", ".idea"} + | {"README", "README.md", "README.rst"} + | {"LICENSE", "LICENSE.md", "LICENSE.txt", "COPYING"} + | {"CHANGELOG.md", "CONTRIBUTING.md", "CODE_OF_CONDUCT.md"} +) + def _module_to_pypi_name(name: str) -> str: return f"simple_module_{name.lower()}" @@ -50,12 +65,21 @@ def _iter_template_files(template_root: Path): yield path -def _require_empty_dest(dest: Path) -> None: - if dest.exists() and any(dest.iterdir()): - raise FileExistsError( - f"Destination {dest} already exists and is non-empty. " - "Choose a new path or remove the contents first." - ) +def _require_empty_dest(dest: Path, *, preserve_existing: frozenset[str] = frozenset()) -> None: + """Refuse a non-empty destination unless every top-level entry is allowed. + + ``preserve_existing`` is matched against the *name* of each top-level entry, + so callers can permit common pre-existing files (``.git``, ``README.md``, + ...) without silently overwriting unrelated user content. + """ + if dest.exists(): + unexpected = sorted(p.name for p in dest.iterdir() if p.name not in preserve_existing) + if unexpected: + raise FileExistsError( + f"Destination {dest} exists and contains files that would collide " + f"with the scaffold: {', '.join(unexpected)}. " + "Move them aside or choose another path." + ) dest.mkdir(parents=True, exist_ok=True) @@ -71,13 +95,20 @@ def _apply_template_files( substitutions: Mapping[str, str], *, path_rewrites: Mapping[str, str] | None = None, -) -> None: + preserve_existing: frozenset[str] = frozenset(), +) -> list[Path]: + """Write template files into ``dest``; return paths skipped to preserve the user's copy.""" + preserved: list[Path] = [] for src in _iter_template_files(src_root): rel_str = str(src.relative_to(src_root)) for old, new in (path_rewrites or {}).items(): rel_str = rel_str.replace(old, new) rel_str = rel_str.removesuffix(".tpl") target = dest / rel_str + top = Path(rel_str).parts[0] if rel_str else "" + if top in preserve_existing and target.exists(): + preserved.append(target) + continue target.parent.mkdir(parents=True, exist_ok=True) if src.suffix == ".tpl": text = src.read_text(encoding="utf-8") @@ -86,32 +117,41 @@ def _apply_template_files( target.write_text(text, encoding="utf-8") else: shutil.copy2(src, target) + return preserved def create_workspace( dest: Path, name: str, template_root: Path | None = None, -) -> Path: - """Materialize the workspace-root shell at ``dest``. + *, + preserve_existing: frozenset[str] = frozenset(), +) -> list[Path]: + """Materialize the workspace-root shell at ``dest``; return preserved paths. Lays down the top-level ``pyproject.toml`` (uv workspace), ``package.json`` (npm workspace), ``Makefile`` (delegates to host), ``.env.example``, ``.gitignore``, and ``README.md``. Does NOT create the host or any modules — those go under ``dest/host`` and ``dest/modules/`` afterwards. + + ``preserve_existing`` lists top-level entry names that may already exist + in ``dest``; the scaffold's copy is skipped and the preserved path is + included in the returned list. Other pre-existing entries raise + ``FileExistsError``. """ dest = Path(dest) - dest.mkdir(parents=True, exist_ok=True) - _apply_template_files( + _require_empty_dest(dest, preserve_existing=preserve_existing) + preserved = _apply_template_files( _resolve_template_root("workspace", template_root), dest, { "{{HOST_NAME}}": validate_scaffold_name(name), "{{HOST_PYPI_NAME}}": to_kebab_case(name), }, + preserve_existing=preserve_existing, ) logger.info("Scaffolded workspace root at %s", dest) - return dest + return preserved def create_host( @@ -120,11 +160,17 @@ def create_host( modules: Sequence[str], template_root: Path | None = None, framework_version: str = "*", -) -> Path: + *, + preserve_existing: frozenset[str] = frozenset(), +) -> list[Path]: + """Scaffold a host project at ``dest``; return preserved pre-existing paths. + + ``preserve_existing`` semantics match :func:`create_workspace`. + """ dest = Path(dest) - _require_empty_dest(dest) + _require_empty_dest(dest, preserve_existing=preserve_existing) module_dep_lines = "\n".join(f' "{_module_to_pypi_name(m)}>=0.1,<1.0",' for m in modules) - _apply_template_files( + preserved = _apply_template_files( _resolve_template_root("host", template_root), dest, { @@ -133,11 +179,12 @@ def create_host( "{{MODULE_DEPS}}": module_dep_lines, "{{FRAMEWORK_VERSION}}": framework_version, }, + preserve_existing=preserve_existing, ) logger.info( "Scaffolded host '%s' at %s (modules: %s)", name, dest, ", ".join(modules) or "" ) - return dest + return preserved def create_module( diff --git a/framework/cli/tests/test_cli_new_dest_tolerance.py b/framework/cli/tests/test_cli_new_dest_tolerance.py new file mode 100644 index 00000000..8cbf5c70 --- /dev/null +++ b/framework/cli/tests/test_cli_new_dest_tolerance.py @@ -0,0 +1,96 @@ +"""Regression tests for ``smpy new`` against a non-empty destination. + +Issue #148: scaffolding into a directory that already contains a fresh +``git init`` (or ``gh repo create``) layout — ``.git/``, ``.gitignore``, +``README.md``, ``LICENSE`` — has to succeed without clobbering the user's +files. Arbitrary pre-existing entries must still hard-fail so we don't +overwrite real work. +""" + +from __future__ import annotations + +from pathlib import Path + +from simple_module_cli.cli import app +from typer.testing import CliRunner + + +def test_sm_new_tolerates_git_init_leftovers_in_dest(tmp_path: Path) -> None: + """``smpy new --dest .`` after ``git init`` / ``gh repo create`` must succeed, + even with ``.git/``, ``.gitignore``, ``README.md``, ``LICENSE`` pre-existing — + the scaffold preserves them and writes everything else.""" + runner = CliRunner() + target = tmp_path / "demo" + target.mkdir() + (target / ".git").mkdir() + user_gitignore = "*.pyc\n" + (target / ".gitignore").write_text(user_gitignore) + user_readme = "# demo (user-authored)\n" + (target / "README.md").write_text(user_readme) + (target / "LICENSE").write_text("MIT\n") + + result = runner.invoke( + app, + ["new", "demo", "--yes", "--db", "sqlite", "--no-install", "--dest", str(target)], + ) + + assert result.exit_code == 0, result.output + assert (target / ".git").is_dir() + assert (target / ".gitignore").read_text() == user_gitignore + assert (target / "README.md").read_text() == user_readme + assert (target / "LICENSE").read_text() == "MIT\n" + assert (target / "host" / "pyproject.toml").is_file() + assert (target / "modules" / "hello").is_dir() + assert "Preserved existing files" in result.output + assert ".gitignore" in result.output + assert "README.md" in result.output + + +def test_sm_new_flat_tolerates_git_init_leftovers(tmp_path: Path) -> None: + """Flat mode (host-at-target) must also tolerate the safe allowlist, + since `smpy new --flat --dest .` is the same flow against a fresh + `git init`.""" + runner = CliRunner() + target = tmp_path / "demo" + target.mkdir() + (target / ".git").mkdir() + (target / ".gitignore").write_text("# user\n") + + result = runner.invoke( + app, + [ + "new", + "demo", + "--yes", + "--flat", + "--db", + "sqlite", + "--no-install", + "--dest", + str(target), + ], + ) + + assert result.exit_code == 0, result.output + assert (target / ".gitignore").read_text() == "# user\n" + assert (target / "pyproject.toml").is_file() + + +def test_sm_new_still_refuses_unrelated_files_in_dest(tmp_path: Path) -> None: + """Tolerance is *narrow* — arbitrary pre-existing files (anything not in + the safe allowlist) must still be a hard error so users don't accidentally + scaffold over real work.""" + runner = CliRunner() + target = tmp_path / "demo" + target.mkdir() + (target / "my_notes.md").write_text("don't clobber me") + + result = runner.invoke( + app, + ["new", "demo", "--yes", "--db", "sqlite", "--no-install", "--dest", str(target)], + ) + + assert result.exit_code != 0 + output = result.output + (result.stderr or "") + assert "my_notes.md" in output + assert (target / "my_notes.md").read_text() == "don't clobber me"