diff --git a/docs/operations/install-binaries.md b/docs/operations/install-binaries.md index 3b1050c..da6ab60 100644 --- a/docs/operations/install-binaries.md +++ b/docs/operations/install-binaries.md @@ -147,10 +147,12 @@ The canonical install is the one this document describes: a launcher at `~/.local/bin/kickstart` (symlink or `#!/bin/sh` wrapper), owned by `kickstart install`, which leads to the active managed payload under `~/.local/share/kickstart`. `kickstart upgrade` refreshes that managed -installation. Treat the payload's internal path as an implementation detail -and use `kickstart install --check` to inspect it. Nothing else should shadow -the launcher. In particular, ad-hoc shims in personal `bin` directories (for -example +installation while reusing the same launcher and managed app root on every +run. A successful upgrade also collapses an accidentally nested payload left +by older upgrade logic back into that stable root. Treat the payload's +internal path as an implementation detail and use `kickstart install --check` +to inspect it. Nothing else should shadow the launcher. In particular, ad-hoc +shims in personal `bin` directories (for example `~/workspace/bin/kickstart`) are not supported: they bypass `kickstart upgrade`, go stale silently, and hide which binary owns the command. Delete them, or reduce them to a one-line `exec "$HOME/.local/bin/kickstart" "$@"` diff --git a/src/utils/installer.py b/src/utils/installer.py index 07aeca6..74eb175 100644 --- a/src/utils/installer.py +++ b/src/utils/installer.py @@ -64,17 +64,36 @@ class PathUpdateResult: on_path_now: bool +def current_entrypoint_path() -> Path: + """Return the absolute invocation path without resolving symlinks. + + Preserving the logical path matters for managed installs: the launcher + symlink identifies both its public directory and the managed app root. A + bare command name is resolved through ``PATH`` so callers still receive an + absolute path. + """ + raw_argv0 = sys.argv[0] if sys.argv and sys.argv[0] else None + if raw_argv0 is not None: + argv0 = Path(raw_argv0).expanduser() + candidate = argv0 + is_bare_command = os.sep not in raw_argv0 and (os.altsep is None or os.altsep not in raw_argv0) + if is_bare_command: + located = shutil.which(str(argv0)) + if located is not None: + candidate = Path(located) + if candidate.exists() or candidate.is_symlink(): + return Path(os.path.abspath(candidate)) + return Path(os.path.abspath(sys.executable)) + + def current_binary_path() -> Path: - """Return the absolute path of the running kickstart entry point. + """Return the resolved absolute path of the running kickstart executable. Tries `sys.argv[0]` first (correct for PyInstaller `--onefile` and `--onedir` builds and for console-script shims) and falls back to `sys.executable` when argv[0] does not point to a real file (this is what happens with `python -m`). """ - argv0 = Path(sys.argv[0]) if sys.argv and sys.argv[0] else None - if argv0 is not None and argv0.exists(): - return _safe_resolve(argv0) - return Path(sys.executable).resolve() + return _safe_resolve(current_entrypoint_path()) def detect_shell(env: Optional[dict[str, str]] = None) -> Optional[str]: @@ -212,21 +231,9 @@ def _install_onedir_bundle( copied = False if not _same_file(bundle_source, bundle_destination): - if bundle_destination.exists() or bundle_destination.is_symlink(): - if not overwrite: - raise FileExistsError(f"{bundle_destination} already exists; pass --force to overwrite") - _remove_path(bundle_destination) - - temp_destination = resolved_app_root / f".{APP_DIR_NAME}.tmp-{os.getpid()}" - if temp_destination.exists() or temp_destination.is_symlink(): - _remove_path(temp_destination) - try: - shutil.copytree(bundle_source, temp_destination, symlinks=True) - temp_destination.rename(bundle_destination) - except Exception: - if temp_destination.exists() or temp_destination.is_symlink(): - _remove_path(temp_destination) - raise + if (bundle_destination.exists() or bundle_destination.is_symlink()) and not overwrite: + raise FileExistsError(f"{bundle_destination} already exists; pass --force to overwrite") + _replace_bundle(bundle_source, bundle_destination) copied = True launcher_changed = _install_launcher(executable_destination, destination, overwrite=overwrite) @@ -333,6 +340,43 @@ def _onedir_bundle_root(source: Path, *, name: str = BINARY_NAME) -> Optional[Pa return None +def _replace_bundle(bundle_source: Path, bundle_destination: Path) -> None: + """Stage and activate a bundle with rollback for staging or activation errors.""" + app_root = bundle_destination.parent + suffix = os.getpid() + temp_destination = app_root / f".{APP_DIR_NAME}.tmp-{suffix}" + backup_destination = app_root / f".{APP_DIR_NAME}.backup-{suffix}" + + for path in (temp_destination, backup_destination): + if path.exists() or path.is_symlink(): + _remove_path(path) + + try: + shutil.copytree(bundle_source, temp_destination, symlinks=True) + except Exception: + if temp_destination.exists() or temp_destination.is_symlink(): + _remove_path(temp_destination) + raise + + previous_moved = False + try: + if bundle_destination.exists() or bundle_destination.is_symlink(): + bundle_destination.rename(backup_destination) + previous_moved = True + try: + temp_destination.rename(bundle_destination) + except Exception: + if previous_moved: + backup_destination.rename(bundle_destination) + raise + finally: + if temp_destination.exists() or temp_destination.is_symlink(): + _remove_path(temp_destination) + + if previous_moved: + _remove_path(backup_destination) + + def _install_launcher(executable: Path, destination: Path, *, overwrite: bool) -> bool: """Expose `executable` at `destination`, preferring a symlink.""" if destination.exists() or destination.is_symlink(): diff --git a/src/utils/updater.py b/src/utils/updater.py index 199789a..9a02ddb 100644 --- a/src/utils/updater.py +++ b/src/utils/updater.py @@ -31,7 +31,7 @@ APP_DIR_NAME, BINARY_NAME, InstallResult, - current_binary_path, + current_entrypoint_path, default_app_root, install_binary, ) @@ -96,21 +96,34 @@ def find_asset(assets: list[ReleaseAsset], name: str) -> Optional[ReleaseAsset]: def resolve_current_install_layout() -> tuple[Path, Optional[Path]]: """Return ``(launcher_dir, app_root_or_None)`` for the currently running install. - When the launcher is a symlink (the onedir install shape), we can recover - the app root from the symlink target. For legacy single-file installs the - launcher is a regular file and we return ``None`` so callers fall back to - `default_app_root` (or skip app-root handling entirely). + A managed launcher symlink reveals the public launcher directory and app + root through its first target. A wrapper-launched managed payload reveals + the stable app root directly. For legacy single-file installs we return + ``None`` so callers fall back to `default_app_root` (or skip app-root + handling entirely). """ - launcher = current_binary_path() + launcher = current_entrypoint_path() launcher_dir = launcher.parent + # A generated wrapper executes the stable managed payload directly. In + # that case argv[0] is already /current/kickstart, and refreshing + # that directory in place keeps the external wrapper valid. + if ( + launcher.name == BINARY_NAME + and launcher.parent.name == APP_DIR_NAME + and (launcher.parent / "_internal").is_dir() + ): + return launcher_dir, launcher.parent.parent + if launcher.is_symlink(): try: - executable = launcher.resolve() + target = launcher.readlink() except OSError: return launcher_dir, None + executable = target if target.is_absolute() else launcher.parent / target + executable = Path(os.path.abspath(executable)) # Expected layout: // - if executable.parent.name == APP_DIR_NAME: + if executable.name == BINARY_NAME and executable.parent.name == APP_DIR_NAME: return launcher_dir, executable.parent.parent return launcher_dir, None diff --git a/tests/unit/utils/test_installer.py b/tests/unit/utils/test_installer.py index 329db84..1f19295 100644 --- a/tests/unit/utils/test_installer.py +++ b/tests/unit/utils/test_installer.py @@ -70,6 +70,44 @@ def test_install_binary_missing_source_raises(tmp_path: Path) -> None: installer.install_binary(missing, tmp_path / "bin") +def test_current_entrypoint_path_preserves_symlink_for_layout_discovery(tmp_path: Path, monkeypatch) -> None: + target = tmp_path / "app" / installer.BINARY_NAME + target.parent.mkdir() + target.write_text("#!/bin/sh\n") + launcher = tmp_path / "bin" / installer.BINARY_NAME + launcher.parent.mkdir() + launcher.symlink_to(target) + monkeypatch.setattr(installer.sys, "argv", [str(launcher)]) + + assert installer.current_entrypoint_path() == launcher + assert installer.current_binary_path() == target + + +def test_current_entrypoint_path_finds_bare_command_on_path(tmp_path: Path, monkeypatch) -> None: + launcher = tmp_path / installer.BINARY_NAME + launcher.write_text("#!/bin/sh\n") + monkeypatch.setattr(installer.sys, "argv", [installer.BINARY_NAME]) + monkeypatch.setattr(installer.shutil, "which", lambda command: str(launcher)) + + assert installer.current_entrypoint_path() == launcher + + +def test_current_entrypoint_path_preserves_explicit_relative_path(tmp_path: Path, monkeypatch) -> None: + working_dir = tmp_path / "working" + working_dir.mkdir() + explicit_launcher = working_dir / installer.BINARY_NAME + explicit_launcher.write_text("#!/bin/sh\n") + path_launcher = tmp_path / "path-bin" / installer.BINARY_NAME + path_launcher.parent.mkdir() + path_launcher.write_text("#!/bin/sh\n") + + monkeypatch.chdir(working_dir) + monkeypatch.setattr(installer.sys, "argv", [f".{os.sep}{installer.BINARY_NAME}"]) + monkeypatch.setattr(installer.shutil, "which", lambda command: str(path_launcher)) + + assert installer.current_entrypoint_path() == explicit_launcher + + def test_uninstall_binary_removes_file(tmp_path: Path, single_file_binary: Path) -> None: target_dir = tmp_path / "user-bin" installer.install_binary(single_file_binary, target_dir) @@ -469,6 +507,63 @@ def test_install_binary_onedir_overwrite_replaces_bundle(tmp_path: Path) -> None assert result.copied is True +def test_install_binary_onedir_staging_failure_preserves_active_nested_bundle(tmp_path: Path, monkeypatch) -> None: + """A failed repair must leave the existing nested payload executable.""" + target_dir = tmp_path / "bin" + target_dir.mkdir() + app_root = tmp_path / "share" / "kickstart" + bundle_dest = app_root / installer.APP_DIR_NAME + nested_bundle = bundle_dest / ".kickstart" / installer.APP_DIR_NAME + (nested_bundle / "_internal").mkdir(parents=True) + nested_executable = nested_bundle / installer.BINARY_NAME + nested_executable.write_text("#!/bin/sh\necho old\n") + nested_executable.chmod(0o755) + + managed_executable = bundle_dest / installer.BINARY_NAME + managed_executable.symlink_to(nested_executable) + launcher = target_dir / installer.BINARY_NAME + launcher.symlink_to(managed_executable) + fresh_launcher = _make_fake_onedir_bundle(tmp_path / "fresh") + + def fail_copytree(*args, **kwargs): + raise OSError("staging failed") + + monkeypatch.setattr(installer.shutil, "copytree", fail_copytree) + + with pytest.raises(OSError, match="staging failed"): + installer.install_binary(fresh_launcher, target_dir, app_root=app_root, overwrite=True) + + assert launcher.resolve() == nested_executable + assert nested_executable.read_text() == "#!/bin/sh\necho old\n" + + +def test_install_binary_onedir_activation_failure_restores_active_bundle(tmp_path: Path, monkeypatch) -> None: + """A failed activation rolls the old payload back into its stable path.""" + old_launcher = _make_fake_onedir_bundle(tmp_path / "old") + target_dir = tmp_path / "bin" + app_root = tmp_path / "share" / "kickstart" + installer.install_binary(old_launcher, target_dir, app_root=app_root) + active_launcher = target_dir / installer.BINARY_NAME + old_target = active_launcher.resolve() + fresh_launcher = _make_fake_onedir_bundle(tmp_path / "fresh") + (fresh_launcher.parent / "_internal" / "marker").write_text("fresh\n") + + original_rename = Path.rename + + def fail_activation(self: Path, target: Path) -> Path: + if self.name.startswith(f".{installer.APP_DIR_NAME}.tmp-"): + raise OSError("activation failed") + return original_rename(self, target) + + monkeypatch.setattr(Path, "rename", fail_activation) + + with pytest.raises(OSError, match="activation failed"): + installer.install_binary(fresh_launcher, target_dir, app_root=app_root, overwrite=True) + + assert active_launcher.resolve() == old_target + assert (app_root / installer.APP_DIR_NAME / "_internal" / "marker").read_text() == "present\n" + + def test_install_binary_onedir_idempotent_same_bundle(tmp_path: Path) -> None: """Re-installing the same bundle is a no-op for both the payload and launcher.""" launcher = _make_fake_onedir_bundle(tmp_path / "src") @@ -478,7 +573,7 @@ def test_install_binary_onedir_idempotent_same_bundle(tmp_path: Path) -> None: # Now re-install pointing at the freshly-installed bundle (the source IS the # destination after the first install). - new_source = (app_root / installer.APP_DIR_NAME / installer.BINARY_NAME) + new_source = app_root / installer.APP_DIR_NAME / installer.BINARY_NAME second_result = installer.install_binary(new_source, target_dir, app_root=app_root, overwrite=True) assert second_result.copied is False assert second_result.already_installed is True diff --git a/tests/unit/utils/test_updater.py b/tests/unit/utils/test_updater.py index bee90ab..d2cb8d1 100644 --- a/tests/unit/utils/test_updater.py +++ b/tests/unit/utils/test_updater.py @@ -76,10 +76,7 @@ def test_expected_archive_name_default(monkeypatch): def test_expected_archive_name_overrides(): - assert ( - updater.expected_archive_name("macos-arm64", "3.13") - == "kickstart-macos-arm64-py3.13.tar.gz" - ) + assert updater.expected_archive_name("macos-arm64", "3.13") == "kickstart-macos-arm64-py3.13.tar.gz" def test_find_asset_match(): @@ -101,7 +98,7 @@ def test_find_asset_missing(): def test_resolve_current_install_layout_onedir(tmp_path, monkeypatch): - """A symlinked launcher reveals the app root via the symlink target.""" + """ENG-204 regression: preserve the managed launcher symlink for layout discovery.""" app_root = tmp_path / ".local" / "share" / "kickstart" bundle = app_root / updater.APP_DIR_NAME bundle.mkdir(parents=True) @@ -114,24 +111,85 @@ def test_resolve_current_install_layout_onedir(tmp_path, monkeypatch): launcher = launcher_dir / updater.BINARY_NAME launcher.symlink_to(executable) - monkeypatch.setattr(updater, "current_binary_path", lambda: launcher) + monkeypatch.setattr(updater.sys, "argv", [str(launcher)]) resolved_dir, resolved_root = updater.resolve_current_install_layout() assert resolved_dir == launcher_dir assert resolved_root == app_root +def test_resolve_current_install_layout_recovers_root_from_nested_payload(tmp_path, monkeypatch): + """An old nested payload still reveals the original managed app root.""" + app_root = tmp_path / ".local" / "share" / "kickstart" + bundle = app_root / updater.APP_DIR_NAME + nested_bundle = bundle / ".kickstart" / updater.APP_DIR_NAME + nested_bundle.mkdir(parents=True) + nested_executable = nested_bundle / updater.BINARY_NAME + nested_executable.write_text("#!/bin/sh\n") + nested_executable.chmod(0o755) + + managed_executable = bundle / updater.BINARY_NAME + managed_executable.symlink_to(nested_executable) + launcher_dir = tmp_path / ".local" / "bin" + launcher_dir.mkdir(parents=True) + launcher = launcher_dir / updater.BINARY_NAME + launcher.symlink_to(managed_executable) + + monkeypatch.setattr(updater.sys, "argv", [str(launcher)]) + resolved_dir, resolved_root = updater.resolve_current_install_layout() + + assert resolved_dir == launcher_dir + assert resolved_root == app_root + + +def test_resolve_current_install_layout_normalizes_relative_launcher_target(tmp_path, monkeypatch): + """A relative managed launcher target identifies the same canonical roots.""" + app_root = tmp_path / "share" / "kickstart" + bundle = app_root / updater.APP_DIR_NAME + (bundle / "_internal").mkdir(parents=True) + executable = bundle / updater.BINARY_NAME + executable.write_text("#!/bin/sh\n") + executable.chmod(0o755) + + launcher_dir = tmp_path / "bin" + launcher_dir.mkdir() + launcher = launcher_dir / updater.BINARY_NAME + launcher.symlink_to(Path("..") / "share" / "kickstart" / updater.APP_DIR_NAME / updater.BINARY_NAME) + + monkeypatch.setattr(updater.sys, "argv", [str(launcher)]) + resolved_dir, resolved_root = updater.resolve_current_install_layout() + + assert resolved_dir == launcher_dir + assert resolved_root == app_root + + def test_resolve_current_install_layout_legacy_single_file(tmp_path, monkeypatch): """A non-symlink launcher (legacy single-file install) yields app_root=None.""" launcher = tmp_path / "bin" / updater.BINARY_NAME launcher.parent.mkdir(parents=True) launcher.write_text("#!/bin/sh\n") launcher.chmod(0o755) - monkeypatch.setattr(updater, "current_binary_path", lambda: launcher) + monkeypatch.setattr(updater.sys, "argv", [str(launcher)]) resolved_dir, resolved_root = updater.resolve_current_install_layout() assert resolved_dir == launcher.parent assert resolved_root is None +def test_resolve_current_install_layout_generated_wrapper_payload(tmp_path, monkeypatch): + """A wrapper-executed managed payload refreshes its stable bundle in place.""" + app_root = tmp_path / "share" / "kickstart" + bundle = app_root / updater.APP_DIR_NAME + (bundle / "_internal").mkdir(parents=True) + executable = bundle / updater.BINARY_NAME + executable.write_text("#!/bin/sh\n") + executable.chmod(0o755) + + monkeypatch.setattr(updater.sys, "argv", [str(executable)]) + resolved_dir, resolved_root = updater.resolve_current_install_layout() + + assert resolved_dir == bundle + assert resolved_root == app_root + + def test_resolve_current_install_layout_symlink_with_unexpected_layout(tmp_path, monkeypatch): """A symlink that doesn't end in `//kickstart` falls back to None.""" elsewhere = tmp_path / "somewhere" / "kickstart" @@ -142,7 +200,7 @@ def test_resolve_current_install_layout_symlink_with_unexpected_layout(tmp_path, launcher = tmp_path / "bin" / updater.BINARY_NAME launcher.parent.mkdir(parents=True) launcher.symlink_to(elsewhere) - monkeypatch.setattr(updater, "current_binary_path", lambda: launcher) + monkeypatch.setattr(updater.sys, "argv", [str(launcher)]) resolved_dir, resolved_root = updater.resolve_current_install_layout() assert resolved_dir == launcher.parent assert resolved_root is None @@ -260,9 +318,7 @@ def _release_payload(tag: str, archive_name: str, *, with_sha: bool = True) -> d {"name": archive_name, "browser_download_url": f"https://x/{archive_name}"}, ] if with_sha: - assets.append( - {"name": f"{archive_name}.sha256", "browser_download_url": f"https://x/{archive_name}.sha256"} - ) + assets.append({"name": f"{archive_name}.sha256", "browser_download_url": f"https://x/{archive_name}.sha256"}) return {"tag_name": tag, "assets": assets} @@ -438,8 +494,8 @@ def test_check_for_update_install_failure_is_categorized(mock_print, tmp_path): @patch("src.utils.updater.__version__", "1.0.0") @patch("builtins.print") -def test_check_for_update_end_to_end_installs_bundle(mock_print, tmp_path, monkeypatch): - """Happy path: latest release publishes a tarball we can download, verify, and install.""" +def test_check_for_update_end_to_end_repairs_and_keeps_stable_bundle_layout(mock_print, tmp_path, monkeypatch): + """ENG-204 regression: two upgrades repair nesting and keep one stable managed root.""" monkeypatch.setattr(updater.sys, "platform", "linux") monkeypatch.setattr(updater.platform, "machine", lambda: "x86_64") monkeypatch.setattr(updater, "host_python_minor", lambda: "3.14") @@ -452,6 +508,7 @@ def test_check_for_update_end_to_end_installs_bundle(mock_print, tmp_path, monke archive_bytes = archive_path.read_bytes() # Compute the matching sha256. import hashlib + archive_sha = hashlib.sha256(archive_bytes).hexdigest() payload = _release_payload("v1.1.0", archive_name, with_sha=True) @@ -469,36 +526,48 @@ def fake_get(url, **kwargs): response.__exit__.return_value = False return response - # Pretend we are running from a fresh onedir install so resolve_current_install_layout - # gives us a launcher_dir + app_root pair. + # Reproduce the broken layout created by an older upgrade: the public launcher + # still points at the canonical managed executable, but that executable became + # another symlink to a nested payload. app_root = tmp_path / "share" / "kickstart" bundle_dest = app_root / updater.APP_DIR_NAME - bundle_dest.mkdir(parents=True) - (bundle_dest / updater.BINARY_NAME).write_text("#!/bin/sh\nold\n") - (bundle_dest / updater.BINARY_NAME).chmod(0o755) + nested_bundle = bundle_dest / ".kickstart" / updater.APP_DIR_NAME + nested_bundle.mkdir(parents=True) + nested_executable = nested_bundle / updater.BINARY_NAME + nested_executable.write_text("#!/bin/sh\nold\n") + nested_executable.chmod(0o755) + managed_executable = bundle_dest / updater.BINARY_NAME + managed_executable.symlink_to(nested_executable) launcher_dir = tmp_path / "bin" launcher_dir.mkdir() launcher = launcher_dir / updater.BINARY_NAME - launcher.symlink_to(bundle_dest / updater.BINARY_NAME) - monkeypatch.setattr(updater, "current_binary_path", lambda: launcher) + launcher.symlink_to(managed_executable) + monkeypatch.setattr(updater.sys, "argv", [str(launcher)]) with patch("src.utils.updater.requests.get", side_effect=fake_get): - result = updater.check_for_update() + first_result = updater.check_for_update() + assert launcher.resolve().parent == bundle_dest + assert not (bundle_dest / ".kickstart").exists() - assert result.target_version == "1.1.0" - assert result.outcome.value == "updated" - assert result.error_category.value == "none" - assert result.checksum_status.value == "verified" + second_result = updater.check_for_update() + + for result in (first_result, second_result): + assert result.target_version == "1.1.0" + assert result.outcome.value == "updated" + assert result.error_category.value == "none" + assert result.checksum_status.value == "verified" - # Verify the launcher now points at the freshly-extracted bundle. + # Both upgrades converge on the same public launcher and canonical payload. assert launcher.is_symlink() + assert launcher.readlink() == bundle_dest / updater.BINARY_NAME target = launcher.resolve() - assert target.parent.name == updater.APP_DIR_NAME + assert target.parent == bundle_dest assert target.read_text().startswith("#!/bin/sh\necho ok") + assert not (bundle_dest / ".kickstart").exists() messages = [c.args[0] for c in mock_print.call_args_list] - assert any("Updated to 1.1.0" in m for m in messages) - assert any("Checksum verified" in m for m in messages) + assert sum("Updated to 1.1.0" in m for m in messages) == 2 + assert sum("Checksum verified" in m for m in messages) == 2 @patch("src.utils.updater.__version__", "1.0.0") @@ -529,7 +598,7 @@ def fake_get(url, **kwargs): response.__exit__.return_value = False return response - monkeypatch.setattr(updater, "current_binary_path", lambda: tmp_path / "bin" / "kickstart") + monkeypatch.setattr(updater, "current_entrypoint_path", lambda: tmp_path / "bin" / "kickstart") with patch("src.utils.updater.requests.get", side_effect=fake_get): result = updater.check_for_update()