Repository navigation
Conversation
Related: #70585 also handles linked-worktree branch mismatches, but it rejects unsafe reuse whereas this PR realigns the checkout. These are competing safety policies, not duplicates. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address wrong-branch kanban worktrees: #37316 broadly added dispatch-time provisioning and corrected existing worktrees via checkout, while #71978 closes the narrower reuse gap in the current implementation by validating and realigning the branch before returning an existing linked worktree.
Related pull requests
- #37316 [closed]
related— (+328/-5) — superseded: This earlier implementation provisioned branch-specific worktrees and, contrary to a provisioning-only characterization, detected an existing checkout on the wrong branch and attempted to switch it before reuse. It remains relevant as a reference implementation, but the contributor states that merged #49855 superseded its broader worktree-resolution changes. - #71978
related— (+127/-1) — merge: The current reuse shortcut checks only repository identity; this diff additionally verifies the requested branch, performs a checkout or branch creation when stale, fails closed if realignment fails, and tests both direct and resolver-level reuse. The contributor notes that #70585 instead rejects mismatched reuse, but that is a competing safety policy rather than evidence that #71978 duplicates #37316 or #70585.
Suggested consolidation
Merge #71978 because its focused diff repairs the demonstrated stale-branch reuse path in the current post-#49855 implementation. Keep #37316 closed as superseded by merged #49855 rather than reopening it; its overlapping wrong-branch handling is useful historical evidence but is attached to the older, broader resolver implementation.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 24 kB of PR diffs, 7 kB of issue/PR text, <1 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Thanks for the focused real-git coverage. The wrong-branch condition is real on current main, but this patch does not cover the ordinary redispatch path. Problems
Suggested changes
Automated hermes-sweeper review. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Delta since our previous triage comment
Since our previous comment, the hermes-sweeper maintainer-bot keep_open review posted by @teknium1 identified an uncovered ordinary redispatch path: the persisted workspace enters the existing-linked-checkout resolver branch and bypasses the modified _ensure_git_worktree. This corrects our earlier merge recommendation because the added resolver test clears workspace_path and therefore does not exercise the persisted-path lifecycle that triggered the bug.
Changed pull requests
- #71978
related— (+127/-1) — keep open: The diff realigns stale branches inside_ensure_git_worktree, but the maintainer-bot keep_open review shows that ordinary redispatch with a persisted workspace returns earlier from_resolve_worktree_workspace, so the fix and tests do not yet cover the primary lifecycle path.
Suggested consolidation
Change the prior recommendation despite the merge-lane finding: keep #71978 open until mismatch handling covers the persisted existing-checkout resolver path, preserves the foreign-sibling behavior, and adds the requested lifecycle regression.
Complex graph unchanged since our previous triage comment.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 24 kB of PR diffs, 7 kB of issue/PR text, <1 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
db4a7af to
ebe16f1
Compare
|
Hi! This PR has been open for about five weeks without anyone taking a look yet. It's not a draft and currently shows as mergeable, so it should be straightforward to review. If someone has a spare moment, could you give it a quick look — or point me to the right person to tag? Happy to add context, rebase, or split it up if that would make review easier. Thanks! |
ebe16f1 to
56d0a8c
Compare
Reject mismatches in both helper reuse and persisted-path resolution without mutating occupied checkouts. Preserve same-branch reuse and foreign-sibling isolation. Verified with isolated scripts/run_tests.sh: baseline 2 passed; regression RED 3 passed/2 failed; final 5 passed. Disabling each guard separately failed only its corresponding regression.
56d0a8c to
8fa17c0
Compare
|
Rebased onto current |
hermes_cli/kanban_db_workspace.py: upstream changed the reuse guard in _ensure_git_worktree to compare common dirs with _path_key(); the PR added a branch-mismatch RuntimeError inside that guard. Kept upstream's condition and nested the PR's check inside it. The _resolve_worktree_workspace hunk merged cleanly (upstream still had the legacy reuse return).
fix(kanban): verify the branch when reusing an existing worktree checkout
Symptom
A kanban task with
workspace_kind=worktreewhose previous dispatch failed orwas interrupted runs its next attempt on the previous run's branch. The
worker cd's into the workspace and commits — the work lands on the wrong
branch, with no error and no warning.
Root cause
hermes_cli/kanban_db.py,_ensure_git_worktree:The reuse shortcut asks only "is this a linked worktree of the same repo?"
It never asks "is it on
branch_name?" — the one parameter the function wasgiven to guarantee.
A previous failed or interrupted dispatch of the same task leaves the canonical
<repo>/.worktrees/<task-id>on whatever branch it died on. The next dispatchof that task hits the shortcut and hands the worker a tree on the stale branch.
_resolve_worktree_workspace's per-task fallback (added for the sibling-sharingcase) cannot heal this: the fallback path it computes for a task is
<repo>/.worktrees/<task-id>— the very path that is stale. It correctlydetects a different task's branch occupying a shared path; it has no answer
for the task's own canonical path being stale, because there is nowhere else to
go. Realigning in
_ensure_git_worktreeis the only place that closes it.Fix
On reuse, compare the checkout's current branch to
branch_name. If itdiffers, switch it (
checkoutwhen the branch exists,checkout -bwhen itdoesn't) and raise if the switch fails, rather than proceeding on the wrong
tree. Same-branch reuse is an early
return, unchanged — the common pathissues no git command and touches no uncommitted work.
+31/−1, one function. No signature change, no new state.
Deliberately NOT changed
test_resolve_worktree_own_path_on_foreign_branch_keeps_legacy_reusepins thebehaviour where a task's explicitly stored
workspace_pathis a foreign-branchcheckout and the per-task fallback resolves to that same path:
mainkeeps thelegacy reuse rather than failing dispatch. An earlier draft of this fix also
realigned that case and turned that test red. That is a separate, deliberate
upstream decision about explicitly-configured paths, so it is left alone; this
PR changes only the reuse shortcut inside
_ensure_git_worktree, which everymaterialization path funnels through.
Tests
tests/hermes_cli/test_kanban_worktree_isolation.py— 4 new tests on thefile's existing real-git-repo helpers (no mocks; real
git init, realgit worktree add):checkout survives, proving no
checkoutis issued on the common path.checkoutvscheckout -bsplit; realigning onto an already-existing branch must check it out, not
fail trying to create a duplicate.
_resolve_worktree_workspace— a no-workspace_pathworktree task anchored on the board's
default_workdir, whose canonicalcheckout was left on
wt/interrupted-run, comes back onwt/<task-id>.This is the user-visible contract.
RED proof (reuse shortcut restored to the unconditional
return, testsunchanged): 3 of 4 fail. Test 2 is the control and passes in both states.
GREEN: 250 passed across
test_kanban_worktree_isolation.py,test_kanban_decompose_db.pyandtest_kanban_db.py. Zero pre-existing testsmodified.