Repository navigation
One directory for the guard, the listing and the delete (#174) - #181
Merged
Merged
Conversation
`dl <ws> rm` could destroy unsaved work and exit 0.
The guard read `record.local_path` unconditionally. The delete used that path
only when it existed and otherwise fell back to the derivation. So a record
pointing somewhere stale had the two naming different directories: the guard
answered `NothingToLose` about the absent recorded one -- correctly, an absent
directory holds nothing -- and the delete then removed the derived one, which was
where the work was. No `--force`, nothing logged.
Reproduced before fixing, and the reproduction is now the test
`test_a_stale_record_does_not_let_the_delete_past_the_guard`: mutating the guard
back to the direct read turns it red with the bug's own signature, exit 0 and
"Removed local clone".
A second face of the same field, worse. `WorktreeInfo.from_dict` builds
`local_path` with `Path(data["local_path"])`, and `Path("")` is `Path(".")` --
truthy, and its `exists()` is True. An empty recorded path therefore passed the
`if wt_info.local_path` test *and* the `.exists()` test and handed
`shutil.rmtree` dl's own working directory. Executed in a sandbox: it emptied
the directory, `.git` included, and only then failed on `os.rmdir(".")`. A
recorded path is now honoured only when it is absolute, since truthiness does
not distinguish these.
All three readers -- the guard, the `--ls --json` row and the delete -- now go
through `WorkspaceCloneManager.resolve_clone_path`. It returns `Optional[Path]`
rather than raising: `get_workspace_path` raises `ValueError` on an unsafe ref,
and one hand-edited record must not be able to take down the whole listing,
which is the harm `read_clone`'s stat guard exists to prevent one layer out.
`None` is a refusal at every caller -- the delete removes nothing, the guard
answers `CouldNotTell`.
The test double gets the real method called with the mock as `self`, not a copy
of the rule. #174 was two places naming one directory and drifting apart; a
double holding a third copy would be the same mistake, and would keep passing
after the rule changed under it.
One existing assertion changed rather than being preserved:
`test_a_clone_that_is_simply_gone_is_still_read_rather_than_assumed` asserted
`path` was the recorded directory. It is now the one `rm` would act on, which is
the point of the ticket. In production these are the same string -- a clone's
leaf *is* its workspace id -- and they differ here only because the fixture gives
the workspace a shorter id than the derivation would.
Reviewer's GuideUnifies clone directory resolution across guard, listing, and delete by introducing WorkspaceCloneManager.resolve_clone_path, tightening safety around recorded paths (including empty/relative ones), and updating tests and documentation to ensure all operations act on the same, correctly derived workspace directory or refuse when it cannot be named. Sequence diagram for unified guard and delete clone resolutionsequenceDiagram
actor User
participant dl as dl
participant WorkspaceCloneManager as WorkspaceCloneManager
participant workspace_state as workspace_state
User->>dl: _unsaved_work_in(workspace_id)
dl->>WorkspaceCloneManager: storage.get_worktree_by_workspace_id(workspace_id)
WorkspaceCloneManager-->>dl: WorktreeInfo or None
alt [record is None]
dl-->>User: workspace_state.NothingToLose()
else [record exists]
dl->>WorkspaceCloneManager: resolve_clone_path(wt_info)
alt [clone_path is None]
dl-->>User: workspace_state.CouldNotTell("could not work out which directory ...")
else [clone_path is Path]
dl->>workspace_state: holds_unsaved_work(clone_path)
workspace_state-->>dl: Unsaved verdict
dl-->>User: Unsaved verdict
end
end
User->>WorkspaceCloneManager: remove_workspace_by_id(workspace_id)
WorkspaceCloneManager->>WorkspaceCloneManager: storage.get_worktree_by_workspace_id(workspace_id)
alt [wt_info is None]
WorkspaceCloneManager-->>User: False
else [wt_info exists]
WorkspaceCloneManager->>WorkspaceCloneManager: resolve_clone_path(wt_info)
alt [ws_path is None]
WorkspaceCloneManager-->>User: False
else [ws_path is Path]
WorkspaceCloneManager->>WorkspaceCloneManager: _remove_clone(ws_path, owner, repo, branch)
WorkspaceCloneManager-->>User: bool
end
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="devlaunch/worktree/workspace_clone.py" line_range="120-123" />
<code_context>
+ of ``dl --ls --json`` -- the same harm :func:`read_clone`'s stat guard
+ exists to prevent, one layer out.
+ """
+ recorded = None if wt_info.local_path is None else Path(wt_info.local_path)
+ if recorded is not None and recorded.is_absolute() and recorded.exists():
+ return recorded
+ try:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `resolve_clone_path` only checks `exists()` on the recorded path, which might accept non-directory paths.
The code and docs treat the clone location as a directory, but the guard only checks `recorded.exists()`. If the stored path is stale or malformed and points to a regular file, it will still be returned and later code (e.g. `_remove_clone`) may try to operate on it as a directory. Using `recorded.is_dir()` (optionally with `exists()`) would better align with the intended semantics and avoid acting on non-directory paths.
```suggestion
"""
recorded = None if wt_info.local_path is None else Path(wt_info.local_path)
if recorded is not None and recorded.is_absolute() and recorded.is_dir():
return recorded
```
</issue_to_address>
### Comment 2
<location path="test/test_workspace_clone.py" line_range="849-822" />
<code_context>
+ assert resolved == derived
+ assert resolved.is_absolute()
+
+ def test_a_record_with_no_path_at_all_still_resolves(
+ self, clone_manager, mock_repo_manager, tmp_repos_dir
+ ):
+ """`None` used to reach `Path(None)` in the guard, which is a TypeError."""
+ repo_root = tmp_repos_dir / "owner" / "repo"
+ mock_repo_manager.get_repo_path.return_value = repo_root
+ derived = repo_root / leaf("nb4")
+ derived.mkdir(parents=True)
+
+ wt_info = MagicMock()
+ wt_info.owner, wt_info.repo, wt_info.branch = "owner", "repo", "nb4"
+ wt_info.local_path = None
+
+ assert clone_manager.resolve_clone_path(wt_info) == derived
+
+ def test_a_usable_recorded_path_still_wins(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for when `resolve_clone_path` must refuse a record and return `None`.
The failure case where `get_workspace_path` raises `ValueError` (unsafe ref) and `resolve_clone_path` returns `None` isn’t currently tested. Please add a unit test that configures `clone_manager.get_workspace_path` to raise `ValueError` for a given `wt_info`, asserts that `resolve_clone_path(wt_info) is None`, and checks that `remove_workspace_by_id` returns `False` without attempting deletion. This will lock in the refusal behaviour and prevent future changes from treating `None` as a valid path.
Suggested implementation:
```python
old_path = repo_root / "nb4" # the bare-branch-name leaf
old_path.mkdir(parents=True)
assert old_path.name != leaf("nb4")
def test_unsafe_ref_record_is_refused(
self, clone_manager, mock_repo_manager, tmp_repos_dir
):
"""Unsafe workspace refs must be refused and not treated as valid paths."""
repo_root = tmp_repos_dir / "owner" / "repo"
mock_repo_manager.get_repo_path.return_value = repo_root
wt_info = MagicMock()
wt_info.workspace_id = "workspace-unsafe"
wt_info.owner, wt_info.repo, wt_info.branch = "owner", "repo", "nb4"
wt_info.local_path = None
# Simulate unsafe ref: resolve_clone_path will internally call get_workspace_path
# which we configure to fail with ValueError.
clone_manager.get_workspace_path = MagicMock(side_effect=ValueError("unsafe ref"))
# The unsafe record must be refused and not treated as a valid path.
assert clone_manager.resolve_clone_path(wt_info) is None
# remove_workspace_by_id must also refuse the workspace and not attempt deletion.
with patch.object(clone_manager, "delete_workspace", autospec=True) as mock_delete:
assert clone_manager.remove_workspace_by_id(wt_info.workspace_id) is False
mock_delete.assert_not_called()
```
1. Ensure `patch` is imported at the top of `test/test_workspace_clone.py`, e.g.:
`from unittest.mock import MagicMock, patch` (or add `patch` to the existing import line).
2. If `clone_manager` uses a different deletion helper than `delete_workspace` inside
`remove_workspace_by_id`, update the patched attribute in the test to match the actual
method being called so that `assert_not_called()` accurately verifies that no deletion
is attempted.
3. If `remove_workspace_by_id` takes a full `wt_info` object instead of `workspace_id`,
or has a different signature, adjust the call in the test accordingly to match the
real API while keeping the core assertions (`resolve_clone_path(...) is None` and
`remove_workspace_by_id(...) is False`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+120
to
+123
| """ | ||
| recorded = None if wt_info.local_path is None else Path(wt_info.local_path) | ||
| if recorded is not None and recorded.is_absolute() and recorded.exists(): | ||
| return recorded |
There was a problem hiding this comment.
suggestion (bug_risk): resolve_clone_path only checks exists() on the recorded path, which might accept non-directory paths.
The code and docs treat the clone location as a directory, but the guard only checks recorded.exists(). If the stored path is stale or malformed and points to a regular file, it will still be returned and later code (e.g. _remove_clone) may try to operate on it as a directory. Using recorded.is_dir() (optionally with exists()) would better align with the intended semantics and avoid acting on non-directory paths.
Suggested change
| """ | |
| recorded = None if wt_info.local_path is None else Path(wt_info.local_path) | |
| if recorded is not None and recorded.is_absolute() and recorded.exists(): | |
| return recorded | |
| """ | |
| recorded = None if wt_info.local_path is None else Path(wt_info.local_path) | |
| if recorded is not None and recorded.is_absolute() and recorded.is_dir(): | |
| return recorded |
| wt_info.local_path = stale | ||
| mock_storage.get_worktree_by_workspace_id.return_value = wt_info | ||
|
|
||
| assert clone_manager.resolve_clone_path(wt_info) == derived |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for when resolve_clone_path must refuse a record and return None.
The failure case where get_workspace_path raises ValueError (unsafe ref) and resolve_clone_path returns None isn’t currently tested. Please add a unit test that configures clone_manager.get_workspace_path to raise ValueError for a given wt_info, asserts that resolve_clone_path(wt_info) is None, and checks that remove_workspace_by_id returns False without attempting deletion. This will lock in the refusal behaviour and prevent future changes from treating None as a valid path.
Suggested implementation:
old_path = repo_root / "nb4" # the bare-branch-name leaf
old_path.mkdir(parents=True)
assert old_path.name != leaf("nb4")
def test_unsafe_ref_record_is_refused(
self, clone_manager, mock_repo_manager, tmp_repos_dir
):
"""Unsafe workspace refs must be refused and not treated as valid paths."""
repo_root = tmp_repos_dir / "owner" / "repo"
mock_repo_manager.get_repo_path.return_value = repo_root
wt_info = MagicMock()
wt_info.workspace_id = "workspace-unsafe"
wt_info.owner, wt_info.repo, wt_info.branch = "owner", "repo", "nb4"
wt_info.local_path = None
# Simulate unsafe ref: resolve_clone_path will internally call get_workspace_path
# which we configure to fail with ValueError.
clone_manager.get_workspace_path = MagicMock(side_effect=ValueError("unsafe ref"))
# The unsafe record must be refused and not treated as a valid path.
assert clone_manager.resolve_clone_path(wt_info) is None
# remove_workspace_by_id must also refuse the workspace and not attempt deletion.
with patch.object(clone_manager, "delete_workspace", autospec=True) as mock_delete:
assert clone_manager.remove_workspace_by_id(wt_info.workspace_id) is False
mock_delete.assert_not_called()- Ensure
patchis imported at the top oftest/test_workspace_clone.py, e.g.:
from unittest.mock import MagicMock, patch(or addpatchto the existing import line). - If
clone_manageruses a different deletion helper thandelete_workspaceinside
remove_workspace_by_id, update the patched attribute in the test to match the actual
method being called so thatassert_not_called()accurately verifies that no deletion
is attempted. - If
remove_workspace_by_idtakes a fullwt_infoobject instead ofworkspace_id,
or has a different signature, adjust the call in the test accordingly to match the
real API while keeping the core assertions (resolve_clone_path(...) is Noneand
remove_workspace_by_id(...) is False).
`ty` is in the `ci` task and rejects `Path = None`. It did not catch this locally because a stray uv `.venv` in the checkout shadowed the pixi environment, so every run reported 44 unresolved-`pytest` imports and nothing else -- a local check that fails for a reason unrelated to the code is one nobody reads.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
+ Coverage 94.63% 94.67% +0.04%
==========================================
Files 21 21
Lines 2570 2591 +21
==========================================
+ Hits 2432 2453 +21
Misses 138 138
🚀 New features to boost your workflow:
|
added 2 commits
August 13, 2026 23:11
The first cut of the resolver used `recorded.exists()`, which reintroduced the bug `read_clone`'s docstring spends a paragraph on -- one layer out, and in the same file that documents it. `Path.exists()` swallows ENOENT, ENOTDIR, EBADF and ELOOP and re-raises everything else. A clone whose parent is mode 000 therefore raised `PermissionError` straight out of the resolver on 3.10-3.13, and returned False on 3.14. One expression, two behaviours, and 3.14's is the dangerous half: "not there" is the answer that sends this method off to name a *different* directory, which is the whole defect #174 is about. This passed locally on 3.14 and failed on all four older legs of the matrix. Worth recording, because the matrix is regularly proposed for trimming: the `default` (3.14) leg went green on the broken code and the py310-py313 legs are what caught it. `_on_disk` is total over the three answers and lives at module scope rather than on the class. That is not style: `_clone_manager` in the tests calls the real `resolve_clone_path` with a MagicMock as `self`, so a `self._on_disk(...)` came back as a truthy Mock and inverted the rule -- two tests that had been red for the right reason went green for the wrong one. Module scope is the part of the method a double cannot stand in front of.
The `ValueError` path and its `None` were reachable and untested — codecov caught what the suite did not. Writing the tests found a third thing: the warning read `wt_info.workspace_id`, a field the resolver otherwise never touches and the test double does not carry. It now names the owner/repo/branch that actually failed validation, which is both the thing that failed and the field a hand-edited metadata.json would have to be fixed in.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #174.
dl <ws> rmcould destroy unsaved work, return 0, and log nothing.The defect
The guard read the clone directory straight off the record:
The delete used that path only when it was on disk, and otherwise fell back to the derivation:
So when the recorded path is stale the two name different directories. The guard asks about the absent one and gets
NothingToLose— which is correct, an absent directory holds nothing — and the delete then removes the derived one, which is where the work is.Reproduced before fixing:
dl --ls --jsonread the record the same way, sopathandunsavedcould describe a third directory — which matters becausewf reapdecides from that listing.The second face: an empty recorded path deletes the working directory
WorktreeInfo.from_dictbuilds this field withPath(data["local_path"]), andPath("")isPath(".")— truthy, and itsexists()is True. So an empty recorded path passed theif wt_info.local_pathtest and the.exists()test, andshutil.rmtreewas handedPath(".").Executed in a sandbox: it emptied the directory,
.gitincluded, and only then failed onos.rmdir("."). The traceback is the only thing the user gets.Truthiness cannot distinguish this, so a recorded path is now honoured only when it is absolute.
The fix
One resolver,
WorkspaceCloneManager.resolve_clone_path, that the guard, the listing and the delete all go through. A guard is only a guard if it inspects what the delete will actually remove.It returns
Optional[Path]rather than raising.get_workspace_pathraisesValueErroron an unsafe ref, and one hand-edited record must not be able to take down the whole ofdl --ls --json— the same harmread_clone's stat guard exists to prevent, one layer out.Noneis a refusal at every caller: the delete removes nothing, the guard answersCouldNotTell.Tests
Six new, mutation-verified rather than trusted:
test_a_stale_record_does_not_let_the_delete_past_the_guardred with the bug's own signature — exit 0 andRemoved local clonetest_an_empty_recorded_path_is_not_the_working_directoryredtest_the_delete_removes_exactly_what_resolve_namedis the binding: without it the two could be made to disagree again by editingremove_workspace_by_idalone and every other test would stay green.The test double now calls the real method with the mock as
selfrather than restating its rule — #174 was two places naming one directory and drifting apart, and a double holding a third copy is the same mistake.One assertion changed rather than preserved
test_a_clone_that_is_simply_gone_is_still_read_rather_than_assumedassertedpathwas the recorded directory. It is now the onermwould act on, which is the point of the ticket. In production these are the same string — a clone's leaf is its workspace id — and they differ only because the fixture gives the workspace a shorter id than the derivation would.Verification
1310 tests pass,
ruff checkclean,ruff formatclean, pylint 10.00/10.🤖 Generated with Claude Code
Summary by Sourcery
Unify workspace clone path resolution so the delete operation, its safety guard, and JSON listing all act on the same directory and avoid unsafe deletions when metadata is stale or malformed.
Bug Fixes:
dl <ws> rmfrom deleting a derived clone directory while the guard inspects a different, stale recorded path.local_pathno longer resolves to the current working directory and cannot trigger removal of dl’s own workspace.Enhancements:
WorkspaceCloneManager.resolve_clone_pathas the single source of truth for mapping worktree records to clone directories and reuse it across guard, delete, and listing paths.Documentation:
CHANGELOG.md, including the new absolute-path requirement for recorded clone directories.Tests:
resolve_clone_pathandremove_workspace_by_idto ensure guards and deletes name the same directory and to cover stale, empty, and missinglocal_pathscenarios.