fix(worktree): never delete through a Windows junction (silent data loss)

[iscreamparis]

fix(worktree): never delete through a Windows junction

git worktree remove follows a Windows JUNCTION as if it were an ordinary
directory and deletes THROUGH it. On a real install (2026-09-06) this emptied
two source repositories that were never part of any worktree.

The chain, all of it ordinary-looking:

<worktree>/app/node_modules   --junction-->  <primary>/app/node_modules
<primary>/app/node_modules/@real3d/stage
                              --junction-->  ../../Stage        (a repo)
<primary>/app/node_modules/@real3d/atoms
                              --junction-->  ../../CRM_Atoms    (a repo)

A kanban worker needed node deps inside its worktree, so it junctioned
node_modules to the primary checkout. npm materialises file: dependencies
as further junctions on Windows. When the task completed, the dispatcher called
git worktree remove; the recursive delete walked junction -> junction -> repo
and emptied both. They were recoverable only because they had been pushed.

The failure is silent by construction. The single trace was

WARNING hermes_cli.kanban_db: git worktree remove failed for task
t_29590f63 at ...\.worktrees\t_29590f63: Invalid argument

— a warning about a worktree, saying nothing about the two repositories it
had just walked through. The user found out when an unrelated build failed.

Why no existing guard caught it

os.path.islink() returns False for a junction. Only NTFS symlinks
(IO_REPARSE_TAG_SYMLINK) set the flag Python reports; a junction
(IO_REPARSE_TAG_MOUNT_POINT) answers islink() == False and isdir() == True.
Every ordinary "don't follow links" check — in git, in shutil.rmtree, in our
own code — therefore sees a plain directory and descends. The only reliable
test is the raw FILE_ATTRIBUTE_REPARSE_POINT bit.

tests/cli/test_reparse_guard.py::test_junction_is_not_seen_by_islink pins
that asymmetry, so if CPython ever changes it the test says so.

The fix

New hermes_cli/reparse_guard.py: walk the tree (never descending INTO a
reparse point — that is the bug itself), unlink each link found, and report any
that resist. Unlinking removes the link, never its target: os.rmdir on a
directory reparse point detaches it without recursing. The target is never
read, resolved, or followed. No-op on non-Windows, where islink is honest and
no deleter follows symlinks.

Wired into all five git worktree remove call sites, each of which now
preserves the worktree rather than deleting through a link it cannot
unlink:

• hermes_cli/kanban_db_workspace.py — dispatcher cleanup (the incident)
• hermes_cli/worktree_ops.py — startup pruner, and failed-add sweep
  (which also has a shutil.rmtree fallback, equally affected)
• hermes_cli/cli.py — end-of-session -w cleanup
• hermes_cli/worktree_gc.py — attended hermes worktree prune

Preserving is the correct failure mode here: a stale worktree costs disk, and
deleting through a junction costs someone's source tree.

Verification

tests/cli/test_reparse_guard.py, 5 tests, Windows-only (skipped elsewhere).
They build the exact production shape — worktree junction -> node_modules
junction -> victim repo — and assert:

