refactor(cli): collapse duplicate dest-check helpers

Code-review followup on #148:
- Drop `check_dest_safe`: it raised the same error as `_require_empty_dest`
  with hand-synced wording. Move the validation into `create_workspace`
  (which previously didn't validate at all — only `create_host` did).
- Unify parameter name across the call chain: `tolerate_existing` /
  `allowed` / `preserve_existing` all collapsed to `preserve_existing`.
- `app_project.py` no longer pre-checks the target — `create_workspace`
  (workspace mode) or `create_host` (flat mode) raises immediately if
  anything unexpected is pre-existing.
- `new.py` uses `Path.is_relative_to` instead of try/except.
- Drop narrative inline comments from the new regression tests; add an
  assertion that `.git/` survives instead of merely setting up `.git/HEAD`.

https://claude.ai/code/session_01K84RjsX1ToXorMyuaxNBT6

Author: claude

framework/cli/simple_module_cli/app_project.py

@@ -29,7 +29,6 @@
 from simple_module_cli.scaffolding import (
     SAFE_PRESERVED_NAMES,
     _module_to_pypi_name,
-    check_dest_safe,
     create_host,
     create_module,
     create_workspace,
@@ -105,29 +104,26 @@ def create_app_project(
     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 pre-existing entries at ``target`` (see ``check_dest_safe``)
-    and returns paths whose scaffold copy was skipped so callers can warn.
+    Tolerates ``SAFE_PRESERVED_NAMES`` at ``target`` and returns the paths
+    whose scaffold copy was skipped so callers can warn.
     """
-    check_dest_safe(target)
-
     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)
         preserved.extend(
-            create_workspace(target, name=name, tolerate_existing=SAFE_PRESERVED_NAMES)
+            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,
-            tolerate_existing=SAFE_PRESERVED_NAMES if flat else frozenset(),
+            preserve_existing=SAFE_PRESERVED_NAMES if flat else frozenset(),
         )
     )
     if not flat:

framework/cli/simple_module_cli/new.py

@@ -120,10 +120,7 @@ def new_project(
             "merge by hand if you want their contents):"
         )
         for path in preserved:
-            try:
-                rel = path.relative_to(target)
-            except ValueError:
-                rel = path
+            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}")

framework/cli/simple_module_cli/scaffolding.py

@@ -25,7 +25,6 @@
 
 __all__ = [
     "SAFE_PRESERVED_NAMES",
-    "check_dest_safe",
     "create_host",
     "create_module",
     "create_workspace",
@@ -36,10 +35,8 @@
 _TEMPLATES_PACKAGE = "simple_module_cli.templates"
 _PACKAGE_PATH_TOKEN = "__PACKAGE__"
 
-# Top-level entries we tolerate at the scaffold target — typical leftovers
-# from ``git init`` / ``gh repo create`` / IDE setup. The scaffold's copy is
-# skipped (the user's file wins) and the preserved path is surfaced so users
-# can merge content in by hand if they care.
+# 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"}
@@ -49,19 +46,6 @@
 )
 
 
-def check_dest_safe(target: Path) -> None:
-    """Refuse a non-empty target unless every entry is in ``SAFE_PRESERVED_NAMES``."""
-    if not target.exists():
-        return
-    unexpected = sorted(p.name for p in target.iterdir() if p.name not in SAFE_PRESERVED_NAMES)
-    if unexpected:
-        raise FileExistsError(
-            f"Destination {target} exists and contains files that would collide "
-            f"with the scaffold: {', '.join(unexpected)}. "
-            "Move them aside or choose another path."
-        )
-
-
 def _module_to_pypi_name(name: str) -> str:
     return f"simple_module_{name.lower()}"
 
@@ -76,15 +60,15 @@ def _iter_template_files(template_root: Path):
         yield path
 
 
-def _require_empty_dest(dest: Path, *, allowed: frozenset[str] = frozenset()) -> None:
+def _require_empty_dest(dest: Path, *, preserve_existing: frozenset[str] = frozenset()) -> None:
     """Refuse a non-empty destination unless every top-level entry is allowed.
 
-    ``allowed`` 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.
+    ``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 allowed)
+        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 "
@@ -108,12 +92,7 @@ def _apply_template_files(
     path_rewrites: Mapping[str, str] | None = None,
     preserve_existing: frozenset[str] = frozenset(),
 ) -> list[Path]:
-    """Write template files into ``dest``; return paths that were preserved.
-
-    When a top-level path segment matches ``preserve_existing`` *and* the target
-    file already exists, the scaffold's copy is skipped and the path is
-    returned so the caller can warn the user.
-    """
+    """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))
@@ -141,26 +120,27 @@ def create_workspace(
     name: str,
     template_root: Path | None = None,
     *,
-    tolerate_existing: frozenset[str] = frozenset(),
+    preserve_existing: frozenset[str] = frozenset(),
 ) -> list[Path]:
-    """Materialize the workspace-root shell at ``dest``.
+    """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.
 
-    ``tolerate_existing`` lists top-level entry names (``.git``, ``README.md``,
+    ``preserve_existing`` lists top-level entry names (``.git``, ``README.md``,
     ...) that may already exist in ``dest``; the scaffold's copy is skipped and
-    the preserved path is included in the returned list.
+    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)
+    _require_empty_dest(dest, preserve_existing=preserve_existing)
     preserved = _apply_template_files(
         _resolve_template_root("workspace", template_root),
         dest,
         {"{{HOST_NAME}}": to_kebab_case(name)},
-        preserve_existing=tolerate_existing,
+        preserve_existing=preserve_existing,
     )
     logger.info("Scaffolded workspace root at %s", dest)
     return preserved
@@ -173,17 +153,14 @@ def create_host(
     template_root: Path | None = None,
     framework_version: str = "*",
     *,
-    tolerate_existing: frozenset[str] = frozenset(),
+    preserve_existing: frozenset[str] = frozenset(),
 ) -> list[Path]:
     """Scaffold a host project at ``dest``; return preserved pre-existing paths.
 
-    ``tolerate_existing`` lists top-level entry names (``.git``, ``README.md``,
-    ...) that may already exist in ``dest``. Their scaffold counterparts are
-    skipped and returned so the caller can surface a notice. Anything else
-    pre-existing raises ``FileExistsError``.
+    ``preserve_existing`` semantics match :func:`create_workspace`.
     """
     dest = Path(dest)
-    _require_empty_dest(dest, allowed=tolerate_existing)
+    _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)
     preserved = _apply_template_files(
         _resolve_template_root("host", template_root),
@@ -193,7 +170,7 @@ def create_host(
             "{{MODULE_DEPS}}": module_dep_lines,
             "{{FRAMEWORK_VERSION}}": framework_version,
         },
-        preserve_existing=tolerate_existing,
+        preserve_existing=preserve_existing,
     )
     logger.info(
         "Scaffolded host '%s' at %s (modules: %s)", name, dest, ", ".join(modules) or "<none>"

framework/cli/tests/test_cli_new_regressions.py

@@ -128,7 +128,6 @@ def test_sm_new_tolerates_git_init_leftovers_in_dest(tmp_path: Path) -> None:
     target = tmp_path / "demo"
     target.mkdir()
     (target / ".git").mkdir()
-    (target / ".git" / "HEAD").write_text("ref: refs/heads/main\n")
     user_gitignore = "*.pyc\n"
     (target / ".gitignore").write_text(user_gitignore)
     user_readme = "# demo (user-authored)\n"
@@ -141,14 +140,12 @@ def test_sm_new_tolerates_git_init_leftovers_in_dest(tmp_path: Path) -> None:
     )
 
     assert result.exit_code == 0, result.output
-    # User-authored files survived unchanged.
+    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"
-    # Scaffold's own files landed alongside them.
     assert (target / "host" / "pyproject.toml").is_file()
     assert (target / "modules" / "hello").is_dir()
-    # CLI surfaced the preservation notice so users can merge by hand.
     assert "Preserved existing files" in result.output
     assert ".gitignore" in result.output
     assert "README.md" in result.output
@@ -201,5 +198,4 @@ def test_sm_new_still_refuses_unrelated_files_in_dest(tmp_path: Path) -> None:
     assert result.exit_code != 0
     output = result.output + (result.stderr or "")
     assert "my_notes.md" in output
-    # User's file is intact.
     assert (target / "my_notes.md").read_text() == "don't clobber me"