Ask git about the clone, not whichever repo it wandered into (#171)

.github/workflows/ci.yml

@@ -40,7 +40,32 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        environment: [py310, py311, py312, py313]
+        # `default` is in here as the newest interpreter, and it is the entry
+        # that was missing. The pinned legs stopped at py313 while `default`
+        # resolves the unpinned `python = ">=3.10"` -- 3.14 today -- so the
+        # newest Python this project supports ran only `test-e2e`, and `pixi run
+        # ci`, the command README's Development section tells a developer to
+        # run, was the one selection no CI job ran.
+        #
+        # The interpreter that matters is not hypothetical:
+        # `workspace_state.read_clone` exists because `Path.is_dir()` raises on
+        # 3.13 and earlier and returns `False` on 3.14, and `False` is the
+        # fail-open answer. What this leg is for is the reversion -- somebody
+        # writing `clone.is_dir()` back in -- on the one interpreter where that
+        # call fails open rather than raising. It is not what catches a boundary
+        # written down wrongly: those tests assert the same answer on every
+        # version, so they are green on both sides of it.
+        #
+        # `default` rather than a `py314` feature on purpose: it needs no new
+        # environment in the lockfile, and it keeps tracking whatever the newest
+        # solvable Python is instead of pinning a version somebody has to
+        # remember to bump. The cost of that choice is worth knowing: `default`
+        # is the only leg here whose Python is not pinned, so `pixi update`
+        # moves it without anyone choosing to, and the day 3.15 solves this
+        # matrix stops covering 3.14 rather than adding to it. `frozen: true`
+        # below keeps each commit reproducible against its lockfile, so this is
+        # a thing to notice at lockfile review, not a hole in the gate.
+        environment: [py310, py311, py312, py313, default]
     steps:
       - name: Checkout
         uses: actions/checkout@v6
@@ -66,8 +91,8 @@ jobs:
   # a pixi dependency, already in the lockfile setup-pixi restores.
   #
   # Outside the matrix above, because what it exercises is devpod and Docker
-  # rather than a Python version; four copies would buy nothing and cost four
-  # image pulls.
+  # rather than a Python version; one copy per matrix leg would buy nothing and
+  # cost one image pull each.
   #
   # Per-PR rather than nightly. It costs a few minutes against a matrix that
   # already costs some and runs beside it, so it does not lengthen the wait by
@@ -103,14 +128,14 @@ jobs:
   # One name for a branch ruleset to require, standing for every job above it.
   #
   # A ruleset names required checks as literal strings, so requiring the jobs
-  # individually means `ci (py310)`, `ci (py311)`, `ci (py312)`, `ci (py313)`
-  # and `e2e` written out in a repository setting -- a list that says nothing
-  # about why, that nobody reviews, and that goes stale the moment a matrix leg
-  # or a job is added or renamed here. The stale case is the dangerous one,
-  # because a required check that no longer exists is not a red tick: it is a
-  # merge that sails through ungated. Requiring `gate` instead puts the decision
-  # in this file, next to the jobs it is about, where a pull request that adds a
-  # job also adds it to `needs` under review.
+  # individually means `ci (py310)`, `ci (py311)`, `ci (py312)`, `ci (py313)`,
+  # `ci (default)` and `e2e` written out in a repository setting -- a list that
+  # says nothing about why, that nobody reviews, and that goes stale the moment
+  # a matrix leg or a job is added or renamed here. The stale case is the
+  # dangerous one, because a required check that no longer exists is not a red
+  # tick: it is a merge that sails through ungated. Requiring `gate` instead
+  # puts the decision in this file, next to the jobs it is about, where a pull
+  # request that adds a job also adds it to `needs` under review.
   #
   # It covers this workflow and only this workflow: `needs` cannot reach across
   # files, so the lint workflow's `prek` job is not in here and has to be

CHANGELOG.md

@@ -242,6 +242,65 @@ table and should be judged on its own merits.
 
 ### Fixed
 
+- **`dl <workspace> rm` could delete a clone that held unsaved work, and say
+  nothing** ([#171](https://github.com/blooop/devlaunch/issues/171)). The guard
+  ran `git` in the clone directory with nothing pinning it there — no
+  `--git-dir`, no `--work-tree`, no ceiling — so git's repository discovery
+  walked up the parent chain. A clone whose `.git` was unusable (half-removed by
+  an interrupted delete, truncated, never finished) did not make git refuse: it
+  made git find an **ancestor** repository and answer about that one. With
+  `dl`'s cache under `$XDG_CACHE_HOME` and a dotfiles repository in `$HOME`,
+  that ancestor is ordinary — and when it was clean and fully pushed, the guard
+  reported "nothing would be lost" about somebody else's repository and the
+  clone went, untracked scratch files and all. Only a *tidy* host could hit it:
+  a dirty ancestor made the guard fire for the wrong reason and hid the bug.
+
+  Git is now asked about one directory and cannot leave it, and "could not tell"
+  is an answer of its own that refuses the delete exactly as "would lose" does
+  — previously both were `None` and `None` meant delete freely. A directory that
+  is *there* but is not a repository git can read is now a refusal rather than a
+  clean bill of health; a directory that is *not* there still holds nothing, so
+  clearing up after a half-finished delete needs no `--force`. `--force` still
+  overrides, in both cases.
+
+  **Breaking, in `dl --ls --json`:** `unsaved` was a string or `null` and is now
+  an object with exactly one key — `{"nothingToLose": true}`,
+  `{"wouldLose": "<what>"}` or `{"couldNotTell": "<why>"}` — the shape `disk`
+  already uses. It is `null` exactly where `devlaunch` is `false`: a workspace
+  `dl` did not create. The break is the safe way round: a reader that tested the
+  old field for truthiness now sees a truthy object for every arm, so it leaves
+  workspaces alone rather than deleting them.
+
+  `unsaved`, `checkedOut` and `path` are answered for every workspace `dl` owns,
+  not only for the ones it still has a metadata record for. They used to gate on
+  the record while `devlaunch` and `disk` gated on the clone directory, so a
+  clone under the cache whose record had gone reported `devlaunch: true` with a
+  measured `disk` and `unsaved: null` beside them — `null` documented as "not
+  `dl`'s clone", on a clone `dl` had just called its own. That is the same
+  sentinel this entry is about, one layer out, and the same divergence
+  [PR #165](https://github.com/blooop/devlaunch/pull/165) closed for `disk`.
+
+  A clone dl cannot even look at — a parent directory it has no search
+  permission on — is a "could not tell" too, and `Path.is_dir()` had no way to
+  say so. It gave a different wrong answer on each supported Python: up to and
+  including 3.13 it re-raised `PermissionError`, so `dl <ws> rm` failed closed
+  by crashing and `dl --ls --json` became a traceback for the whole listing
+  because of one workspace; on 3.14 it returns `False`, which read as "not
+  there, so nothing to lose" — a clone that may be full of work, reported as
+  free to delete. The errno is now read directly: ENOENT and ENOTDIR mean there
+  is no clone there, and everything else means dl was not allowed to find out.
+  A path with a NUL byte in it — a record a hand-edited `metadata.json` can
+  produce — is a "could not tell" as well rather than a `ValueError` out of the
+  listing.
+
+  The boundary above was executed on 3.10.20, 3.11.15, 3.12.13, 3.13.14 and
+  3.14.6, and the `ci` matrix now runs every one of those minor versions; it
+  previously stopped at 3.13, so `pixi run ci` never ran on the newest Python
+  this project supports. This entry said "3.13+" for two rounds of review, and
+  no test would have said otherwise — what they assert is the same on every
+  version, so they are green either side of wherever the prose puts the line.
+  Somebody running it is what corrected it.
+
 - A cold `devpod up` no longer prints a red `fatal ... Process exited with
   status 1` from the tools probe. The probe asks a yes/no question and reports
   nothing; "no" is its everyday answer on a fresh workspace, and devpod

README.md

@@ -12,7 +12,7 @@ A streamlined CLI for [devpod](https://devpod.sh) with intuitive autocomplete an
 [![PyPI](https://img.shields.io/pypi/v/devlaunch)](https://pypi.org/project/devlaunch/)
 [![Conda](https://img.shields.io/badge/conda-v0.0.9-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch)
 [![License](https://img.shields.io/github/license/blooop/devlaunch)](https://opensource.org/license/mit/)
-[![Python](https://img.shields.io/badge/python-3.10%20%7C%203.11%20%7C%203.12%20%7C%203.13-blue)](https://www.python.org/downloads/)
+[![Python](https://img.shields.io/badge/python-3.10%20%7C%203.11%20%7C%203.12%20%7C%203.13%20%7C%203.14-blue)](https://www.python.org/downloads/)
 [![Pixi Badge](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/prefix-dev/pixi/main/assets/badge/v0.json)](https://pixi.sh)
 
 ## Installation
@@ -450,34 +450,68 @@ cleanup tool must not ignore — `unsaved`:
   "repo": "blooop/devlaunch",
   "branch": "wayfinder/devlaunch-80",
   "state": "Stopped",
-  "unsaved": "2 uncommitted change(s) (pixi.lock, notes.md) and 1 unpushed commit(s)"
+  "unsaved": {
+    "wouldLose": "2 uncommitted change(s) (pixi.lock, notes.md) and 1 unpushed commit(s)"
+  }
 }
 ```
 
-`unsaved` is a description of what deleting would destroy, or `null` when the
-clone holds nothing that does not also exist on a remote — uncommitted changes
-(untracked files included) and commits no remote has. The changed paths are
-named, not just counted, and that matters more than it looks: a devcontainer
-that runs a package install in its `postCreateCommand` can leave a tracked
-lockfile modified in *every* workspace it builds — this repo's own does — and as
-a bare count that is indistinguishable from an hour of unsaved work. A cleanup
-tool believing the count would then never clean anything. Named, it is
-judgeable. A workspace `dl` did not
-create reports `devlaunch: false` with no repo, branch or `unsaved`: there is no
-clone of `dl`'s to protect, and it has no business inspecting your checkout.
+`unsaved` is an object with exactly one key, and the key says which of three
+answers it is:
 
-**`dl <workspace> rm` refuses when the clone holds unsaved work**, so a caller
-that forgets to read the field is still caught:
+| `unsaved` | Meaning |
+| --- | --- |
+| `{"nothingToLose": true}` | Everything in the clone exists on a remote too. Deleting it costs nothing. |
+| `{"wouldLose": "<what>"}` | Uncommitted changes (untracked files included), commits no remote has, or both. |
+| `{"couldNotTell": "<why>"}` | `git` could not read the clone as a repository — a half-removed `.git`, an interrupted delete. The files are still there and nothing has established that they exist anywhere else. |
+
+The changed paths are named, not just counted, and that matters more than it
+looks: a devcontainer that runs a package install in its `postCreateCommand` can
+leave a tracked lockfile modified in *every* workspace it builds — this repo's
+own does — and as a bare count that is indistinguishable from an hour of unsaved
+work. A cleanup tool believing the count would then never clean anything. Named,
+it is judgeable. A workspace `dl` did not create reports `devlaunch: false` and
+no `unsaved` — `unsaved` is `null` exactly where `devlaunch` is `false`, and
+nowhere else: there is no clone of `dl`'s to protect, and it has no business
+inspecting your checkout. (`repo` and `branch` are a weaker test and not the
+same set: they come from `dl`'s metadata record, and a clone `dl` owns can have
+lost its record while the clone and the work in it are still on disk. That clone
+is inspected and reported like any other.)
+
+**`dl <workspace> rm` refuses to delete a clone it would lose work from — when
+the recorded clone holds unsaved work, and when it cannot tell what that clone
+holds**, so a caller that forgets to read the field is still caught. (Recorded,
+because that is the directory the guard reads; the case with no record is
+neither, and is described below.)
 
 ```
 $ dl blooop/repo@feature rm
 error: devlaunch-repo-feature-xyz holds 1 unpushed commit(s).
        Push or commit it, or run: dl blooop/repo@feature rm --force
 ```
 
+```
+$ dl blooop/repo@feature rm
+error: devlaunch-repo-feature-xyz: git could not read /home/…/repo/feature:
+       fatal: not a git repository. devlaunch will not delete a clone it cannot
+       check. Look at it, or run: dl blooop/repo@feature rm --force
+```
+
 That refusal is the only judgement `dl` makes here, and it is not about finished
-work — it is `dl` declining to destroy the only copy of something. Say `--force`
-if you mean it.
+work — it is `dl` declining to destroy the only copy of something, including
+when it cannot prove there is another copy. Say `--force` if you mean it.
+
+The guard reads `dl`'s metadata record, so the recorded directory is the one it
+asks about. (The delete does not always remove that same directory: when the
+recorded path is not on disk it falls back to a derived one. That divergence is
+older than this guard and is tracked as devlaunch#174.) One case is therefore
+neither a refusal nor a delete: a clone under `dl`'s cache that has **no** record — a metadata write
+that failed, a record pruned, a cache restored without one. The listing still
+reports what that clone holds, so `unsaved` is the field to read; but `rm`
+removes the devpod workspace, exits `0` without asking for `--force`, and leaves
+the clone on disk, because there is no recorded directory for it to remove
+either. Nothing is destroyed, and nothing then points at the clone: it is yours
+to keep or to `rm -rf` by hand.
 
 [`wf`](https://github.com/blooop/wayfinder) is the caller this was built for: it
 names its branches after its tickets, so it knows which workspaces belong to