Skip to content

The unsaved-work guard and the delete can name different directories #174

Closed

Description

@blooop

Type: build
Map: #139

dl <ws> rm can ask "is there unsaved work?" about one directory and then destroy a different one. Found during the two-axis review of PR #173 and reproduced by execution against the real clone manager. Pre-existing on merged main, not introduced by that PR.

The mechanism

The guard reads record.local_path (devlaunch/dl.py:463). The delete does not: remove_workspace_by_id (devlaunch/worktree/workspace_clone.py:448-468) falls back to the derived clone path when local_path is not on disk, and shutil.rmtrees that instead.

So when a record's local_path points somewhere that no longer exists — a moved cache, a half-finished migration, a hand-edited record, exactly the states #88 documents — the guard truthfully reports "nothing to lose" about the absent path, and the delete then removes the live derived clone.

Reproduction (executed, real clone manager and storage)

record local_path: /…/moved-away          (absent)  -> guard saw NothingToLose()
dl r-feature-aaa rm                       ->  EXIT 0
Removed workspace clone: /…/repos/blooop/r/r-feature-piheniho
derived clone still on disk: False
work file still on disk:    False

Unsaved work destroyed, exit 0, no --force, no refusal.

Why this is not #171

#171 is the same class — the guard answers about a directory other than the one that dies — but a different mechanism. There, git's discovery walked up to an ancestor repository; the fix constrains discovery so it cannot escape the clone. Here, discovery is fine: the guard and the delete are simply given different paths, and both answer correctly about the path they were given.

PR #173 does not introduce this and does not make it worse. It does, however, re-affirm and pin the NothingToLose-for-an-absent-directory arm that this defect leans on — correctly, because "a directory that is not there holds nothing" is true, and the same arm is what lets rm clear away a clone someone removed by hand without --force. The defect is not that arm; it is that the guard and the delete do not agree on which directory is at stake.

What the fix has to achieve

  1. One path, decided once. The guard and the delete must be handed the same resolved directory, so a disagreement is impossible rather than merely unlikely. A caller that resolves the path and passes it to both is the obvious shape; deriving it twice is what created this.
  2. A disagreement, if it can still occur, must refuse. If local_path and the derived path can both exist and differ, that is a "cannot tell which" and must fail closed the way holds_unsaved_work can answer about an ancestor repository and report a dirty clone as safe to delete #171's CouldNotTell does — never a silent choice of one.
  3. Do not regress the hand-removed case. dl <ws> rm must still clear a workspace whose clone is genuinely gone, exit 0, without --force. That is pinned by test/test_workspace_state.py:660 and by Ask git about the clone, not whichever repo it wandered into (#171) #173's over-refusal mutation; keep both green.

Test-first

Guards must be shown red under a mutation that reintroduces the fault, not merely green — this repo has shipped hollow guards repeatedly (#145's leaked module memo made a pinned regression permanently green; review of #170 found two CouldNotAsk producers that stayed green when mutated to report "clean"). Run the full CI selection, never a filtered -k run.

At minimum: the reproduction above, asserting the derived clone and its unsaved file survive and the command refuses; the hand-removed case still exits 0 without --force; and whatever new arm the fix introduces is exhaustively matched (ty) at every consumer, as #173 established for Unsaved.

Related: #171 (same class, different mechanism), #88 (the record/reality divergence that produces the triggering state), #159 (--prune, parked, reads the same guard).

Activity

  1. blooop commented on Aug 10, 2026

    @blooop
    OwnerAuthor

    breadcrumb — a third face of this defect, in the listing rather than the delete. Found while reviewing PR #173 at 395e527, reproduced by execution.

    This ticket currently describes the rm face: the guard reads record.local_path while remove_workspace_by_id falls back to the derived path and rmtrees that. dl --ls --json has the same disagreement through a different door.

    Path("") is PosixPath('.'), and it is truthy, so a record whose local_path is the empty string makes the listing report devlaunch: true, path: ".", and an unsaved computed about dl's own current working directory — in the reviewer's run, their worktree's dirty files were reported as that workspace's unsaved work — while the sibling disk field is measured on the real clone. One row, three fields, two different directories, neither of them the workspace's.

    models.py:54 has already turned "" into Path('.') by the time workspace_clone.py:467's guard would look, so that guard does not fire either. dl never writes an empty local_path itself (workspace_clone.py:432 uses a derived absolute path), so this needs a corrupted or hand-edited metadata.json — the same provenance as this ticket's main case.

    It was deliberately not fixed in #173: patching the listing alone would put the listing and the guard on two different directories, which is this ticket's own failure mode. So the fix belongs here, and its requirement (1) — one path, decided once — should cover the listing as well as the guard and the delete. Three readers of the same record, one resolved path.

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

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions