fix(update): reconcile local branches during update - #22037
Conversation
|
Real-world confirmation from a local dogfood/update flow: this is still a sharp edge for local-only patch branches. Observed flow on macOS managed install ( After the update, the active checkout was back on This was not uncommitted working-tree loss — the commits still existed on the local branch — but from the runtime/user perspective the update silently deactivated the local patched runtime. Important nuance for this PR: the affected branch was local-only and had no meaningful remote tracking branch. A fix that only switches back/rebases against
This aligns with the user-facing problem described here: local contributors/dogfooders can think their patches “disappeared” after a successful update even though Git merely switched the active checkout away from their branch. Related recent PR: #40673 also touches feature-branch update behavior/banner distance, but this local-only branch case seems closest to this PR’s scope. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real update safety gap. Current main still switches from a local branch to the update target at hermes_cli/main.py:9608-9622, while the successful-update path does not check the original branch back out; its only restoration is in the no-update return at hermes_cli/main.py:9668-9691.
Problems
hermes_cli/main.py:6187-6191treats a failedgit cherryinvocation as an empty comparison. Thenhermes_cli/main.py:6252-6253treats it as normal and permits the update to remain onmain, recreating the silent deactivation scenario on comparison failure.- The submitted control flow predates current
--branchsupport: it hard-codesmainathermes_cli/main.py:7310-7312, whereas current main resolves the update target athermes_cli/main.py:9562-9568. - The new tests cover a successful rebase (
tests/hermes_cli/test_update_autostash.py:555-581) but not the promised rebase-conflict recovery or failedgit cherrypath.
Suggested changes
- Model comparison failure explicitly and fail closed with recovery guidance.
- Integrate the reconciliation around current target-branch semantics and add the two failure-path tests.
Automated hermes-sweeper review.
| return _LocalPatchStatus(already_upstream=[], local_only=[]) | ||
|
|
||
| if result.returncode != 0: | ||
| return _LocalPatchStatus(already_upstream=[], local_only=[]) |
There was a problem hiding this comment.
A failed git cherry comparison is not equivalent to an empty comparison. This return flows through has_commits == False and leaves the update on main without a warning, recreating the silent deactivation case on any comparison error. Return an explicit unknown/error status and fail closed with recovery guidance.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Three PRs address the update path’s unsafe handling of committed local patches, but they cover two distinct checkout shapes: #4142 and #70953 protect commits carried directly on the update branch, while #22037 reconciles a separate local-only branch that the updater otherwise deactivates by switching to the update target and remaining there.
Related pull requests
- #4142 [closed]
related— (+57/-16) — superseded: This closed PR detects commits ahead on the update branch and rebases them instead of hard-resetting, but it lacks the durable backup, fail-closed inspection, configuration, and integration coverage in #70953; it remains relevant as the earlier implementation of the same core fix and was closed pending a fresh rebase. - #22037
related— (+283/-1) — keep open and revise: This is the only PR that directly addresses silent deactivation of a separate local-only patch branch by comparing patch IDs, restoring and rebasing still-local patches, and leaving upstream-equivalent patches behind. The contributor keep_open review must be resolved before merge: the current diff converts failedgit cherryexecution into an empty successful comparison, hard-codesmaininstead of honoring current--branchtarget resolution, and does not test comparison failure or rebase-conflict recovery. - #70953
related— (+760/-29) — complementary preferred implementation for update-branch divergence: This PR safely handles committed patches directly on the selected update branch through fail-closed inspection, default refusal or opt-in rebase, durable backup refs, verified abort recovery, and real-Git conflict tests. It substantially supersedes #4142, but it does not address #22037’s distinct case where patches live on a separate local-only branch that is switched away from before the pull.
Duplicates
#4142 and #70953 substantially overlap on preserving committed patches carried directly on the update branch; #70953 is the broader and safer successor. #22037 is not a full duplicate because it covers reconciliation of a separate local-only branch.
Suggested consolidation
Merge #22037 only after addressing the explicit contributor keep_open review by making git cherry failure fail closed, using the resolved --branch update target, and adding comparison-failure and rebase-conflict recovery tests; preferably reuse #70953’s durable-backup and verified-recovery mechanics. Close #4142 as superseded by #70953; do not close #70953 as a duplicate of #22037 because its update-branch safety scope remains distinct.
Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 61 kB of PR diffs, 6 kB of issue/PR text, 3 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Delta since our previous triage comment
@teknium1’s new review confirms that #70953 addresses a still-live destructive hard-reset path, while correcting our earlier assessment of its merge readiness: the updater has moved from hermes_cli/main.py to hermes_cli/update_cmd.py, and the config default has moved to hermes_cli/config_defaults.py. The review also confirms that #70953 remains complementary to #22037 rather than replacing its separate local-branch reconciliation scope.
Changed pull requests
- #70953
related— (+760/-29) — keep open and port before merge: The diff still provides the stronger update-branch reconciliation design—fail-closed refusal or backed-up rebase with verified conflict recovery—but, as @teknium1’s keep_open review identifies, it currently modifies obsolete implementation and config locations; it must be ported tohermes_cli/update_cmd.pyusing the_m()seam and tohermes_cli/config_defaults.py, then rerun its integration tests.
Suggested consolidation
The consolidation recommendation is unchanged: revise #22037 for its reviewed local-branch gaps, retain #70953 for the distinct update-branch fix after porting it to the live updater, and keep #4142 superseded by #70953.
Complex graph unchanged since our previous triage comment.
Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 61 kB of PR diffs, 6 kB of issue/PR text, 5 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
Teach
hermes updateto reconcile local fix branches against the updated upstreammain.When a user starts on a local branch, Hermes now uses
git cherry origin/main <branch>after fetch to compare commits by patch-id:mainmainhermes-update-snapshot/...safety branchmainupdated, and prints recovery guidance instead of leaving the install conflictedThis makes the common "I have a local hotfix; did upstream fix this yet?" flow less scary without using filename or keyword guesses.
Docs
Updated the update guide and CLI reference to document local branch reconciliation.
Tests run
/Users/niko/.hermes/hermes-agent-provider-routing/.venv/bin/pytest -q tests/hermes_cli/test_update_autostash.py- 27 passed/Users/niko/.hermes/hermes-agent-provider-routing/.venv/bin/python -m compileall hermes_cli/main.pygit diff --check