[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config

src/specify_cli/extensions/__init__.py

@@ -9,11 +9,13 @@
 from __future__ import annotations
 
 import copy
+import errno
 import hashlib
 import json
 import os
 import re
 import shutil
+import stat
 import tempfile
 import zipfile
 from dataclasses import dataclass
@@ -101,6 +103,51 @@ def _load_core_command_names() -> frozenset[str]:
 CORE_COMMAND_NAMES = _load_core_command_names()
 
 
+def _fsync_fd(fd: int) -> None:
+    """Sync a file descriptor, raising on real storage errors."""
+    try:
+        os.fsync(fd)
+    except AttributeError:
+        return
+    except NotImplementedError:
+        return
+    except OSError as exc:
+        if exc.errno in {errno.ENOTSUP, errno.EOPNOTSUPP, errno.EINVAL, errno.EBADF}:
+            return
+        raise
+
+
+def _fsync_directory(path: Path) -> None:
+    """Sync a directory when the platform supports it."""
+    if not path.exists():
+        return
+    if os.name == "nt":
+        return
+    try:
+        dir_fd = os.open(str(path), os.O_RDONLY | getattr(os, "O_DIRECTORY", 0))
+    except (AttributeError, NotImplementedError):
+        return
+    except OSError as exc:
+        if exc.errno in {errno.ENOTSUP, errno.EOPNOTSUPP, errno.EINVAL, errno.EBADF}:
+            return
+        try:
+            dir_fd = os.open(str(path), os.O_RDONLY)
+        except (AttributeError, NotImplementedError):
+            return
+        except OSError as exc2:
+            if exc2.errno in {errno.ENOTSUP, errno.EOPNOTSUPP, errno.EINVAL, errno.EBADF}:
+                return
+            raise
+    try:
+        _fsync_fd(dir_fd)
+    finally:
+        try:
+            os.close(dir_fd)
+        except OSError:
+            # Cleanup after an fsync failure should not mask the original error.
+            pass
+
+
 class ExtensionError(Exception):
     """Base exception for extension-related errors."""
 
@@ -699,6 +746,20 @@ def __init__(self, project_root: Path):
         self.extensions_dir = project_root / ".specify" / "extensions"
         self.registry = ExtensionRegistry(self.extensions_dir)
 
+    def _rescue_staging_dir(self, extension_id: str) -> Path:
+        """Fixed-length staging directory path for a preserved-config rescue.
+
+        The extension ID can be arbitrarily long (manifest validation caps only
+        the character set, not the length), so embedding it verbatim in a single
+        path component could push the ``.rescue-staging-<id>`` directory past a
+        filesystem's per-component byte limit and make every reinstall after
+        ``--keep-config`` fail with ``ENAMETOOLONG`` even though the extension
+        installs fine at ``dest_dir``. Hash the ID to a fixed-length suffix so
+        the component length is bounded regardless of ID length.
+        """
+        digest = hashlib.sha256(extension_id.encode("utf-8")).hexdigest()[:16]
+        return self.extensions_dir / f".rescue-staging-{digest}"
+
     @staticmethod
     def _collect_manifest_command_names(manifest: ExtensionManifest) -> Dict[str, str]:
         """Collect command and alias names declared by a manifest.
@@ -1402,12 +1463,259 @@ def install_from_directory(
                 backup_config_dir.unlink()
             did_remove = self.remove(manifest.id)
 
+        # Load and validate .extensionignore BEFORE reading/creating the rescue
+        # staging directory (and thus before deleting dest_dir). The loader can
+        # raise ValidationError (invalid UTF-8) or OSError; doing it first means
+        # such a failure aborts while the kept config is still authoritative in
+        # its documented location, rather than leaving a freshly published
+        # staging copy that a later retry (after the user edits the kept config)
+        # would reload and use to overwrite the newer bytes. Any staging left by
+        # an earlier destructive attempt is intentionally left intact here.
+        ignore_fn = self._load_extensionignore(source_dir)
+
+        # Rescue any config files left behind by a prior `remove --keep-config`.
+        # When an extension is removed with --keep-config, it is no longer in
+        # the registry but its config files remain in dest_dir.  A subsequent
+        # plain (non-force) install would delete that directory unconditionally,
+        # silently discarding the preserved config.  We read those files into
+        # memory and also write a durable staging copy outside dest_dir so
+        # that a partial rmtree, failed copytree, or partial restore cannot
+        # permanently discard the user's original bytes on a retry.  The
+        # staging dir is removed only after every config has been successfully
+        # restored.
+        stranded_configs: dict[str, tuple[bytes, int]] = {}
+        rescue_staging_dir = self._rescue_staging_dir(manifest.id)
+        # A staging directory is trusted only when this completion marker is
+        # present.  The marker is written after every staged file is complete
+        # and removed before the non-atomic cleanup, so a crash mid-staging or
+        # mid-cleanup can never leave a partial directory that a retry mistakes
+        # for a complete durable backup.
+        rescue_complete_marker = rescue_staging_dir / ".rescue-complete"
+        staging_is_complete = (
+            rescue_staging_dir.is_dir()
+            and not rescue_staging_dir.is_symlink()
+            and rescue_complete_marker.is_file()
+            and not rescue_complete_marker.is_symlink()
+        )
+
+        if staging_is_complete and not self.registry.is_installed(manifest.id):
+            # A previous install attempt staged the configs but never
+            # completed cleanly.  Reload from the durable backup so the
+            # original bytes are used on retry rather than whatever
+            # mixture of packaged defaults and partial restores remains
+            # on disk.  Only load non-symlinked files whose names match
+            # the two recognised config suffixes so a tampered staging
+            # directory cannot inject arbitrary files.
+            #
+            # A complete staging directory proves only that staging finished,
+            # not that dest_dir was ever modified: a crash after staging was
+            # synced but before the rmtree below leaves the live kept config
+            # intact. If the user then edits that live config before retrying,
+            # blindly preferring the staged bytes would silently overwrite the
+            # newer config. The staged and live copies are indistinguishable
+            # in provenance from disk alone (a genuine post-crash edit vs. a
+            # packaged default written by a partially-completed copytree), so
+            # when a live config disagrees with its staged copy we must not
+            # silently pick either — preserve both and abort, letting the user
+            # resolve it. dest_dir is still untouched here, so raising is safe.
+            conflicting: list[str] = []
+            for staged_file in sorted(rescue_staging_dir.iterdir()):
+                if (
+                    staged_file.is_file()
+                    and not staged_file.is_symlink()
+                    and staged_file.name.endswith(("-config.yml", "-config.local.yml"))
+                ):
+                    staged_bytes = staged_file.read_bytes()
+                    live_file = dest_dir / staged_file.name
+                    if live_file.is_file() and not live_file.is_symlink():
+                        try:
+                            live_bytes = live_file.read_bytes()
+                        except OSError:
+                            live_bytes = None
+                        if live_bytes is not None and live_bytes != staged_bytes:
+                            conflicting.append(staged_file.name)
+                    stranded_configs[staged_file.name] = (
+                        staged_bytes,
+                        staged_file.stat().st_mode,
+                    )
+            if conflicting:
+                names = ", ".join(sorted(conflicting))
+                raise ValidationError(
+                    "Preserved extension config conflict for "
+                    f"'{manifest.id}': the current config file(s) ({names}) in "
+                    f"{dest_dir} differ from the rescued backup left by an "
+                    f"interrupted install in {rescue_staging_dir}. Both copies "
+                    "have been preserved. Resolve manually — keep the current "
+                    f"file and delete {rescue_staging_dir}, or restore the "
+                    "backup over the current file — then reinstall."
+                )
+        elif dest_dir.exists() and not self.registry.is_installed(manifest.id):
+            for cfg_file in (
+                list(dest_dir.glob("*-config.yml"))
+                + list(dest_dir.glob("*-config.local.yml"))
+            ):
+                if cfg_file.is_file() and not cfg_file.is_symlink():
+                    stranded_configs[cfg_file.name] = (
+                        cfg_file.read_bytes(),
+                        cfg_file.stat().st_mode,
+                    )
+
+        if stranded_configs and not staging_is_complete:
+            # Write a durable backup outside dest_dir before any
+            # destructive operation so the original bytes survive a
+            # crash or partial failure at any later step.  The staging
+            # dir is cleaned up only after every restore succeeds.
+            #
+            # Any pre-existing staging dir here lacks the completion marker
+            # (staging_is_complete is False), so it is a stale partial from an
+            # interrupted attempt — remove it first for a clean write.
+            if rescue_staging_dir.is_symlink():
+                rescue_staging_dir.unlink()
+            elif rescue_staging_dir.is_dir():
+                shutil.rmtree(rescue_staging_dir)
+            elif rescue_staging_dir.exists():
+                rescue_staging_dir.unlink()
+            try:
+                rescue_staging_dir.mkdir(parents=True, exist_ok=True)
+                for filename, (content, mode) in stranded_configs.items():
+                    staged = rescue_staging_dir / filename
+                    # Create the staging file with mode 0600 before writing so
+                    # the preserved bytes are never transiently readable by other
+                    # local users, even on a umask that would produce 0644.
+                    # O_BINARY (0 on POSIX) is required so Windows does not open
+                    # the descriptor in text mode and translate the preserved
+                    # bytes' "\n" into "\r\n" as they are written.
+                    fd = os.open(
+                        str(staged),
+                        os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_BINARY", 0),
+                        0o600,
+                    )
+                    try:
+                        # os.write() may write fewer bytes than requested, so
+                        # loop until the whole buffer is on disk — a truncated
+                        # "durable" backup would be trusted over the intact
+                        # config on a retry and cause silent data loss.
+                        view = memoryview(content)
+                        written = 0
+                        while written < len(view):
+                            written += os.write(fd, view[written:])
+                        try:
+                            os.fchmod(fd, stat.S_IMODE(mode))
+                        except (AttributeError, NotImplementedError, OSError):
+                            try:
+                                staged.chmod(stat.S_IMODE(mode))
+                            except (NotImplementedError, OSError):
+                                pass  # Best-effort; chmod may not be supported on all platforms.
+                        _fsync_fd(fd)
+                    finally:
+                        os.close(fd)
+                # Flush the staging directory metadata before publishing the
+                # completion marker so a crash cannot leave a visible marker with
+                # only a subset of staged files.
+                _fsync_directory(rescue_staging_dir)
+                # Write the completion marker only after every staged file is
+                # fully written so a retry trusts staging only when it is whole.
+                marker_fd = os.open(
+                    str(rescue_complete_marker),
+                    os.O_WRONLY | os.O_CREAT | os.O_EXCL,
+                    0o600,
+                )
+                try:
+                    _fsync_fd(marker_fd)
+                finally:
+                    os.close(marker_fd)
+                _fsync_directory(rescue_staging_dir)
+                _fsync_directory(rescue_staging_dir.parent)
+            except BaseException:
+                # Durable staging failed (or was interrupted).  Continuing with
+                # only the in-memory copy would reintroduce the permanent-loss
+                # path this staging exists to close: the rmtree below could
+                # delete the originals and a later restore failure would leave
+                # no on-disk copy.  dest_dir is still untouched here, so clean
+                # up the partial staging dir and abort the install instead of
+                # proceeding destructively.
+                shutil.rmtree(rescue_staging_dir, ignore_errors=True)
+                raise
+
         # Install extension (dest_dir computed above during self-install guard)
         if dest_dir.exists():
             shutil.rmtree(dest_dir)
 
-        ignore_fn = self._load_extensionignore(source_dir)
-        shutil.copytree(source_dir, dest_dir, ignore=ignore_fn)
+        def _restore_stranded_config_file(
+            target: Path, content: bytes, preserved_mode: int
+        ) -> None:
+            tmp_path: Path | None = None
+            try:
+                # A short fixed prefix, not f".{target.name}.": the preserved
+                # config filename may itself already be near the filesystem's
+                # per-component byte limit, and NamedTemporaryFile appends a
+                # random suffix to the prefix — reusing the full name would push
+                # the temp file past the limit and raise ENAMETOOLONG on every
+                # retry. tempfile already guarantees collision avoidance.
+                with tempfile.NamedTemporaryFile(
+                    mode="wb",
+                    dir=target.parent,
+                    prefix=".cfg-restore.",
+                    delete=False,
+                ) as tmp:
+                    tmp_path = Path(tmp.name)
+                    tmp.write(content)
+                    tmp.flush()
+                    _fsync_fd(tmp.fileno())
+                try:
+                    tmp_path.chmod(stat.S_IMODE(preserved_mode))
+                except (NotImplementedError, OSError):
+                    pass  # Best-effort; chmod may not be supported on all platforms.
+                os.replace(tmp_path, target)
+                try:
+                    target_fd = os.open(str(target), os.O_RDONLY)
+                except (AttributeError, OSError, NotImplementedError):
+                    target_fd = None
+                try:
+                    if target_fd is not None:
+                        _fsync_fd(target_fd)
+                finally:
+                    if target_fd is not None:
+                        try:
+                            os.close(target_fd)
+                        except OSError:
+                            pass
+                _fsync_directory(target.parent)
+            except BaseException:
+                if tmp_path is not None and tmp_path.exists():
+                    tmp_path.unlink()
+                raise
+
+        try:
+            shutil.copytree(source_dir, dest_dir, ignore=ignore_fn)
+        except BaseException:
+            # copytree failed — dest_dir may be absent or only partially
+            # created.  Write the rescued configs back now so they are not
+            # permanently lost even though the install did not complete.
+            if stranded_configs:
+                dest_dir.mkdir(parents=True, exist_ok=True)
+                for filename, (content, mode) in stranded_configs.items():
+                    target = dest_dir / filename
+                    _restore_stranded_config_file(target, content, mode)
+            raise
+
+        # Restore stranded configs rescued before the rmtree above.
+        for filename, (content, mode) in stranded_configs.items():
+            target = dest_dir / filename
+            _restore_stranded_config_file(target, content, mode)
+
+        # Every stranded config has been restored successfully — the
+        # durable staging backup is no longer needed.  Raise on failure so
+        # the install is not reported as successful while a stale backup
+        # that could be misread on the next retry remains on disk.
+        if rescue_staging_dir.is_dir() and not rescue_staging_dir.is_symlink():
+            # Remove the completion marker before the non-atomic rmtree so a
+            # crash mid-cleanup cannot leave a staging dir that a retry would
+            # wrongly trust as a complete durable backup.
+            rescue_complete_marker.unlink(missing_ok=True)
+            _fsync_directory(rescue_staging_dir)
+            shutil.rmtree(rescue_staging_dir)
+            _fsync_directory(rescue_staging_dir.parent)
 
         # Register commands with AI agents
         registered_commands = {}

src/specify_cli/workflows/_commands.py

@@ -1666,8 +1666,8 @@ def _validate_and_install_local(
                 except OSError as cleanup_exc:
                     console.print(
                         "[yellow]Warning:[/yellow] Could not remove temporary "
-                        f"download file {_escape_markup(str(tmp_path))}: "
-                        f"{_escape_markup(str(cleanup_exc))}"
+                        f"workflow download file: {_escape_markup(str(cleanup_exc))} "
+                        f"(path: {_escape_markup(str(tmp_path))})"
                     )
             console.print(f"[red]Error:[/red] Failed to download workflow: {_escape_markup(str(exc))}")
             raise typer.Exit(1)
@@ -1691,8 +1691,8 @@ def _validate_and_install_local(
             except OSError as exc:
                 console.print(
                     "[yellow]Warning:[/yellow] Could not remove temporary "
-                    f"download file {_escape_markup(str(tmp_path))}: "
-                    f"{_escape_markup(str(exc))}"
+                    f"workflow download file: {_escape_markup(str(exc))} "
+                    f"(path: {_escape_markup(str(tmp_path))})"
                 )
         return
 

tests/extensions/test_update_agent_context_feature_json.py

@@ -11,7 +11,6 @@
 
 from tests.conftest import requires_bash
 from tests.extensions.test_extension_agent_context import (
-    BASH,
     POWERSHELL,
     _bash_posix_path,
     _run_bash_agent_context_script,