• test_git_worktree_remove_destroys_through_a_junction — the damage is real
  and it is git doing it (this test FAILS if git ever fixes the behaviour,
  which is the signal we'd want)
• test_guard_neutralizes_the_junction_and_saves_the_victim — with the guard,
  the victim repo and the junction's target both survive
• test_guard_unlinks_the_link_not_its_target
• test_find_never_descends_into_a_reparse_point
• test_junction_is_not_seen_by_islink

All 5 pass. The wider worktree/kanban-workspace suite is 95 passed; the 3
failures in TestPrMergedEscapeHatch / TestCronWorktreeMaintenance reproduce
identically on an unmodified checkout of the same commit, so they are
pre-existing and unrelated.

[andrexibiza]

Review of exact head c88ca7272829bd2be0d99c59beded1cb4007c3c5 — not an acceptance receipt. The incident/repro is important, but this implementation still has three correctness gates before it is safe to land.

1. Unknown filesystem state currently fails open. find_reparse_points() catches os.scandir, os.lstat/is_reparse_point, and entry.is_dir() OSErrors by skipping that subtree/entry. assert_safe_to_remove() can therefore return [] even though part of the deletion tree was never classified. The caller then proceeds to git worktree remove. On this safety boundary the state space has to be ordinary | reparse | unknown; unknown must refuse recursive deletion. A metadata/access failure can otherwise hide the exact junction class this guard exists to stop.

2. The scan and the recursive delete are two different generations. A reparse point created after assert_safe_to_remove() returns and before/during git worktree remove is still followable. The class invariant cannot be “there were no reparse points when we looked”; it must be “the deletion owner can only traverse ordinary edges for this deletion generation.” Fence all writers before the scan/delete pair or make the deletion itself reparse-aware, and add a deterministic race test that inserts a junction between inspection and removal.

3. The patch adds new cleanup policy to cli.py, which is still beyond the repository's 2K owner ceiling (the exact head has live code past line 2000). This cleanup seam should move to a bounded worktree-removal owner and have all callers consume it rather than adding another policy branch to the monolith.

The desired class repair is one shared deletion primitive with cleanup root × edge kind × physical target ownership × cleanup generation; any unclassified edge preserves the tree. Hosted checks on this head are also still action_required, so exact-object execution evidence remains outstanding.

[iscreamparis]

Thanks — all three gates were fair, and the first one was a real bug in the code I submitted. Rewritten from origin/main rather than patched; new head 4f7da23052b432f459851c477eb67fcd4190773f.

1. Unknown filesystem state failed open — correct, and it was the worst of the three.

find_reparse_points() swallowed OSError by skipping the entry, so assert_safe_to_remove() could return [] with part of the tree never classified — exactly the hiding place the guard exists to close. The state space is now ordinary | reparse | unknown:

• is_reparse_point() raises UnknownEdge instead of answering False when it cannot classify (a missing entry is still a definite False — absent is an answer, unreadable is not);
• any unknown edge aborts the whole removal, having deleted nothing further;
• the caller preserves the tree and logs the offending path with its cause.

Three tests pin it: test_unclassifiable_entry_refuses_the_whole_removal, test_unreadable_directory_refuses_rather_than_skipping, test_is_reparse_point_raises_on_an_unclassifiable_path.

2. Two generations — you were right, and your second option was the right one.

I took "make the deletion itself reparse-aware". reparse_guard.remove_tree() now performs the deletion, so the traversal that classifies an edge is the traversal that deletes it; git worktree remove is never asked to delete anything, since its walk is the unsafe part. Removal is: safe post-order delete, then git worktree prune for the dangling admin entry.

test_a_junction_created_mid_walk_is_still_not_followed is the deterministic race you asked for: it plants a junction after root has been enumerated once — i.e. after any pre-flight scan would have declared the tree clean — and asserts the victim survives.

I have not claimed the window is fully closed, and the module says so. What remains is in-process and one edge wide: between an entry's lstat and the scandir of that same directory. Closing that needs handle-based O_NOFOLLOW-style traversal, which CPython does not expose portably on Windows. I would rather state the residual than imply an invariant the code cannot hold. If you know the shape you'd want there, I'll take the pointer.

3. cli.py and the 2K ceiling — taken, and it improved the patch.

hermes_cli/worktree_removal.py is now the single removal owner: callers state policy (may I remove this, may I drop the branch), it owns the mechanics. cli.py loses its removal policy to it and is +5 lines net. Four other sites shrink as well.

Six call sites consume it, not five: kanban_db_workspace.py (the incident), worktree_ops.py (startup pruner + failed-add sweep, whose shutil.rmtree fallback had the same flaw), cli.py, worktree_gc.py, and web_git.py::worktree_remove — which is reachable from the dashboard, so the same traversal was pointable by an HTTP request.

That sixth one I had missed. test_no_caller_invokes_git_worktree_remove_directly found it — the ownership invariant is enforced by a test rather than documented, so the next one that appears fails CI and gets pointed at the module to use.

Evidence on this exact head: 11/11 in tests/cli/test_reparse_guard.py; 101 passed across the worktree/workspace/kanban/web_git selection; ruff check clean on all touched files. The 3 failures in TestPrMergedEscapeHatch / TestCronWorktreeMaintenance reproduce identically on an unmodified checkout of the same base commit (6d9c166455), so they are pre-existing and unrelated — I verified that on a separate clone rather than assuming it.

Hosted checks are still action_required and need a maintainer to authorise the run; happy to rebase or split if that helps them go green.

[andrexibiza]

Re-reviewed the current PR head 4f7da23052b432f459851c477eb67fcd4190773f against its exact base 693641aa8b4359c602283bdbbc14041e03bc47bc and current main at the same SHA. This is a follow-up to the prior review at c88ca7272829bd2be0d99c59beded1cb4007c3c5; I reconciled all three prior gates against the new tree.

Prior finding 1 — unknown filesystem state: CLOSED

The prior implementation treated OSError while classifying an entry as though the entry could safely be skipped. That allowed assert_safe_to_remove() to return an empty result while part of the deletion tree remained unclassified.

The new hermes_cli/reparse_guard.py makes the state space explicit:

• is_reparse_point() raises UnknownEdge for an lstat failure and only treats FileNotFoundError as a definite absent edge (hermes_cli/reparse_guard.py:87-105).
• remove_tree() propagates that refusal rather than silently continuing (hermes_cli/reparse_guard.py:123-175).
• remove_worktree() preserves the tree and reports the offending path when classification fails (hermes_cli/worktree_removal.py:91-104).

The new tests exercise unclassifiable entries, unreadable directories, and the raised UnknownEdge contract. This prior fail-open gate is closed.

Prior finding 2 — scan/delete generation gap: CLOSED

The prior design performed a safety scan and then handed the tree to git worktree remove, leaving a race in which a reparse point could be introduced between inspection and the recursive delete.

The new remove_tree() owns the deletion traversal itself. It classifies each child and immediately either descends into an ordinary directory, unlinks a reparse point, or refuses on an unknown edge. git worktree remove is no longer the deleter. The deterministic test_a_junction_created_mid_walk_is_still_not_followed regression covers a junction planted after the root enumeration and verifies the target survives.

The author explicitly retains the narrower in-process lstat→scandir residual instead of claiming a stronger guarantee than this implementation provides. That is an honest boundary, and the prior two-generation defect is closed.

Prior finding 3 — cli.py owner ceiling: CLOSED

The prior head added cleanup policy to cli.py while the file remained beyond the repository’s 2K owner ceiling. The current head moves physical removal into hermes_cli/worktree_removal.py, leaves cli.py at a net reduction, and routes the callers through one removal owner. The ownership tripwire test_no_caller_invokes_git_worktree_remove_directly checks that no caller bypasses that primitive.

New blocking finding — the dashboard force contract is silently discarded

hermes_cli/web_models.py:194-197 exposes GitWorktreeRemoveBody.force, and hermes_cli/web_routers/git.py:182-186 forwards the client’s boolean to hermes_cli.web_git.worktree_remove. The new hermes_cli/web_git.py:640-650 accepts that force argument but ignores it and always calls remove_worktree(_main_root(cwd), worktree_path) with the removal owner’s default behavior.

This is not cosmetic API drift. The old implementation passed the requested force flag to Git:

_git_ok(_main_root(cwd), ["worktree", "remove", *(["--force"] if force else []), worktree_path])

The new physical remover does not accept a force policy at all; it unconditionally deletes the supplied directory tree. A dashboard request with force=false therefore no longer preserves a dirty worktree. It deletes it.

I reproduced the changed behavior directly on the exact head with a temporary Git repository: created a linked worktree, wrote an uncommitted file into it, called the new removal path, and observed RemovalOutcome(removed=True) with both the worktree and the uncommitted file gone. The existing regression tests/hermes_cli/test_kanban_worktree_teardown.py::test_tree_dirtied_between_check_and_removal_preserved also fails on the PR head (1 failed, 19 passed in the focused selection) while the same test passes on unmodified current main.

The fix must preserve the API contract: force=false must refuse removal when the worktree is dirty, and force=true may remove it only after the same reparse-safe traversal. Thread the policy through the single removal owner and add a dashboard-path regression covering both values.

Verification

On the exact current head:

• tests/cli/test_reparse_guard.py: 11 passed
• Focused worktree safety selection: 19 passed, 1 failed — the failure is the dirty-tree preservation regression above
• py_compile passed for all touched Python files
• git diff --check passed
• Current GitHub state is MERGEABLE but BLOCKED; the head has zero completed checks because the workflow still requires maintainer approval

Landing order and repository-object graph

The PR is directly based on current main (693641aa8b4359c602283bdbbc14041e03bc47bc), so there is no stale-base sequencing issue. The relevant landing object is this PR itself; the prior review is the parent review state, not a separate implementation dependency. The dashboard route is part of the same deletion-safety surface and must be repaired in this PR rather than treated as a follow-up.

[andrexibiza]

Re-reviewing exact head 4f7da23052b432f459851c477eb67fcd4190773f. This is materially new context beyond the earlier current-head issue-thread note: I rechecked the actual commit graph, the centralized removal sink, the dashboard contract, the existing Kanban teardown regressions, and the Windows reparse traversal itself.

The rewrite did close important parts of the first review: UnknownEdge is a real fail-closed classification state, physical deletion now has a bounded owner, direct git worktree remove callers are fenced by a test, and the new Windows-shaped coverage is useful. That is good work. The remaining blockers are at the mutation/settlement boundary rather than the original broad design.

1. Blocking — removal authority is still checked too early, and force is lost at the sink

The dashboard route still carries GitWorktreeRemoveBody.force into web_git.worktree_remove(cwd, worktree_path, force), but this head discards that boolean and calls remove_worktree(...) unconditionally. The central owner’s own module doc says callers state “may I remove this, and may I force,” but the function has no force/dirty contract at all.

That converts the old non-force behavior into effectively forced physical deletion. It also regresses the other side of the same invariant in Kanban: current main has test_tree_dirtied_between_check_and_removal_preserved, specifically because _worktree_is_dirty() is only a pre-check and Git’s non-force remove used to revalidate at the mutation sink. On this head, a worktree that becomes dirty after that pre-check is handed to remove_tree() and its new work is deleted.

This class already has repository history: #28818 / merged #31708 established that cleanup metadata is not deletion authority, and #33774 established that workspace lifecycle has to preserve data while another owner may still need it. This PR is the current canonical carrier for the junction-specific physical-removal repair; I did not find a competing open junction/reparse worktree-removal PR, so this should be fixed here rather than split into another carrier.

Required shape: thread explicit force policy into the single owner and validate current dirty state at the effect boundary. force=False must refuse a dirty worktree; force=True may proceed through the same reparse-safe walker. Keep/add regressions for dashboard true/false and for the existing “dirtied after pre-check” Kanban race.

2. Blocking — an already-classified directory can still be swapped to a junction before its deferred scandir

remove_tree() classifies entry_path, then for an ordinary directory pushes (entry_path, False) onto stack. When that directory is popped later, it goes straight to os.scandir(current) without reclassifying current.

So the residual is wider than the documented one-edge lstat→scandir window. A directory can be classified ordinary, wait arbitrarily behind other stack work, be replaced by a junction, and then be traversed by scandir with no new reparse check. The deterministic race test on this head plants a new child junction after root enumeration; that child is freshly classified when encountered, so it does not exercise this replacement shape.

At minimum, reclassify current immediately before every descent and add the adversarial regression that replaces an already-classified directory with a junction after classification but before its scandir; the victim target must survive. If the remaining immediate lstat→scandir race cannot be closed portably on Windows, keep that narrower residual explicit rather than claiming the deferred-directory generation is covered.

3. Blocking settlement/receipt mismatch — unknown can arrive after destructive progress, but the API reports “preserved” semantics

The docs say unknown “aborts the whole removal” and callers preserve the tree. Mechanically, ordinary files are unlinked as traversal proceeds. If a later sibling/subtree raises UnknownEdge, earlier entries are already gone. remove_worktree() then returns RemovalOutcome(False, ...), and callers log that the worktree was preserved even though it may now be partially deleted.

This needs a typed distinction between refusal-before-mutation and partial/indeterminate settlement (or a traversal design that can actually guarantee zero prior mutation on refusal). Add a test where a removable sibling is deleted before a later injected unknown; the result must not claim an intact preserved worktree.

There is a similar smaller receipt bug in worktree_removal.py: _git() uses check=False, but unlock/prune/branch cleanup only catches exceptions and never inspects returncode. A nonzero git worktree prune can therefore leave admin state behind while remove_worktree() still returns removed=True. Physical-tree removal plus metadata-cleanup failure is partial success, not a clean success receipt.

Exact object / landing state

The PR’s base ref points at current main 693641aa8b4359c602283bdbbc14041e03bc47bc, but the reviewed commit’s parent/merge-base is 6d9c16645599122d2dbac7ae689b6f73c4cd5fda. The actual compare is 1 ahead / 64 behind, not direct-current-main. None of the 64 intervening main commits touches this PR’s eight files, so I do not see a direct file-conflict interlock, but the branch still needs current-main replay/rebase before exact landing proof.

Fresh exact-head hosted state is also not an acceptance receipt: CI 34066105801, Docker 34066104966, and Nix 34066104976 are all completed action_required. This one-commit train is therefore 0/1 hosted-green. Local Windows-specific evidence is valuable, but it cannot substitute for exact-head hosted acceptance under the repository’s every-commit-green rule.

The centralization is the right direction, and the response to the first review materially improved the code. Once force/dirty authority is consumed at the real deletion sink, the deferred-directory replacement race is pinned, partial settlement is represented truthfully, and the rebased exact head is green, this becomes a much stronger safety boundary for every caller rather than only a fix for the original incident.

[andrexibiza]

force is now accepted by this public/dashboard-facing function but discarded here. Because remove_worktree() has no dirty/force policy, force=False no longer means “Git must refuse a dirty worktree”; it means the custom walker deletes it anyway. Please carry this authority into the single removal owner and revalidate dirtiness at the mutation sink. The existing Kanban TOCTOU regression (test_tree_dirtied_between_check_and_removal_preserved) is the same missing sink-time check from another caller.

[andrexibiza]

Request changes: the new removal owner closes the original junction traversal path, but it currently regresses dirty-worktree authority, retains a reachable replacement race, and reports destructive or incomplete settlement as clean/preserved.

Blocking findings

• hermes_cli/web_git.py:640-650 accepts force but discards it at line 647. The centralized owner at hermes_cli/worktree_removal.py:63-119 has no force or dirty-state contract. A dashboard request with force=false, or a worktree dirtied after the Kanban pre-check, can therefore delete the dirty tree. Carry explicit force authority into the effect boundary and revalidate current dirtiness there.

• hermes_cli/reparse_guard.py:151-172 can traverse a directory replaced with a junction after it was classified. The directory is queued as ordinary, then later passed to os.scandir(current) without reclassification. The added race test plants a new child before classification; it does not cover replacement of an already-classified directory. Reclassify immediately before every descent and add that regression.

• hermes_cli/reparse_guard.py:161-175 mutates ordinary siblings before a later UnknownEdge aborts. hermes_cli/worktree_removal.py:91-104 returns RemovalOutcome(False) and callers describe the tree as preserved even though earlier entries may already be deleted. Refusal-before-mutation and partial/indeterminate settlement need distinct outcomes, or the walk must guarantee refusal before mutation.

• hermes_cli/worktree_removal.py:55-60,106-119 ignores nonzero return codes from git worktree prune and git branch -D, returning RemovalOutcome(True) even when administrative cleanup failed. Report metadata-cleanup failure as partial settlement.

• tools/subagent_worktree.py:168-175 still executes git worktree remove --force directly. The new ownership test scans only hermes_cli/**/*.py and cli.py, so this production cleanup path remains outside the reparse-safe owner. Route it through the shared removal owner and extend the behavioral coverage.

• cli.py remains 4,665 physical lines, and this PR adds cleanup policy in that owner. Keep the policy in the bounded removal owner rather than extending the over-ceiling godfile.

Evidence

• Exact head reviewed: 4f7da23052b432f459851c477eb67fcd4190773f.
• Direct Windows execution reproduced the dirty-worktree deletion, the classified-directory junction replacement, partial deletion after UnknownEdge, and success receipts after forced Git return-code failures.
• tests/cli/test_reparse_guard.py: 11 passed.
• The Kanban teardown suite: 10 passed, 1 failed — test_tree_dirtied_between_check_and_removal_preserved.
• Current main is 693641aa8b4359c602283bdbbc14041e03bc47bc; this head is 1 commit ahead and 64 behind it. Exact-head CI, Docker, and Nix are completed ACTION_REQUIRED, not hosted-green acceptance.

[andrexibiza]

Follow-up on unchanged head 4f7da23052b432f459851c477eb67fcd4190773f: two additional P1 data-loss findings, beyond the existing review and inline threads. Those findings are not being closed or duplicated here.

Moving the physical deletion into one bounded owner gives these checks one place to live. However, replacing Git's deletion also removed its target-identity and lock checks. Neither regression requires a junction or a race.

P1 — an existing path is not authority to delete a worktree

hermes_cli/worktree_removal.py:81-93 never establishes that worktree_path is a registered linked worktree belonging to repo_root. The failed worktree unlock result does not stop deletion. The dashboard now reaches that sink at hermes_cli/web_git.py:647; its route at hermes_cli/web_routers/git.py:182-186 normalizes the two paths independently. _fs_path() resolves a path, but does not establish worktree membership.

In real disposable repositories, the old git worktree remove --force <target> refused each of these targets with exit 128 and left its sentinel intact: an unregistered directory, the main checkout itself, and a linked worktree owned by another repository. Calling the new owner with the same arguments deleted the target and returned RemovalOutcome(removed=True, reason='') in all three cases. The main-checkout case also removed its .git directory.

Validate the exact registered linked-worktree identity and common Git directory before any unlock or filesystem mutation, and always reject the main checkout and foreign/unregistered targets on the normal removal path. force must not override that ownership boundary. Preserve failed-worktree add recovery through explicit ownership of that creation attempt's partial artifacts; an arbitrary existing directory cannot serve as its substitute.

P1 — dashboard removal now silently overrides worktree locks

remove_worktree(..., unlock=True) unconditionally unlocks at worktree_removal.py:83-87. The dashboard supplies no explicit unlock authorization. Previously its boolean force could add only one --force; Git still refused a locked worktree. In a clean, locked linked-worktree fixture, that old command returned 128 and preserved the files. The new owner unlocked it, deleted it, and returned success.

Keep lock state and unlock authorization separate from dirty-state force. Normal dashboard removal must preserve a locked worktree unless an explicit supported policy authorizes unlocking it. Internal stale/failed-creation cleanup can pass its own bounded authorization. Merely setting unlock=False is insufficient: the custom walker ignores Git's administrative lock, so the lock must be checked before physical deletion.

Execution evidence

Fresh Linux fixtures, Python 3.13.5, Git 2.47.3; real Git commands and real filesystem effects. The two executed production files were verified against this head's Git blob IDs:

• reparse_guard.py: 66dbdf8db00b5f80b2c823653894cff235aa3490
• worktree_removal.py: df60673b797073cf9c8d3b23d6af2bdeb8849a78

Safety case / control	Observed result
Unregistered directory	Git refused; new owner deleted it and reported success
Main checkout	Git refused; new owner deleted it and reported success
Another repository's linked worktree	Git refused; new owner deleted it and reported success
Locked linked worktree	Git refused with one force flag; new owner unlocked/deleted it
Clean owned linked worktree	Correctly removed; main checkout retained; registration pruned
Stationary POSIX symlink	Link removed; external target content retained

Reviewer regression harness: 4 failed safety assertions, 2 passed controls, exit 1. Command: PYTHONPATH=$PWD python -m pytest test_removal_authority.py -q -s --basetemp=/mnt/data/pr104535_review/tmp_authority_1 in the isolated source directory. This is sink-level execution plus source tracing of the dashboard route, not a full HTTP, full-repository-suite, or native-Windows test receipt.

Exact landing state

Still open/non-draft and mergeable. Current main/base ref is 693641aa8b4359c602283bdbbc14041e03bc47bc; the actual merge base is 6d9c16645599122d2dbac7ae689b6f73c4cd5fda, 1 ahead / 64 behind. Exact-head CI, Docker, and Nix are all action_required: 0/1 PR commits hosted-green, not acceptance.

These two authority checks belong in this PR's shared removal owner, alongside the already-open dirty/force, traversal, and settlement corrections—not in another competing remover.

Disposable-fixture regression harness used for the six cases

Save as test_removal_authority.py with the reviewed source on PYTHONPATH. All deletion targets are under pytest temporary directories.

"""Review regressions; every filesystem mutation is confined to pytest tmp_path.

Uses the exact Git-blob-verified production modules from head 4f7da230.
No source changes, no network, and no user repositories are touched.
"""
import os
import subprocess
from pathlib import Path

import pytest
from hermes_cli.worktree_removal import remove_worktree


def git(repo: Path, *args: str, check=True):
    return subprocess.run(['git', '-C', str(repo), *args], capture_output=True,
                          text=True, check=check, timeout=10)


def init_repo(path: Path):
    path.mkdir()
    git(path, 'init', '-q')
    # Real tracked content, not an empty proof commit; disposable fixture only.
    (path / 'sentinel.txt').write_text('must survive invalid removal\n')
    git(path, 'add', 'sentinel.txt')
    git(path, '-c', 'user.name=Review fixture', '-c', 'user.email=fixture@example.invalid',
        'commit', '-qm', 'fixture content')
    return path


@pytest.mark.parametrize('target_kind', ['unregistered', 'main', 'other_repo_worktree'])
def test_reject_foreign_or_main_root_before_deletion(tmp_path, target_kind):
    repo = init_repo(tmp_path / 'main')
    if target_kind == 'main':
        target = repo
    elif target_kind == 'unregistered':
        target = tmp_path / 'unrelated_directory'
        target.mkdir()
        (target / 'sentinel.txt').write_text('outside the worktree namespace\n')
    else:
        owner = init_repo(tmp_path / 'other_repo')
        target = tmp_path / 'foreign_linked_worktree'
        git(owner, 'worktree', 'add', '-qb', 'foreign', str(target))
    sentinel = target / 'sentinel.txt'
    before = sentinel.read_bytes()
    control = git(repo, 'worktree', 'remove', '--force', str(target), check=False)
    assert control.returncode != 0
    assert sentinel.read_bytes() == before
    result = remove_worktree(str(repo), target)
    print(f'CASE={target_kind} git_returncode={control.returncode} git_stderr={control.stderr.strip()!r} '
          f'outcome={result!r} target_exists={target.exists()} sentinel_exists={sentinel.exists()}')
    assert not result, 'Removal of a non-owned linked worktree must refuse before effects'
    assert sentinel.read_bytes() == before


def test_locked_linked_worktree_requires_explicit_unlock_authority(tmp_path):
    repo = init_repo(tmp_path / 'main')
    target = tmp_path / 'locked_worktree'
    git(repo, 'worktree', 'add', '-qb', 'locked', str(target))
    git(repo, 'worktree', 'lock', '--reason', 'still owned', str(target))
    control = git(repo, 'worktree', 'remove', '--force', str(target), check=False)
    assert control.returncode != 0
    assert (target / 'sentinel.txt').exists()
    result = remove_worktree(str(repo), target)
    print(f'CASE=locked git_returncode={control.returncode} outcome={result!r} '
          f'target_exists={target.exists()}')
    assert not result, 'Dashboard had no unlock authority; a single force flag cannot override a lock'


def test_clean_owned_linked_worktree_positive_control(tmp_path):
    repo = init_repo(tmp_path / 'main')
    target = tmp_path / 'owned_worktree'
    git(repo, 'worktree', 'add', '-qb', 'owned', str(target))
    result = remove_worktree(str(repo), target)
    assert result
    assert not target.exists()
    assert (repo / 'sentinel.txt').exists()
    assert str(target) not in git(repo, 'worktree', 'list', '--porcelain').stdout


def test_nonmoving_symlink_target_positive_control(tmp_path):
    repo = init_repo(tmp_path / 'main')
    target = tmp_path / 'owned_worktree'
    git(repo, 'worktree', 'add', '-qb', 'owned', str(target))
    victim = tmp_path / 'external'
    victim.mkdir()
    (victim / 'sentinel.txt').write_text('not owned\n')
    (target / 'external_link').symlink_to(victim, target_is_directory=True)
    result = remove_worktree(str(repo), target)
    assert result
    assert (victim / 'sentinel.txt').read_text() == 'not owned\n'

[andrexibiza]

[P1] Validate registered linked-worktree identity before deleting this path. This owner only checks that the supplied path exists. It never verifies membership in repo_root's worktrees or rejects the primary checkout. The dashboard now forwards its independently normalized worktreePath directly here, and a failed git worktree unlock does not stop this call.

On this exact source, real disposable Git fixtures showed that an unrelated directory, the primary checkout (including .git), and another repository's linked worktree are all deleted with RemovalOutcome(True). The old git worktree remove --force refused each target with exit 128 and preserved its sentinel. This is distinct from the already-reported dirty/force regression: no amount of force should turn a foreign path or the main checkout into an owned linked worktree.

Establish the exact linked-worktree/common-Git-directory identity before any unlock or deletion. Handle genuinely owned failed-add partial artifacts with an explicit creation-attempt ownership contract, not this unrestricted normal removal path. The review body contains the three-case reproducer and results.

[andrexibiza]

[P1] Preserve worktree locks unless this caller explicitly owns the unlock. The new default unlock=True is inherited by web_git.worktree_remove, so an ordinary dashboard remove now silently unlocks a worktree before deleting it. Previously that route could supply at most one --force; Git still refused a locked worktree.

Reproduced with a clean linked worktree locked using git worktree lock --reason 'still owned': the old removal command exited 128 and retained the file; the new owner unlocked/deleted the tree and reported success. This needs its own lock/unlock policy, separate from the dirty/force flag. Normal dashboard removal must refuse a locked tree unless explicit unlock authority exists; internal stale/failed-creation cleanup can pass bounded authorization. Simply switching this argument to False does not enforce the lock, because remove_tree() bypasses Git's lock checks entirely.

[andrexibiza]

The prior submitted review objects were the wrong publication surface and are being superseded. This is the ordinary PR summary for the exact head 4f7da23052b432f459851c477eb67fcd4190773f; the line-attached findings are on the changed-file review threads.

Blocking findings

• The dashboard force contract is discarded at hermes_cli/web_git.py:647; the central owner has no sink-time dirty-state policy. force=false can delete a dirty worktree, and the Kanban dirty-after-precheck regression fails.
• The central owner accepts any existing path without proving it is the registered linked worktree belonging to repo_root; the main checkout, an unregistered directory, and a foreign repository worktree can be deleted.
• The default unlock=True silently overrides worktree locks before deletion; lock authority must remain distinct from dirty-state force.
• The walker can traverse a directory replaced by a junction after classification, and does not guard reparse-point ancestors.
• UnknownEdge can arrive after earlier siblings were deleted, while callers report preservation; partial settlement needs a distinct outcome.
• Nonzero git worktree prune and branch-delete results are ignored while removal is reported successful.
• tools/subagent_worktree.py:171-175 still directly invokes git worktree remove --force, leaving a production path outside the shared safe owner.

Evidence

• Exact head: 4f7da23052b432f459851c477eb67fcd4190773f.
• Direct Windows probes reproduced dirty-worktree deletion, replacement-race target deletion, partial deletion after UnknownEdge, and false success after forced Git return-code failures.
• tests/cli/test_reparse_guard.py: 11 passed.
• Kanban teardown: 10 passed, 1 failed — test_tree_dirtied_between_check_and_removal_preserved.
• Current GitHub state: open, mergeable, BLOCKED; exact-head hosted checks are ACTION_REQUIRED, not acceptance-green.

The PR remains non-acceptable until the shared owner consumes deletion authority and ownership, every cleanup caller uses it, settlement receipts are truthful, and the exact head is reverified.

[andrexibiza]

Full re-review — exact head 4f7da23052b432f459851c477eb67fcd4190773f

The explicit UnknownEdge classification and the bounded removal owner are useful improvements over the original pre-scan. Keep those improvements. The remaining problem is that replacing Git's removal operation also replaces its authorization, target selection and settlement behavior; the current replacement does not preserve those contracts.

I re-read the eight-file diff, the removal implementations, all six migrated call sites, the remaining subagent/Desktop paths, the tests and the existing discussion. I also ran fresh disposable filesystem/Git reproductions against the exact production module bytes. This is not an unchanged-SHA status check, and the execution claims below are new Linux results, not a repetition of the earlier Windows claims.

Disposition: the data-loss blockers remain. This pass additionally establishes a parent-replacement escape after enumeration, unrelated-worktree index deletion by repository-wide pruning, an uncovered native Desktop removal path, a false-positive race test, and a live merge interlock with #104226. The existing force, target-identity and lock threads remain applicable; I have not duplicated those inline threads or marked them resolved.

P1 — validate the exact owned linked worktree before any mutation

At worktree_removal.py:81–93, an existing path is enough to reach physical deletion. A failed worktree unlock does not stop it, and no registered-worktree/common-Git-directory check replaces Git's check.

Fresh real-Git controls refused three targets with exit 128 and left their sentinels intact: an unregistered directory, the main checkout itself, and a linked worktree belonging to another repository. The new owner deleted each target and returned RemovalOutcome(removed=True, reason=''). The main-checkout case also loses .git.

The dashboard route normalizes the repository and target paths independently, then reaches this owner. Path normalization is not proof that the second path is a linked worktree owned by the first repository. Validate that exact relationship, reject the main checkout and foreign/unregistered targets, and bind any requested branch deletion to the admitted worktree. force must not override target ownership. Failed-worktree add recovery needs explicit ownership of that creation attempt's partial artifacts, not permission to delete an arbitrary pre-existing directory.

P1 — restore the non-force dirty-tree contract at the removal boundary

web_git.py:640–650 still accepts force but never passes or consumes it. The owner has no dirty/force argument or check.

Fresh fixtures covered both modified tracked content and a new untracked file. Non-force git worktree remove refused each with exit 128; the replacement owner deleted each and returned success. This confirms the existing dashboard finding. The Kanban pre-check remains insufficient too: the old non-force Git removal provided a second dirty-state check, and this patch removes it. The existing test_tree_dirtied_between_check_and_removal_preserved is still a required caller regression; I am not claiming a fresh run of that full suite here.

Thread explicit force policy into the shared owner, default it to non-force, and consume current dirty-state authority at the effect boundary under the cleanup lifecycle's actual exclusion/fencing. Preserve intentionally forced internal cleanup as explicit caller policy; do not make every caller effectively forced.

P1 — a Git lock is not removed merely by omitting the unlock command

At worktree_removal.py:83–87, unlock=True silently supplies authorization that the dashboard never requested. Conversely, unlock=False skips the command but does not prevent the custom walker from deleting a locked tree.

I tested both values on clean, locked linked worktrees. Git's previous single---force command refused with exit 128. The replacement deleted the tree and returned success in both cases. Keep lock state and authorization to release that lock separate from dirty-state force. A normal removal must enforce the lock before touching files; internal dead-owner/failed-creation cleanup can supply its own bounded authorization.

P1 — the escape is not limited to a queued directory or an immediate lstat→scandir interval

The previously reported queued-directory replacement still reproduces: classify a_pending as ordinary, replace it with an external symlink while another root entry is processed, and its later scandir follows the replacement. External content is deleted.

The additional case is a parent replacement after enumeration. At reparse_guard.py:152–173, child operations use Path(entry.path). After scandir(root) has produced its entries, rename that root aside and replace its original pathname with a symlink to another directory containing a matching filename. The child's final component is still an ordinary file, so the fresh child lstat does not detect that its ancestor changed. _unlink_edge(entry_path) deletes the external file. The candidate completed successfully in this fixture.

This reproduced on Linux, with no Windows junction and no timing-dependent thread race. Under the same replacement schedule, the platform's fd-based shutil.rmtree preserved the external file and refused the final substituted-root removal. The compact regression below includes that control.

Consequently, adding another is_reparse_point(current) immediately before scandir is not a complete fix: the parent can change after that check/enumeration. Bind traversal and child deletion to retained directory identity using fd/handle-relative no-follow operations, or establish an enforceable exclusion fence covering namespace replacement for the entire removal. A same-process/same-pass walk is not such a fence. The dashboard path also does not establish the module docstring's assumption that the owning writer is dead.

P1 — single-worktree removal now prunes another worktree's private index

worktree_removal.py:106–109 describes dropping the admin entry, but invokes repository-wide git worktree prune without a target.

Fresh fixture: create two linked worktrees, stage content in the second that differs from its working copy, and temporarily move that second worktree aside without repairing its registration. Remove only the first worktree. The new owner deletes the second worktree's .git/worktrees/<id>/index and registration as collateral cleanup. The old targeted git worktree remove <first> leaves that private index intact. Both branches are exercised below.

This is loss of another worktree's index state and registration; I am not claiming its dangling blob is immediately removed from the object database. It nevertheless exceeds the requested mutation scope. Some pre-existing sweep callers already ran a global prune; introducing it inside the shared single-target owner also gives that broader effect to dashboard, ordinary CLI and Kanban removals that did not request a repository-wide sweep.

Restrict administrative settlement to the exact admitted worktree under the repository metadata's coordination boundary. Repository-wide pruning must remain a separately authorized operation. Successful physical deletion of one tree is not authority to garbage-collect every absent registration.

P1 scope-completion gate — native Desktop and subagent cleanup still bypass the repair

Two existing production paths remain outside this owner:

• tools/subagent_worktree.py:168–175 directly runs git worktree remove --force after its clean/zero-commit probes. This was identified previously and remains unchanged.
• apps/desktop/electron/git-worktree-ops.ts:350–364 still constructs and executes ['worktree', 'remove', ...]. It is wired to hermes:git:worktreeRemove in git-ipc.ts. Repairing the Python remote-dashboard endpoint does not repair this local Electron path.

These are uncovered existing paths, not newly introduced commands in this diff. The new ownership test scans only hermes_cli/**/*.py and root cli.py, so it cannot establish the repository-wide claim. Route every production removal through the same safety contract, including the native-host boundary, and add behavioral entrypoint coverage. Do not simply copy an independent deletion policy into Electron or count a Python-only text scan as proof that the Desktop path is covered.

P2 — distinguish pre-effect refusal, partial deletion and incomplete metadata settlement

The fresh late-UnknownEdge fixture deleted an earlier sibling and the worktree's .git link file, then returned RemovalOutcome(False, ...) after a later classification failure. The caller-facing "Keeping/Preserving" wording and the comment at worktree_removal.py:95–96 do not describe an intact tree. Retaining the remaining directory is not equivalent to preserving its prior contents.

Separately, injecting nonzero return codes for worktree prune and branch -D produced RemovalOutcome(True, reason='') while the registration or branch remained. A dangling root link also survives the wt.exists() guard while the method reports it removed.

Represent physical progress and metadata settlement separately, with the affected paths and remaining obligations. Do not turn a metadata failure into a generic "preserved" result after successful deletion, or discard the failure because subprocess.run(check=False) did not raise. Inspect return codes, and classify the root with no-follow/entry-existence semantics rather than using target-following exists() as the entire root gate.

P2 — the new race regression can pass without creating a junction

At test_reparse_guard.py:258–270, planted['done'] is set before mklink, the return code is ignored, and the assertion checks only that boolean.

I invoked this test function directly with subprocess.run forced to return exit 1, creating no link. The test returned normally. A separate mutation assertion requiring it to reject that setup failed. This tests the test's assertion logic; it is not native-Windows execution.

Require successful creation and verify that the planted entry actually is a reparse point before counting the race as exercised. Add both replacement shapes above, not only a newly created child. Keep any demonstration that a particular Git version is vulnerable separate from a permanent safety gate that requires future Git versions to continue deleting the victim.

Live graph and landing interlock

Observed upstream main: 08b140d14e6c1d49f9b7ad02c9437fe940d54d65. Actual merge base: 6d9c16645599122d2dbac7ae689b6f73c4cd5fda. The exact compare is 1 ahead / 66 behind. None of those 66 intervening main commits changes this PR's eight paths. That is not a current-main execution receipt, and the earlier description that this head was directly based on current main was incorrect.

There is now a concrete open-PR interlock: #104226, observed head a310e5643eefa6523a1bfaeb6559fd1737486f5a, changes the same hermes_cli/kanban_db_workspace.py and adds _discard_unseeded_worktree, which directly calls git worktree remove --force after a worktree-created hook blocks. Its rollback is precisely a path where a seeding hook may have left links behind. This is a landing/composition requirement, not code already present in #104535's reviewed head.

Keep #104535 as the bounded removal repair and make #104226 consume that corrected contract, retaining its explicit failed-creation discard authority and branch-preservation semantics. The combined tree must exercise failed seeding with links and cleanup failure/retry behavior. Neither landing order may silently reintroduce the direct deleter or bypass hook admission on retry. No third removal-policy carrier is needed.

Structural accounting also needs to remain accurate: the new owners are 191 and 119 lines, but root cli.py remains 4,665 lines and this diff is +5 net, not a net reduction. Centralization is real progress; it is not a pass of the previously raised 2K ceiling. Keep the remaining cleanup/reporting behavior out of that over-ceiling owner rather than claiming the structural gate is closed.

The PR description also needs to match this head: the remover is no longer a Windows-only pre-scan/no-op on POSIX, there are 11 tests, and modern shutil.rmtree is not correctly described as always descending into stationary Windows junctions. CPython documents the junction behavior change since 3.8 and the fd-based attack-resistant implementation where supported. That is not a claim that every Windows traversal race is solved by using shutil.

Fresh execution receipts and limits

Environment: Linux, Python 3.13.5, Git 2.47.3, pytest 9.0.2. All filesystem mutations occurred in disposable fixtures. The complete production modules were copied from the pinned head and checked with git hash-object before/after execution:

hermes_cli/reparse_guard.py       66dbdf8db00b5f80b2c823653894cff235aa3490
hermes_cli/worktree_removal.py    df60673b797073cf9c8d3b23d6af2bdeb8849a78
tests/cli/test_reparse_guard.py   bfb4d10b8dba4dd7ddf67d4377656c666223168a

• Independent 20-case safety harness, run twice in fresh fixtures: 14 failed, 6 passed on both runs; exit 1. Failures cover the three wrong targets, two dirty states, both lock argument values, both replacement races, late unknown, two metadata-return-code failures, unrelated-index pruning and dangling-root settlement. These are 14 failing safety cases, not 14 distinct bug classes. Passing controls include clean owned removal, a stationary external link, pre-effect unknown refusal, absent root, targeted Git metadata cleanup and fd-based ancestor-replacement handling.
• The PR's original test file, -k 'not no_caller': 5 passed, 5 Windows-only skipped, 1 deselected; exit 0. The ownership scan was deliberately deselected because the local execution snapshot was not a complete repository checkout; a scan over missing callers would be a vacuous pass. Caller coverage above comes from the live pinned source inspection instead.
• Race-test setup mutation: 1 failed; exit 1, proving the original test accepts failed junction creation.
• Compact executable regressions below: 2 failed, 2 passed; exit 1. These repeat the two new destructive cases with controls; they are not additional independent findings.
• py_compile passed for the two complete production modules and the original test file. No native-Windows, Electron end-to-end, full Kanban-suite or repository-wide test execution is claimed for this pass.

The hosted state was refreshed before publication. Exact-head CI, Docker Build, Test, and Publish, and Nix flake check are all completed action_required, not green. This one-commit PR is 0/1 hosted-green. Local controls do not replace native-Windows and exact-current-head acceptance.

Executable regressions for ancestor replacement and unrelated-index pruning (2 candidate failures / 2 controls passed)

Save this as a scratch test outside the repository, point PYTHONPATH at an isolated checkout of the reviewed head, and run python -m pytest /path/to/test_new_boundaries.py -q. This uses only temporary repositories and files. The ancestor control requires a platform where shutil.rmtree.avoids_symlink_attacks is true; the results above were obtained on Linux.

"""Disposable, executable regression cases for PR #104535's new review findings."""
import os
import shutil
import subprocess
from pathlib import Path

import pytest
from hermes_cli import reparse_guard as rg
from hermes_cli.worktree_removal import remove_worktree


def git(repo, *args):
    return subprocess.run(["git", "-C", str(repo), *args], check=True,
                          capture_output=True, text=True, timeout=10)


@pytest.mark.parametrize("use_candidate", [True, False])
def test_enumerated_parent_cannot_retarget_child_deletion(tmp_path, monkeypatch, use_candidate):
    root, victim, saved = (tmp_path / n for n in ("root", "victim", "saved"))
    root.mkdir()
    victim.mkdir()
    (root / "same.txt").write_text("original")
    sentinel = victim / "same.txt"
    sentinel.write_text("external")
    inode, real_scan, swapped = root.stat().st_ino, os.scandir, False

    class Scan:
        def __init__(self, entries):
            self.entries = entries
        def __iter__(self):
            return iter(self.entries)
        def __enter__(self):
            return iter(self.entries)
        def __exit__(self, *args):
            return False

    def scan_then_swap(path):
        nonlocal swapped
        selected = os.fstat(path).st_ino == inode if isinstance(path, int) else Path(path) == root
        with real_scan(path) as entries:
            snapshot = list(entries)
        if selected and not swapped:
            root.rename(saved)
            root.symlink_to(victim, target_is_directory=True)
            swapped = True
        return Scan(snapshot)

    monkeypatch.setattr(os, "scandir", scan_then_swap)
    try:
        if use_candidate:
            rg.remove_tree(root)
        else:
            assert shutil.rmtree.avoids_symlink_attacks
            shutil.rmtree(root)
    except OSError:
        pass  # Refusal is acceptable; deleting the external file is not.
    assert swapped
    assert sentinel.exists() and sentinel.read_text() == "external"


@pytest.mark.parametrize("use_candidate", [True, False])
def test_single_target_cleanup_keeps_unrelated_private_index(tmp_path, monkeypatch, use_candidate):
    monkeypatch.setenv("GIT_CONFIG_GLOBAL", os.devnull)
    monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1")
    repo, chosen, other = (tmp_path / n for n in ("repo", "chosen", "other"))
    repo.mkdir()
    git(repo, "init", "-q")
    (repo / "file.txt").write_text("base")
    git(repo, "add", "file.txt")
    git(repo, "-c", "user.name=Review fixture", "-c", "user.email=fixture@example.invalid",
        "commit", "-qm", "fixture")
    git(repo, "worktree", "add", "-qb", "chosen", str(chosen))
    git(repo, "worktree", "add", "-qb", "other", str(other))
    (other / "file.txt").write_text("staged-only")
    git(other, "add", "file.txt")
    (other / "file.txt").write_text("different working copy")
    admin = Path(git(other, "rev-parse", "--absolute-git-dir").stdout.strip())
    other.rename(tmp_path / "temporarily_moved")
    if use_candidate:
        assert remove_worktree(str(repo), chosen)
    else:
        git(repo, "worktree", "remove", str(chosen))
    assert not chosen.exists()
    assert (admin / "index").exists()

The repair should converge in the existing owner: exact linked-worktree admission; explicit force and lock authority; identity-bound traversal; target-scoped metadata settlement; truthful partial outcomes; and every production entrypoint consuming the same contract. The original incident is worth fixing promptly, but the replacement must not trade one Windows deletion escape for broader target, lifecycle and metadata deletion hazards.

[andrexibiza]

P1 — this is repository-wide pruning, not settlement of the selected worktree. Fresh real-Git fixture: create chosen and other, stage a version in other different from its working copy, then temporarily move other without repairing its registration. remove_worktree(repo, chosen) deletes other's private .git/worktrees/<id>/index and registration as well, while targeted git worktree remove chosen preserves them. The full review includes an executed regression with both arms (candidate fails, Git control passes). Scope administrative cleanup to the exact admitted worktree; ordinary dashboard/CLI/Kanban removal must not acquire repository-wide garbage-collection authority.

[andrexibiza]

P1 — rechecking the final component does not bind its parent identity. In a deterministic Linux fixture, enumerate root, rename it aside, and replace the old pathname with a symlink to an external directory containing the same child filename. Path(entry.path) now resolves through that replacement; the fresh child lstat sees an ordinary file and _unlink_edge deletes the external file. The candidate returns success. The fd-based shutil.rmtree control under the same schedule preserves the victim. This extends the existing queued-directory finding: merely reclassifying current before scandir does not close replacement after enumeration. Child operations need retained fd/handle-relative identity, or an enforceable namespace-exclusion fence. Executed regression and control are in the full review.

[andrexibiza]

P1 scope-completion gate — this scan cannot establish one repository-wide removal owner. Besides the previously identified tools/subagent_worktree.py:168-175 bypass, exact head 4f7da230 still has native Electron removeWorktree() in apps/desktop/electron/git-worktree-ops.ts:350-364, wired to hermes:git:worktreeRemove in git-ipc.ts, executing Git's remover directly. Neither tools/ nor apps/desktop/ is searched here. The Python remote-dashboard repair does not cover the native-host path. Cover both runtime entrypoints and their shared safety contract, not only this Python subset. Also interlock #104226: its current _discard_unseeded_worktree adds another direct deleter in kanban_db_workspace.py.

[andrexibiza]

P2 — this race test passes when junction creation fails. planted['done'] is set before mklink, and neither the return code nor the resulting reparse entry is checked. I invoked this exact test function with subprocess.run forced to return exit 1 and create nothing; it returned normally. That was a test-logic mutation probe on Linux, not a native-Windows execution claim. Require successful creation and verify the link is a reparse point before treating the scenario as exercised. Otherwise the final assertion certifies that the call was attempted, not that a junction was present.

[kshitijk4poor]

PR Review — #104535

Verdict: Request changes. The Windows junction hazard in git worktree remove is plausible, but the replacement removes every safety check git provided, and a live probe on the merged tree shows it deleting things main refuses to touch.

Tests: merged tree vs origin/main — 136 passed, 1 failed, 5 skipped (Windows-only). The failure tests/hermes_cli/test_kanban_worktree_teardown.py::test_tree_dirtied_between_check_and_removal_preserved is NEW (green on main): the PR deletes a dirty worktree it should preserve.

Live probe (merged tree, macOS, real git repos): (A) linked worktree with a symlinked dir pointing outside → removed, outside files survive ✓. (B) an unrelated directory → deleted. (C) the primary checkout → its .git deleted. (D) another repository's linked worktree → deleted. (E) web_git.worktree_remove(force=False) on a locked + dirty worktree → deleted. On main git worktree remove refuses B–E.

Critical

• hermes_cli/worktree_removal.py:84-96 — only checks wt.exists(); never confirms the path is a registered linked worktree of repo_root (git worktree list --porcelain). Probe B/C/D above.
• hermes_cli/web_git.py:647 — force is discarded (remove_worktree has no dirty/force parameter), so dashboard force=false deletes dirty and locked trees. Probe E + the red kanban test.
• hermes_cli/worktree_removal.py:76-82 — unlock=True default strips the hermes pid=<pid> lock that worktree_ops.py:609 sets precisely so removal is refused while in use.
• Bug class incomplete: tools/subagent_worktree.py:171 and apps/desktop/electron/git-worktree-ops.ts:350-364 still call git worktree remove directly; the ownership test's glob (hermes_cli/**, cli.py) cannot see either.

Warnings

• hermes_cli/reparse_guard.py:143-175 — files are unlinked as encountered, so an UnknownEdge on a later sibling leaves a half-deleted tree while callers log "Preserving worktree" (kanban_db_workspace.py:212, worktree_ops.py:94, cli.py:1073).
• worktree_removal.py:101-111 — repo-wide git worktree prune can drop admin entries of other temporarily-unreachable worktrees; prune/branch -D return codes are ignored.
• reparse_guard.py:175 — classifies on FILE_ATTRIBUTE_REPARSE_POINT alone, so OneDrive placeholders / WOF-compressed entries count as reparse and abort the whole removal; stdlib checks st_reparse_tag == IO_REPARSE_TAG_MOUNT_POINT.
• The rationale that shutil.rmtree follows junctions (reparse_guard.py:90-96, PR body) is false on the supported interpreters (≥3.11): shutil._rmtree_islink treats mount-point reparse points as links and never recurses, and POSIX rmtree is fd-based.
• tests/cli/test_reparse_guard.py:778-803 reads source text and regex-scans it (AGENTS.md: never read source in tests); :529 aliases skipif instead of the windows_only marker, so 6 tests never run on either lane; :751-760 race test passes when mklink fails.

Suggested shape: verify registered worktree → shutil.rmtree(wt) (already junction-safe; onexc → refusal) → git worktree remove/prune. tools/skill_manager_guards.py:98-100 and tools/skills_hub_install.py:31-35 already have an _is_link_or_junction helper to reuse. Route the Electron and subagent_worktree sites through the same owner, keep force/lock semantics, and cut tests to ≤2 invariants with the windows_only marker. Happy to re-review once the registration check and force are back.

[kshitijk4poor]

Closing this one with credit and a clear ask, after a full review pass on the current head 4f7da23052 (merged onto origin/main d0df324 and probed live).

What holds up: the incident report is valuable, and the fail-closed UnknownEdge classification is the right instinct for a deletion boundary.

Why it can't land in this shape (all reproduced on the merged tree with real git repos, macOS):

• remove_worktree(repo_root, <unrelated dir>) → RemovalOutcome(removed=True), directory deleted. remove_worktree(repo_root, repo_root) → the primary checkout's .git deleted. Main's git worktree remove --force refuses both (rc 128).
• Locked + dirty worktree → deleted with the default unlock=True; web_git.py:647 discards force, so the dashboard route can delete dirty/locked trees.
• tests/hermes_cli/test_kanban_worktree_teardown.py::test_tree_dirtied_between_check_and_removal_preserved goes RED (green on main) — the kanban path relied on git's dirty re-check at removal time.
• git worktree prune at worktree_removal.py:109 is repository-wide; tools/subagent_worktree.py:171 and apps/desktop/electron/git-worktree-ops.ts still call git directly.
• Premise: CPython ≥3.11 shutil.rmtree treats IO_REPARSE_TAG_MOUNT_POINT as a link (_rmtree_islink; os.walk uses is_junction()), and git's remove_dir_recursively lstat's — neither descends into a junction. The only test that would demonstrate the traversal is windows_only via a bare skipif alias, so it has never run in CI.

Path forward: a @pytest.mark.windows_only reproduction of the actual traversal on the Windows lane, then a small pre-check (verify the target is a registered linked worktree of repo_root via git worktree list --porcelain, refuse and log if any entry is_symlink() or is_junction()), keeping git worktree remove [+ per-caller --force] as the deleter. Happy to carry that with your authorship on the incident write-up and the two binding tests if you'd rather not iterate. Refs the prior review threads above for the per-line details.