Repository navigation
Conversation
ac318bc to
ed12707
Compare
|
Follow-up: addressed an independent review finding for The stale-registration concern remains intentionally scoped to #81336, which is the complementary missing-worktree recovery PR. |
ed12707 to
c0901b1
Compare
fix(kanban): block conflicting worktree branches
|
Enough1122
left a comment
There was a problem hiding this comment.
Logic checks out (porcelain parsing, same-path resolution with normcase, reuse gating, force_trip plumbing are all sound). Two coordination/robustness points (see inline):
| ) | ||
|
|
||
|
|
||
| def _validate_worktree_branch_available(target: Path, branch_name: str) -> None: |
There was a problem hiding this comment.
Two things worth coordinating with #81336 (also open, touching the same _ensure_git_worktree + git worktree list --porcelain parsing): (1) the two PRs will conflict on merge and interact semantically — #81336 re-creates a stale/missing registration (adding --force) while this PR rejects branch reuse, and its create_task preflight here will REJECT creating a task whose branch is still registered at a deleted worktree path (the exact stale case #81336 recovers). Worth cross-referencing so both land consistently. (2) _git_worktrees_for_branch swallows every subprocess/parse exception and returns [] — a fail-open, so any git error (binary missing, 30s timeout, non-repo) silently skips the deterministic rejection and a conflicting task still gets created at the create-time check (the dispatch-time check still catches it later). If the intent is terminal-by-first-dispatch, consider distinguishing 'no checkouts' from 'could not query git'.
|
Follow-up on the latest review:
Validation performed:
No full-suite run was performed; validation remains scoped to the affected files and review regressions. |
Thanks for the thorough follow-up — the shared |
What does this PR do?
This prevents Kanban worktree tasks from silently sharing or racing over the same Git branch. A task now fails at creation or dispatch when its requested branch is already checked out by another worktree, while intentional reuse is allowed only when
--workspacenames that exact linked worktree.The change also classifies branch-collision failures as terminal configuration errors instead of consuming generic worker/spawn retries.
Related Issue
Related to #81336, which handles missing or stale worktree paths. This PR is complementary: it handles a live checkout of the requested branch at a different path. It does not replace or close #81336.
Type of Change
Changes Made
hermes_cli/kanban_db.pygit worktree addto cover the TOCTOU window.hermes_cli/kanban.pyWorktreeBranchConflictErroras a terminal configuration failure in ready and review dispatch lanes.--branchCLI help text.tests/hermes_cli/test_kanban_db.pygit worktree add --forcecreates duplicate checkouts.How to Test
Run the focused regression tests:
scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py -q -k "create_worktree_task_rejects_branch_checked_out_elsewhere or create_worktree_task_allows_explicit_linked_worktree_reuse or rejects_reuse_when_branch_has_duplicate_checkout or create_worktree_task_rejects_primary_checkout_branch_reuse or dispatch_blocks_worktree_branch_collision_without_retry or dispatch_blocks_branch_collision_created_during_worktree_add"Result: 7 passed.
Run lint and syntax checks:
Result: both passed.
Run the affected test file:
On Windows 11 this currently reports 32 passed / 3 failed. The three failures predate this diff and concern raw wait-status decoding, Git porcelain slash normalization, and the Hermes executable fallback shim; none exercises the changed collision paths.
Verify the diff:
Result: passed.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass; the affected file has three pre-existing Windows failures documented aboveDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
No screenshot is applicable. Focused test, lint, syntax-check, and affected-file results are recorded above.