diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 4706778cc95..3965435c1c1 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -1209,16 +1209,19 @@ function Install-Repository { } $didUpdate = $true } else { - # Directory exists but isn't a usable git repo. Wipe it and - # fall through to a fresh clone. A leftover ``.git`` stub from - # a partial uninstall used to lock the installer into the - # "update" branch forever, emitting three ``fatal: not a git - # repository`` errors and failing with "not in a git directory". - Write-Warn "Existing directory at $InstallDir is not a valid git repo -- replacing it." + # Directory exists but isn't a usable git repo -- e.g. an + # interrupted clone with no initial commit (#40998), or a leftover + # ``.git`` stub from a partial uninstall that used to lock the + # installer into the "update" branch forever. Move it aside rather + # than deleting it -- never destroy a directory the user might still + # want -- and fall through to a fresh clone. + $backupDir = "$InstallDir.broken-" + (Get-Date -Format "yyyyMMdd-HHmmss") + Write-Warn "Existing directory at $InstallDir is not a valid git repo." + Write-Warn "Moving it aside to $backupDir before re-cloning." try { - Remove-Item -Recurse -Force $InstallDir -ErrorAction Stop + Move-Item -LiteralPath $InstallDir -Destination $backupDir -ErrorAction Stop } catch { - Write-Err "Could not remove $InstallDir : $_" + Write-Err "Could not move $InstallDir aside : $_" Write-Info "Close any programs that might be using files in $InstallDir (editors," Write-Info "terminals, running hermes processes) and try again." throw diff --git a/scripts/install.sh b/scripts/install.sh index 05270254993..db3ae5b8bb6 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1094,11 +1094,14 @@ clone_repo() { # An interrupted previous clone leaves a .git with no initial commit, where # the update path's `git stash` / `git checkout` abort with "You do not - # have the initial commit yet" and fail the install (#40998). Drop such a - # partial checkout so the fresh-clone path below handles it cleanly. + # have the initial commit yet" and fail the install (#40998). Move such a + # partial checkout aside -- never delete it, in case it holds something the + # user wants -- so the fresh-clone path below can proceed. if [ -d "$INSTALL_DIR/.git" ] && ! git -C "$INSTALL_DIR" rev-parse --verify HEAD >/dev/null 2>&1; then - log_warn "Existing checkout at $INSTALL_DIR has no commits (interrupted clone) -- replacing it." - rm -rf "$INSTALL_DIR" + backup_dir="${INSTALL_DIR}.broken-$(date -u +%Y%m%d-%H%M%S)" + log_warn "Existing checkout at $INSTALL_DIR has no commits (interrupted clone)." + log_warn "Moving it aside to $backup_dir before re-cloning." + mv "$INSTALL_DIR" "$backup_dir" fi if [ -d "$INSTALL_DIR" ]; then diff --git a/tests/test_install_no_initial_commit.py b/tests/test_install_no_initial_commit.py index 586f7b35cb0..321ddd0b400 100644 --- a/tests/test_install_no_initial_commit.py +++ b/tests/test_install_no_initial_commit.py @@ -64,7 +64,7 @@ def _run_guard(install_dir: Path) -> None: assert res.returncode == 0, res.stderr -def test_install_sh_guard_removes_commitless_checkout(tmp_path: Path) -> None: +def test_install_sh_guard_moves_commitless_checkout_aside(tmp_path: Path) -> None: install_dir = tmp_path / "hermes-agent" install_dir.mkdir() _git(install_dir, "init") @@ -78,7 +78,12 @@ def test_install_sh_guard_removes_commitless_checkout(tmp_path: Path) -> None: assert head.returncode != 0 _run_guard(install_dir) - assert not install_dir.exists(), "commit-less checkout should be removed" + # The original path is cleared so a fresh clone can proceed, but the + # content is preserved in a backup (never deleted -- review feedback). + assert not install_dir.exists(), "commit-less checkout should be moved aside" + backups = list(install_dir.parent.glob(install_dir.name + ".broken-*")) + assert len(backups) == 1, "broken checkout should be moved to one backup dir" + assert (backups[0] / "leftover.txt").read_text() == "partial download" def test_install_sh_guard_keeps_repo_with_commits(tmp_path: Path) -> None: @@ -92,6 +97,9 @@ def test_install_sh_guard_keeps_repo_with_commits(tmp_path: Path) -> None: _run_guard(install_dir) assert install_dir.exists() assert (install_dir / "f.txt").exists(), "a real checkout must be left intact" + assert not list(install_dir.parent.glob(install_dir.name + ".broken-*")), ( + "a healthy checkout must not be moved aside" + ) def test_install_sh_guard_ignores_non_repo_dir(tmp_path: Path) -> None: @@ -117,3 +125,12 @@ def test_install_ps1_validity_requires_initial_commit() -> None: r"if \(\$revParseOk -and \$statusOk -and \$hasCommit\) \{", text, ), "repo validity must be gated on $hasCommit, not just rev-parse + status" + # Cleanup must be non-destructive: move the broken checkout aside, never + # `Remove-Item -Recurse -Force` it (review feedback on #40998). + assert "Move-Item -LiteralPath $InstallDir" in text, ( + "install.ps1 must move an invalid checkout aside, not delete it" + ) + assert "Remove-Item -Recurse -Force $InstallDir -ErrorAction Stop" not in text, ( + "the destructive wipe of an existing install dir must be gone " + "(transient cleanup of a just-failed clone is fine)" + )