fix(update): never leave a failed branch rollback on broken code

Review follow-ups for the branch-switch rollback: - If the parked branch cannot be checked out again (for example, another worktree holds it), restore its commit detached. Before this, the install stayed on the broken update branch, and the recovery hint repeated the failing command. - Any `merge-base --is-ancestor` result other than "contained" keeps the strict baseline. rc 128 used to fall back to the lenient one, which contradicted its own comment. - `_pull_updates` returns the baseline it verified, and `_apply_pulled_update` checks against it. Before, that function recomputed the baseline with a second rev-parse and merge-base against a hardcoded `origin/<branch>`, which a fork push in between could move. - Drop a `commit_count == -1` check that could never be false. - Update the rollback description in `updating.md`. - Tests share the fixture `git` helper, a repo-init helper and a `pull()` wrapper instead of repeating those calls.
2026-09-28 14:18:20 +05:30
parent 8ba5d3948c
commit 9e8202f802
3 changed files with 63 additions and 64 deletions
--- a/hermes_cli/update_cmd.py
+++ b/hermes_cli/update_cmd.py
@@ -832,6 +832,12 @@ def _rollback_if_pulled_syntax_error(git_cmd, pre_pull_sha, *, rollback_branch=N
        target = rollback_branch if rollback_branch not in (None, "HEAD") else pre_pull_sha[:10]
        print(f"→ Rolling back to {target}...")
        rollback_result = _git_run(git_cmd, rollback_args)
+        if rollback_result.returncode != 0 and rollback_branch not in (None, "HEAD"):
+            # The parked branch can be unavailable (e.g. checked out in another worktree): restore
+            # its commit detached so the install still boots the code it ran before.
+            print(f"  ✗ Could not check out {rollback_branch}; restoring its commit detached.")
+            rollback_args = ["checkout", "--detach", pre_pull_sha]
+            rollback_result = _git_run(git_cmd, rollback_args)
        if rollback_result.returncode == 0:
            print("  ✓ Rollback complete — your install is unchanged.")
            print("  Try ``hermes update`` again later once a fix lands.")
@@ -855,9 +861,9 @@ def _update_movement_baseline(git_cmd, pre_pull_sha, pre_sync_sha, rollback_bran
        # the stale/diverged branch tip, not the original detached checkout.
        contains_target = _git_run(
            git_cmd, ["merge-base", "--is-ancestor", target_sha, pre_pull_sha])
-        # rc 128 (a SHA git cannot resolve) falls through to the pre-switch baseline: the worst
-        # case is a loud "did not move" refusal, never a silent success.
-        if contains_target.returncode == 1:
+        # Anything but "contained" (rc 1, or rc 128 for a SHA git cannot resolve) keeps the strict
+        # baseline: the worst case is a loud "did not move" refusal, never a silent success.
+        if contains_target.returncode != 0:
            return pre_pull_sha
    return pre_sync_sha or pre_pull_sha

@@ -868,7 +874,7 @@ def _pull_updates(
    in_place_update=False, _windows_gateway_resume=None):
    """Fast-forward onto ``origin/<branch>`` and settle the autostash. Divergence by shape:
    custom branch -> merge, same branch -> rescue ref then reset; a
-    post-pull syntax error in a critical file rolls back. Exits on failure; returns pre-pull SHA."""
+    post-pull syntax error in a critical file rolls back. Exits on failure; returns the SHA HEAD had to move off."""
    update_succeeded = False
    # Rescue refs must retain the immediate pre-pull tip, even when syntax
    # rollback needs to cross an earlier upstream sync.
@@ -934,7 +940,7 @@ def _pull_updates(
                _m()._restore_stashed_changes(
                    git_cmd, _m().PROJECT_ROOT, auto_stash_ref, prompt_user=prompt_for_restore,
                    input_fn=gw_input_fn)
-    return pre_pull_sha
+    return movement_baseline


@dataclass
@@ -1104,7 +1110,7 @@ def _prepare_checkout_for_update(
        parked_branch_switched=parked_branch_switched, prompt_for_restore=prompt_for_restore,
        switch_block_reason=switch_block_reason, upstream_checked=upstream_checked,
        pre_sync_sha=moved_from_sha, rollback_branch=rollback_branch,
-        switched_without_new_commits=switched_without_new_commits and commit_count == -1)
+        switched_without_new_commits=switched_without_new_commits)


@dataclass
@@ -1343,13 +1349,10 @@ def _finish_already_up_to_date(


 def _apply_pulled_update(
-    git_cmd, branch, pre_pull_sha, _plan, *, _windows_gateway_resume, completion_request: dict) -> None:
-    """Post-pull phase: verify HEAD, sync Python/Node/web/Desktop, maintenance, fleet restart."""
-    movement_baseline = _plan.pre_sync_sha or pre_pull_sha
-    if _plan.rollback_branch is not None:
-        target_sha = (_git_run(git_cmd, ["rev-parse", f"origin/{branch}^{{commit}}"]).stdout or "").strip()
-        movement_baseline = _update_movement_baseline(
-            git_cmd, pre_pull_sha, _plan.pre_sync_sha, _plan.rollback_branch, target_sha)
+    git_cmd, branch, movement_baseline, _plan, *, _windows_gateway_resume, completion_request: dict) -> None:
+    """Post-pull phase: verify HEAD, sync Python/Node/web/Desktop, maintenance, fleet restart.
+
+    ``movement_baseline`` is what ``_pull_updates`` returned: the SHA HEAD had to move off."""
    post_pull_sha = _verify_head_after_pull(
        git_cmd, branch, movement_baseline, in_place_update=_plan.in_place_update,
        _windows_gateway_resume=_windows_gateway_resume)
@@ -1526,7 +1529,7 @@ def _cmd_update_impl(args, gateway_mode: bool):
            print("→ Updates available (commit count unknown on this shallow checkout)")

        print("→ Pulling updates...")
-        pre_pull_sha = _pull_updates(
+        movement_baseline = _pull_updates(
            git_cmd, branch, _plan.auto_stash_ref, prompt_for_restore=_plan.prompt_for_restore,
            gw_input_fn=gw_input_fn, discard_local_changes=opts.discard_local_changes,
            keep_stash=opts.keep_stash, target_ref=target_ref, pre_sync_sha=_plan.pre_sync_sha,
@@ -1534,7 +1537,7 @@ def _cmd_update_impl(args, gateway_mode: bool):
            sync_upstream=is_fork and branch == "main" and not release_sha, assume_yes=assume_yes,
            in_place_update=_plan.in_place_update, _windows_gateway_resume=_windows_gateway_resume)
        _apply_pulled_update(
-            git_cmd, branch, pre_pull_sha, _plan,
+            git_cmd, branch, movement_baseline, _plan,
            _windows_gateway_resume=_windows_gateway_resume, completion_request=completion_request)
    except subprocess.CalledProcessError as e:
        try:
--- a/tests/hermes_cli/test_update_existing_branch_baseline.py
+++ b/tests/hermes_cli/test_update_existing_branch_baseline.py
@@ -1,25 +1,25 @@
 """Real Git regression coverage for updates that land via an existing branch."""
 import subprocess
+from pathlib import Path

 import pytest

 from hermes_cli import update_cmd
-from tests.hermes_cli.test_update_target_identity import update_tree  # noqa: F401
+from tests.hermes_cli.test_update_target_identity import git, update_tree  # noqa: F401


-def git(root, *args):
-    result = subprocess.run(["git", *args], cwd=root, capture_output=True, text=True)
-    assert result.returncode == 0, result.stderr
-    return result.stdout.strip()
+def init_repo(root, monkeypatch):
+    root.mkdir()
+    git(root, "init", "-q", "-b", "main")
+    git(root, "config", "user.name", "Fixture")
+    git(root, "config", "user.email", "fixture@example.com")
+    monkeypatch.setattr(update_cmd._m(), "PROJECT_ROOT", root)


@pytest.fixture
 def checkout(tmp_path, monkeypatch):
    root = tmp_path / "checkout"
-    root.mkdir()
-    git(root, "init", "-q", "-b", "main")
-    git(root, "config", "user.name", "Fixture")
-    git(root, "config", "user.email", "fixture@example.com")
+    init_repo(root, monkeypatch)
    (root / "cli.py").write_text("value = 1\n", encoding="utf8")
    git(root, "add", ".")
    git(root, "commit", "-qm", "old")
@@ -29,7 +29,6 @@ def checkout(tmp_path, monkeypatch):
    tip = git(root, "rev-parse", "HEAD")
    git(root, "update-ref", "refs/remotes/origin/main", tip)
    git(root, "checkout", "-q", "--detach", old)
-    monkeypatch.setattr(update_cmd._m(), "PROJECT_ROOT", root)
    return root, old, tip


@@ -41,18 +40,21 @@ def prepare(root, *, is_fork=False):
    )


+def pull(plan, **kwargs):
+    return update_cmd._pull_updates(
+        ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
+        gw_input_fn=None, discard_local_changes=False, keep_stash=False,
+        pre_sync_sha=plan.pre_sync_sha, rollback_branch=plan.rollback_branch, **kwargs,
+    )
+
+
 def test_existing_main_counts_from_running_detached_code(checkout):
    root, old, tip = checkout
    plan = prepare(root)
    assert git(root, "rev-parse", "HEAD") == tip
    assert plan.commit_count == 1
    assert plan.pre_sync_sha == old
-    update_cmd._pull_updates(
-        ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
-        gw_input_fn=None, discard_local_changes=False, keep_stash=False,
-        pre_sync_sha=plan.pre_sync_sha,
-        rollback_branch=plan.rollback_branch,
-    )
+    pull(plan)
    assert git(root, "rev-parse", "HEAD") == tip


@@ -80,11 +82,7 @@ def test_syntax_failure_returns_to_original_checkout_without_rewriting_main(chec
    git(root, "checkout", "-q", "feature" if parked else old)
    plan = prepare(root)
    with pytest.raises(SystemExit) as exc:
-        update_cmd._pull_updates(
-            ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
-            gw_input_fn=None, discard_local_changes=False, keep_stash=False,
-            pre_sync_sha=plan.pre_sync_sha, rollback_branch=plan.rollback_branch,
-        )
+        pull(plan)
    assert exc.value.code == 1
    assert git(root, "rev-parse", "HEAD") == old
    assert git(root, "rev-parse", "--abbrev-ref", "HEAD") == ("feature" if parked else "HEAD")
@@ -92,6 +90,26 @@ def test_syntax_failure_returns_to_original_checkout_without_rewriting_main(chec
    assert (root / "cli.py").read_text(encoding="utf8") == "value = 1\n"


+
+def test_rollback_restores_the_commit_when_the_parked_branch_is_taken(checkout, tmp_path):
+    """Another worktree holding the parked branch must not leave the install on broken code."""
+    root, old, tip = checkout
+    git(root, "checkout", "-qb", "feature")
+    git(root, "commit", "--allow-empty", "-qm", "local")
+    feature_tip = git(root, "rev-parse", "HEAD")
+    git(root, "checkout", "-q", "main")
+    (root / "cli.py").write_text("def broken(\n", encoding="utf8")
+    git(root, "commit", "-qam", "bad upstream")
+    git(root, "update-ref", "refs/remotes/origin/main", git(root, "rev-parse", "HEAD"))
+    git(root, "checkout", "-q", "feature")
+    plan = prepare(root)
+    git(root, "worktree", "add", "-q", str(tmp_path / "other"), "feature")
+    with pytest.raises(SystemExit):
+        pull(plan)
+    assert git(root, "rev-parse", "HEAD") == feature_tip
+    assert git(root, "rev-parse", "--abbrev-ref", "HEAD") == "HEAD"
+    assert (root / "cli.py").read_text(encoding="utf8") == "value = 1\n"
+
 def test_locally_ahead_switch_still_needs_completion(checkout):
    root, old, tip = checkout
    git(root, "checkout", "-q", "--detach", tip)
@@ -123,11 +141,7 @@ def test_merge_that_reports_success_without_moving_stale_main_is_refused(checkou

    monkeypatch.setattr(update_cmd, "_git_run", merge_is_a_silent_noop)
    with pytest.raises(SystemExit):
-        update_cmd._pull_updates(
-            ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
-            gw_input_fn=None, discard_local_changes=False, keep_stash=False,
-            pre_sync_sha=plan.pre_sync_sha, rollback_branch=plan.rollback_branch,
-        )
+        pull(plan)
    assert git(root, "rev-parse", "HEAD") == old


@@ -137,11 +151,7 @@ def test_stale_local_main_can_catch_up_to_original_detached_tip(checkout):
    git(root, "checkout", "-q", "--detach", tip)
    plan = prepare(root)
    assert plan.commit_count != 0
-    update_cmd._pull_updates(
-        ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
-        gw_input_fn=None, discard_local_changes=False, keep_stash=False,
-        pre_sync_sha=plan.pre_sync_sha, rollback_branch=plan.rollback_branch,
-    )
+    pull(plan)
    assert git(root, "rev-parse", "main") == tip


@@ -207,12 +217,7 @@ def test_early_fork_sync_without_push_still_completes(checkout, monkeypatch):
    assert plan.upstream_checked
    assert git(root, "rev-parse", "HEAD") == upstream_tip
    assert git(root, "rev-parse", "origin/main") == tip
-    before_pull = update_cmd._pull_updates(
-        ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
-        gw_input_fn=None, discard_local_changes=False, keep_stash=False,
-        pre_sync_sha=plan.pre_sync_sha, rollback_branch=plan.rollback_branch,
-        sync_upstream=True,
-    )
+    before_pull = pull(plan, sync_upstream=True)
    completed = []
    monkeypatch.setattr(update_cmd, "_complete_source_update", completed.append)
    request = {}
@@ -230,7 +235,6 @@ def test_command_hands_off_after_existing_main_switch(update_tree, monkeypatch,
    run = subprocess.run

    def local_git_only(command, *args, **kwargs):
-        from pathlib import Path
        assert Path(command[0]).name.lower() in {"git", "git.exe"}, command
        assert Path(kwargs["cwd"]).resolve() in {t.clone, t.origin}, command
        return run(command, *args, **kwargs)
@@ -251,11 +255,7 @@ def test_fork_sync_round_trip_is_not_misclassified_as_noop(tmp_path, monkeypatch
    """origin/main moves first, then the upstream sync returns HEAD to the SHA that was
    running before the update: still a successful branch repair, not a no-op."""
    root = tmp_path / "fork"
-    root.mkdir()
-    git(root, "init", "-q", "-b", "main")
-    git(root, "config", "user.name", "Fixture")
-    git(root, "config", "user.email", "fixture@example.com")
-    monkeypatch.setattr(update_cmd._m(), "PROJECT_ROOT", root)
+    init_repo(root, monkeypatch)

    def commit(value):
        (root / "state.txt").write_text(value + "\n", encoding="utf-8")
@@ -279,11 +279,7 @@ def test_fork_sync_round_trip_is_not_misclassified_as_noop(tmp_path, monkeypatch
        return True

    monkeypatch.setattr(update_cmd._m(), "_sync_with_upstream_if_needed", sync_upstream)
-    update_cmd._pull_updates(
-        ["git"], "main", plan.auto_stash_ref, prompt_for_restore=False,
-        gw_input_fn=None, discard_local_changes=False, keep_stash=False,
-        pre_sync_sha=plan.pre_sync_sha, rollback_branch=plan.rollback_branch, sync_upstream=True,
-    )
+    pull(plan, sync_upstream=True)

    assert git(root, "rev-parse", "HEAD") == upstream_tip
    assert git(root, "rev-parse", "main") == upstream_tip
--- a/website/docs/getting-started/updating.md
+++ b/website/docs/getting-started/updating.md
@@ -135,7 +135,7 @@ For an admitted source checkout, `hermes update` runs these phases:

 1. **Pre-update snapshot** — Hermes saves selected state files for every profile in that profile's `state-snapshots/` directory. These include pairing data, cron jobs, `config.yaml`, `.env`, and `auth.json`. Automatic quick snapshots skip individual files larger than 1 GiB. `updates.pre_update_backup` selects `quick`, `full`, or `off`. Full archives use the [backup exclusions](../reference/faq.md#hermes-backup-vs-hermes-profile-export). Recovery uses [Snapshots and rollback](../user-guide/checkpoints-and-rollback.md). Quick snapshots recover state files, not application code. The snapshot is best-effort: if it fails, the update prints a `⚠ Pre-update snapshot FAILED` warning and continues, and the receipt records `pre_update_backup` as a failed step (a deliberate `off`/`--no-backup` lands in the receipt's skips with its reason instead).
 2. **Code update** — applies the configured source branch or stable release tag and updates submodules.
-3. **Post-pull syntax validation + auto-rollback** — after the pull, Hermes compiles the nine critical files every `hermes` invocation imports at startup. If any fails to parse (e.g. an orphan merge-conflict marker, an accidentally truncated file), Hermes runs `git reset --hard <pre-pull-sha>` to roll the install back so your shell stays bootable. Re-run `hermes update` once the upstream fix lands.
+3. **Post-pull syntax validation + auto-rollback** — after the pull, Hermes compiles the nine critical files every `hermes` invocation imports at startup. If any fails to parse (e.g. an orphan merge-conflict marker, an accidentally truncated file), Hermes rolls the install back so your shell stays bootable: `git reset --hard <pre-pull-sha>` on the update branch, or, when the update started from a detached checkout or a parked feature branch, `git checkout --detach <sha>` / `git checkout <branch>` back to where it started (a branch it cannot check out again is restored detached at the same commit). Re-run `hermes update` once the upstream fix lands.
 4. **Dependency preparation** — PM provisions required tools and prepares a complete Python environment from the new lock, existing extras, and enabled plugin requirements. It validates that environment before publishing its selection. A plugin never fails the update. A plugin that no longer fits the new core is added to `plugins.disabled` in every profile that enables it (a memory provider has `memory.provider` cleared). That covers a `requires-python` that excludes Hermes's Python, a `manifest_version` newer than this Hermes supports, and dependencies that the resolver proves can't resolve alongside core and earlier plugins in config order, or that fail their own build. A download or network failure gets one retry and is disabled if it fails again. The update prints `⚠ Disabled plugin '<name>' in <home>: <reason>`, records it in the receipt's warnings, and continues. Re-enable it with `hermes plugins enable <name>` once the plugin ships a compatible release, or once the network is back. A `requires_hermes` range the running version misses never disables a plugin, because a source checkout without release tags can read as an older release. That plugin sits out instead (`⚠ Left plugin '<name>' … out of this update`), stays enabled, and rejoins once Hermes reports a version it accepts. A secondary profile whose config cannot be read sits out the same way until the config is fixed. Only a core that cannot build on its own fails this step.
 5. **Config migration** — detects new config options added since your version and prompts you to set them
 6. **Desktop rebuild (stage-and-swap)** — if the Hermes Desktop app was built from this checkout, it is rebuilt so the GUI matches the new code. The rebuild packs into a temporary staging directory next to `apps/desktop/release/`, verifies the staged app, and only then renames it over the previous build (on Windows a real-time scanner briefly holding `release/win-unpacked` is ridden out with a few short retries). A rebuild that fails at any point — corrupt Electron download, missing dependency, disk full — leaves the previous app untouched and launchable; the update fails at that step, and `hermes desktop --build-only --force-build` or the next `hermes update` retries the rebuild. On macOS the rebuilt bundle is then copied (with `ditto`, signature intact) over a stale `/Applications/Hermes.app` or `~/Applications/Hermes.app`, so the copy Finder and the Dock launch matches the backend; an installed copy that is currently running is left alone and the update tells you to quit it and run `hermes update` again. On Windows, when the update finishes from inside the Desktop app it would rebuild (the app completing an interrupted update at launch), the rebuild is skipped with a notice instead: Windows locks a running app's files, and stopping the app would end the update with it. The rest of the update completes; quit Hermes Desktop and run `hermes desktop` from a terminal, or use **Update now** in **Settings → About**, to rebuild and reopen it.