Skip to content

fix(update): return to the user's feature branch after a successful update - #67884

Open
nickkpoon wants to merge 1 commit into
NousResearch:mainfrom
nickkpoon:fix/update-restore-feature-branch
Open

fix(update): return to the user's feature branch after a successful update#67884
nickkpoon wants to merge 1 commit into
NousResearch:mainfrom
nickkpoon:fix/update-restore-feature-branch

Conversation

@nickkpoon

Copy link
Copy Markdown

Problem

hermes update's no-updates path politely switches back to whatever branch the user was on — but the successful-pull path leaves HEAD on the update target, silently dropping any local feature branch from the working tree.

For editable/git installs this swaps the running code out from under the user. Real incident (2026-07-19): an update triggered mid-session moved a live install from a feature branch to stock main; the gateway restarted seconds later on code the user never chose to run, and the features that branch carried broke with confusing provider errors. The asymmetry makes it worse: users learn from the no-op path that their branch is preserved, so the successful path's behavior is a surprise.

Fix

After the pull succeeds (and before the dependency sync, so the venv is built against the tree that will actually run), _restore_feature_branch_after_update:

  1. checks the user's original branch back out,
  2. rebases it onto the freshly updated target (--autostash, since the flow may have just re-applied stashed local edits),
  3. re-runs the same critical-file syntax guard the pulled target got.

Every failure mode falls back to today's behavior — staying on the target — with the branch left untouched and the manual recovery command printed: checkout failure, rebase conflict (aborted cleanly), or a rebased tree that can't bootstrap the CLI. The helper never raises; a branch-restore failure can't fail the update.

Tests

tests/hermes_cli/test_update_branch_restore.py — 6 tests on real throwaway git repos (no network): clean rebase + return, no-op on target/detached HEAD, conflict fallback (branch SHA untouched, no rebase-in-progress leftovers), syntax-guard fallback, dirty-tree autostash.

🤖 Generated with Claude Code

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to closed #40673, which also addressed post-update feature-branch restoration. This is an active resubmission, not a duplicate of a closed PR.

@x7peeps

x7peeps commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Closing as duplicate

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a real update-path asymmetry: current main still restores current_branch only for commit_count == 0 at hermes_cli/update_cmd.py:3319-3326, while a successful merge proceeds to dependency installation without restoring it (hermes_cli/update_cmd.py:3429-3542).

Problems

  • The update pipeline was extracted after this PR opened. Current hermes_cli/main.py:4786-4795 re-exports _cmd_update_impl from hermes_cli/update_cmd.py; commit 927463efcc made that mechanical move. The proposed main.py helper/call site therefore needs relocation during salvage.
  • Current successful updates restore the autostash before the proposed helper would run (hermes_cli/update_cmd.py:3499-3524). A failed stash apply resets the target working tree and leaves the stash for manual recovery (hermes_cli/update_cmd.py:1025-1055), but the proposed helper has no stash result to restore after returning to the feature branch. The new tests exercise the helper directly rather than this integrated ordering.

Suggested changes

  • Move the implementation into hermes_cli/update_cmd.py and preserve its _m() helper indirection.
  • Restore a feature branch's autostash only after its checkout/rebase path, with an end-to-end dirty-feature-branch test including stash conflicts.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@nickkpoon
nickkpoon force-pushed the fix/update-restore-feature-branch branch from d33b578 to 1dc2399 Compare August 16, 2026 06:23
@nickkpoon

Copy link
Copy Markdown
Author

I have updated the PR to address the recent architectural changes and the sweeper feedback:

  1. Relocated Implementation: Moved the feature branch restoration logic into the newly extracted hermes_cli/update_cmd.py, preserving the _m() helper indirection to prevent circular imports.
  2. Autostash Ordering: Adjusted the execution sequence so that the feature branch checkout and rebase occur before the finally block restores the autostash. This ensures the stashed changes are correctly applied on top of the newly updated feature branch.
  3. E2E Testing: Added a comprehensive end-to-end test (test_end_to_end_dirty_feature_branch_stash_conflict) that mocks the git fetch/pull cycle. It explicitly simulates a dirty feature branch, forces a stash conflict during restoration, and verifies that the update safely fails closed by falling back to the target branch while keeping the stash intact for manual recovery.

Everything is passing locally and the update flow is thoroughly tested. Ready for re-review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants