From b92c67b4fcbad84aa05441bf4c3054ae817f02f2 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 10 Aug 2026 11:04:16 +0100 Subject: [PATCH 1/5] Ask git about the clone, not whichever repo it wandered into `holds_unsaved_work` -- the one judgement dl makes before destroying a directory -- could answer about a *different repository* and report "nothing would be lost" while the clone held unsaved work. Live in merged main, not only in the unmerged --prune work. `workspace_state._git` ran `git ... cwd=clone` with no --git-dir, no --work-tree and no ceiling, so git's repository discovery walked up the parent chain. A clone whose `.git` was unusable -- truncated, half-removed by an interrupted delete -- 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 repo in $HOME that ancestor is ordinary, and when it was clean and fully pushed `holds_unsaved_work` returned None for a clone holding untracked work. `dl rm` deleted it without asking for --force. The failure needed a *tidy* host: a dirty ancestor fired the guard for the wrong reason, about the wrong repository, and hid it. Two changes, neither sufficient alone: 1. Every git command names its repository (`--git-dir`/`--work-tree`), which switches discovery off entirely, so an unusable `.git` is a refusal. Verified against real git 2.55.0 on five shapes a broken clone actually takes -- garbage `.git`, empty `.git`, HEAD-only `.git`, a real clone with its object store deleted, a truncated gitfile -- all of which answered about the ancestor before and refuse now, while a healthy clone and a linked worktree (gitfile `.git`) still answer normally. GIT_CEILING_DIRECTORIES was the other candidate and is not used: it bounds discovery rather than switching it off, and when its path does not match what git resolved it fails *open*, back to the ancestor, silently. 2. The refusal has somewhere to go. `Optional[str]` conflated "nothing to lose" with "could not tell" and both were None, which meant delete freely. It is now a total sum -- NothingToLose / WouldLose(what) / CouldNotTell(why) -- every caller names the arm it handles, and could-not-tell refuses the delete exactly as would-lose does. The same sentinel one layer down (`if status:` reading a *refused* `git status` as a clean tree) goes with it: `_git` returns GitSaid/GitRefused, so empty output and no output stop looking alike. A directory that is *not there* still holds nothing, so clearing up after a half-finished delete needs no --force. A directory that *is* there and is not a repository is now a refusal rather than a clean bill of health -- it holds whatever files are in it, and with no repository to consult nothing has established they exist anywhere else. Breaking in `dl --ls --json`: `unsaved` was a string or null and is now an object with one key naming the arm, the shape `disk` already uses. Null keeps only its other meaning -- not dl's clone to inspect. It breaks the safe way: a reader testing the old field for truthiness now sees a truthy object for every arm and leaves workspaces alone. Closes #171 --- CHANGELOG.md | 29 ++++ README.md | 48 ++++-- devlaunch/dl.py | 74 +++++++-- devlaunch/workspace_state.py | 276 ++++++++++++++++++++++++++----- test/test_workspace_state.py | 312 ++++++++++++++++++++++++++++++++--- 5 files changed, 647 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 561b3076..ab31e08a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -242,6 +242,35 @@ table and should be judged on its own merits. ### Fixed +- **`dl 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": ""}` or `{"couldNotTell": ""}` — the shape `disk` + already uses. It is `null` only where `repo` and `branch` are: 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. + - 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 diff --git a/README.md b/README.md index 4687b7a7..f30259e0 100644 --- a/README.md +++ b/README.md @@ -450,24 +450,33 @@ 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 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": ""}` | Uncommitted changes (untracked files included), commits no remote has, or both. | +| `{"couldNotTell": ""}` | `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` with +no repo, branch or `unsaved` — `unsaved` is `null` there, and only there: there +is no clone of `dl`'s to protect, and it has no business inspecting your +checkout. + +**`dl rm` refuses when the clone holds unsaved work — and when it +cannot tell**, so a caller that forgets to read the field is still caught: ``` $ dl blooop/repo@feature rm @@ -475,9 +484,16 @@ 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. [`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 diff --git a/devlaunch/dl.py b/devlaunch/dl.py index 5d95a51f..8cfe2dd0 100644 --- a/devlaunch/dl.py +++ b/devlaunch/dl.py @@ -436,21 +436,30 @@ def update_cache_background(force: bool = False) -> None: pass -def _unsaved_work_in(workspace_id: str) -> Optional[str]: - """What deleting *workspace_id* would destroy, or None if nothing would be. - - Answers None for a workspace devlaunch has no record of, which is the honest - answer rather than a permissive one: those are workspaces opened from a path - or a URL that dl never cloned and does not manage, so it has no clone of its - own to protect and no business inspecting someone's checkout to find one. +def _unsaved_work_in(workspace_id: str) -> workspace_state.Unsaved: + """What deleting *workspace_id* would destroy, as far as dl can establish. + + Answers `NothingToLose` for a workspace devlaunch has no record of, which is + the honest answer rather than a permissive one: those are workspaces opened + from a path or a URL that dl never cloned and does not manage, so it has no + clone of its own to protect and no business inspecting someone's checkout to + find one. + + A record dl cannot *read* is a different matter and gets a different answer + (devlaunch#171). Both used to be None, and None meant delete freely -- the + same conflation the clone-level guard had, one level up: dl does not know + where the clone is, so it has not established that anything is safe, and the + delete stops until somebody says `--force`. """ try: record = _get_clone_manager().storage.get_worktree_by_workspace_id(workspace_id) except (OSError, RuntimeError) as e: logging.debug(f"could not read the workspace record for {workspace_id}: {e}") - return None + return workspace_state.CouldNotTell( + f"could not read the workspace record for {workspace_id}: {e}" + ) if record is None: - return None + return workspace_state.NothingToLose() return workspace_state.holds_unsaved_work(Path(record.local_path)) @@ -469,10 +478,20 @@ def workspaces_as_json(with_size: bool = False) -> int: - `repo` and `branch` come from the record dl wrote when it made the clone; a workspace dl did not make has neither, and says so with `devlaunch: false` rather than a guess. - - `unsaved` is the field a caller must not ignore: a description of what - deleting would destroy, or null. `dl rm` refuses on it too, so a - caller that forgets is still caught -- but a caller that reads it can - leave the workspace alone instead of arguing with a refusal. + - `unsaved` is the field a caller must not ignore: an object with exactly + one key, and the key says which of three answers it is -- + `{"nothingToLose": true}`, `{"wouldLose": ""}`, or + `{"couldNotTell": ""}`. It is null only where `repo` and `branch` + are: this is not dl's clone and it has nothing to inspect. `dl rm` + refuses on the last two as well, so a caller that forgets is still + caught -- but a caller that reads it can leave the workspace alone + instead of arguing with a refusal. + + It used to be a string or null, and null carried "nothing would be lost" + as well as "could not find out" -- which is how a broken clone got + deleted with work in it (devlaunch#171). The break is deliberate and it + breaks the safe way: a reader that tested the old field for truthiness + now sees a truthy object for every arm, so it leaves workspaces alone. - `state` is devpod's, one `devpod status` per workspace, which is why this is a command someone runs rather than something on the fast path. - `disk` appears only when `--size` was asked for, and describes dl's own @@ -503,7 +522,11 @@ def workspaces_as_json(with_size: bool = False) -> int: "path": str(clone_path) if clone_path else None, "state": get_workspace_state(ws.id), "lastUsed": ws.last_used, - "unsaved": state.unsaved if state else None, + # A nested object with one key, and the key says which kind of + # answer it is -- the shape `disk` uses, for the same reason + # (devlaunch#171). `null` keeps only its other meaning: there is no + # clone of dl's own here to inspect. + "unsaved": workspace_state.unsaved_as_json(state.unsaved) if state else None, } if with_size: # Absent unless asked, so a reader can never mistake "nobody asked" @@ -2142,7 +2165,8 @@ def print_help(): dl up Start the workspace without attaching dl stop Stop the workspace dl rm, prune Delete the workspace. Refuses if its clone - holds uncommitted or unpushed work; add + holds uncommitted or unpushed work, or if + git cannot read the clone to find out; add --force to delete it anyway. dl code Open in VS Code dl restart Stop and start (no rebuild) @@ -2561,13 +2585,29 @@ def _run_cli(argv: Optional[List[str]] = None) -> int: # does (a ticket tool, a script, a person), and this is what keeps a # confident caller from destroying an hour of somebody's afternoon. if "--force" not in args[2:]: + # Three answers, and only one of them is permission (devlaunch#171). + # Matched arm by arm rather than tested for truth, so that an answer + # this code does not know about stops the delete instead of sliding + # through an `else`. unsaved = _unsaved_work_in(workspace_id) - if unsaved: + if isinstance(unsaved, workspace_state.WouldLose): logging.error( - f"{workspace_id} holds {unsaved}. Push or commit it, or run: " + f"{workspace_id} holds {unsaved.description}. Push or commit it, or run: " f"dl {raw_spec} rm --force" ) return 1 + if isinstance(unsaved, workspace_state.CouldNotTell): + # Refused for not knowing, and it says which. The work is still + # on disk, and nothing has established that it exists anywhere + # else -- which is the same standing as unpushed work, and gets + # the same refusal and the same way past it. + logging.error( + f"{workspace_id}: {unsaved.reason}. devlaunch will not delete a clone it " + f"cannot check. Look at it, or run: dl {raw_spec} rm --force" + ) + return 1 + if not isinstance(unsaved, workspace_state.NothingToLose): + workspace_state.unhandled_unsaved(unsaved) return workspace_delete(workspace_id) if subcommand == "up": diff --git a/devlaunch/workspace_state.py b/devlaunch/workspace_state.py index afdfc7ba..a08c003a 100644 --- a/devlaunch/workspace_state.py +++ b/devlaunch/workspace_state.py @@ -22,40 +22,205 @@ abandoned one are indistinguishable, a branch merged upstream may still have work to do, and a repo whose flow does not delete branches gets nothing. The caller that knows the answer should say the answer. + +**Three answers, not two (devlaunch#171).** This module used to answer +``Optional[str]``: a description of what would be lost, or ``None``. ``None`` +carried two meanings — "nothing would be lost" and "I could not find out" — and +the destructive caller read both as permission. That is not a hypothetical +conflation; it shipped, and it destroyed the wrong thing: + +``_git`` ran ``git … cwd=clone`` with no ``--git-dir``, no ``--work-tree`` and +no ceiling, so git's repository **discovery walked up the parent chain**. A +clone whose ``.git`` was unusable — truncated, half-removed by an interrupted +delete, or never finished — did not make git refuse. It made git find an +*ancestor* repository and answer confidently about that one. With ``dl``'s cache +under ``$XDG_CACHE_HOME`` and a dotfiles repository in ``$HOME``, that ancestor +is common; when it was clean and fully pushed, ``holds_unsaved_work`` returned +``None`` for a clone holding untracked work and ``dl rm`` deleted it +without so much as asking for ``--force``. The failure needed a *tidy* host to +appear, because a dirty ancestor made the guard fire — for the wrong reason, +about the wrong repository — and hid it. + +Both halves are fixed here and neither is sufficient alone: + +1. Every git command names its repository explicitly (:func:`_git`), so + discovery is switched off and an unusable ``.git`` produces a refusal. +2. A refusal has somewhere to go: :data:`Unsaved` is a total sum — + :class:`NothingToLose`, :class:`WouldLose`, :class:`CouldNotTell` — and + every caller must name the arm it is handling. "Could not tell" refuses a + delete exactly as "would lose" does. """ import logging import subprocess from dataclasses import dataclass from pathlib import Path -from typing import List, Optional +from typing import Any, Dict, List, NoReturn, Optional, Union logger = logging.getLogger(__name__) +@dataclass(frozen=True) +class NothingToLose: + """Everything in the clone exists somewhere else. Deleting it costs nothing. + + Carries no payload on purpose: there is nothing to say. It is a distinct + type rather than a ``None`` so that a caller cannot reach it by accident — + the whole of devlaunch#171 was a caller reaching it by accident. + """ + + +@dataclass(frozen=True) +class WouldLose: + """Deleting the clone would destroy *description*. + + A description rather than a flag, and that is load-bearing: it is printed to + a person deciding whether to force the delete, and "3 uncommitted change(s) + (pixi.lock, notes.md, …) and 2 unpushed commit(s)" is the thing that answers + them. See :func:`_name_a_few` for why the count alone is not enough. + """ + + description: str + + def __post_init__(self) -> None: + # An empty description would print as "workspace holds ." and read as a + # bug in dl rather than as a reason to stop. If there is nothing to + # name, the answer is NothingToLose, not a WouldLose with no words. + if not self.description: + raise ValueError("WouldLose needs something to say; use NothingToLose() instead") + + +@dataclass(frozen=True) +class CouldNotTell: + """git could not be asked about this clone, and *reason* is what it said. + + Not a failure to report — a report. It is the answer for a directory that is + there but is not a repository git can read, and it must stop a delete for + the same reason :class:`WouldLose` does: the files are still on disk, and + nothing has established that they exist anywhere else. + + The reason is carried rather than reconstructed at the point of printing, + because the cause is not guessable from the path: an interrupted delete, a + ``.git`` written by a container as another user, a truncated gitfile and a + directory that was never a clone all arrive here and read differently to + the person who has to decide what to do about it. It names the directory it + is about, which is the specific thing the shipped bug got wrong. + """ + + reason: str + + +# Three arms and no fourth. "The directory is not there" is not a fourth: it is +# NothingToLose, because there is no work in it to lose -- the same answer +# disk_usage gives such a directory (Measured(0)), and what lets a caller clear +# away a workspace whose clone was already removed by hand. +Unsaved = Union[NothingToLose, WouldLose, CouldNotTell] + + +def unhandled_unsaved(unsaved: NoReturn) -> NoReturn: + """Reject an unsaved arm nobody handled -- at type-check time, not at runtime. + + Exported because the arms are read outside this module (``dl.py``'s delete + guard and its JSON listing), and an ``else`` hand-rolled at a call site is + exactly how a fourth arm would be silently read as "safe to delete". + + Hand-rolled rather than :func:`typing.assert_never`, which is 3.11+ while + this package supports 3.10; a parameter typed ``NoReturn`` gets the same + treatment from the checker. The same shape as + :func:`devlaunch.disk_usage._unhandled_usage`, deliberately. + """ + raise AssertionError(f"Unhandled unsaved-work answer: {unsaved!r}") + + +def unsaved_as_json(unsaved: Unsaved) -> Dict[str, Any]: + """How an answer reads to a tool: one key, and the key says which kind it is. + + Deliberately not a nullable string. A caller that reads ``nothingToLose`` + has been told nothing would be lost; a caller that reads ``couldNotTell`` + has been told dl does not know, and cannot have got there by finding a + field absent or null. The shape :func:`devlaunch.disk_usage.usage_as_json` + already uses, for the same reason. + + This is a deliberate break in ``dl --ls --json``: the field used to be a + string or ``null``. It breaks in the safe direction — a reader that tested + the old field for truthiness now sees an object, which is truthy for every + arm, so it leaves workspaces alone rather than deleting them. ``null`` + survives one level up, in ``dl.py``, where it keeps its other meaning: + there is no clone of dl's own there to inspect. + """ + if isinstance(unsaved, NothingToLose): + return {"nothingToLose": True} + if isinstance(unsaved, WouldLose): + return {"wouldLose": unsaved.description} + if isinstance(unsaved, CouldNotTell): + return {"couldNotTell": unsaved.reason} + unhandled_unsaved(unsaved) + + @dataclass(frozen=True) class CloneState: """What one workspace clone holds, as far as git can tell. - ``unsaved`` is the load-bearing field and is deliberately a description - rather than a flag: it is printed to a person who is deciding whether to - force a delete, and "3 uncommitted change(s) and 2 unpushed commit(s)" is - the thing that answers them. ``None`` means the clone holds nothing that - does not also exist on a remote. + ``branch`` is what the clone has checked out, or ``None`` when git could not + say — which is every case where ``unsaved`` is a :class:`CouldNotTell`, and + also an unborn HEAD. It is reported beside the recorded branch rather than + instead of it (``dl --ls --json`` prints both), so a clone an agent moved + off its branch is visible as such. """ branch: Optional[str] - unsaved: Optional[str] + unsaved: Unsaved + + +@dataclass(frozen=True) +class GitSaid: + """git ran and exited 0. ``output`` may be empty; empty is an answer.""" + output: str + + +@dataclass(frozen=True) +class GitRefused: + """git could not answer, and ``reason`` is what it said about that.""" -def _git(repo: Path, *args: str) -> Optional[str]: - """Run git in *repo*, returning stdout, or ``None`` if it refused. + reason: str - A refusal is "cannot tell", never an answer: a clone that is broken, gone or - not a repository must not stop the other workspaces being reported, and must - never be reported as *safe to delete* on the strength of a failed command — - every caller here treats ``None`` as "no information", and the one place - that matters (:func:`holds_unsaved_work`) fails safe explicitly. + +# The distinction the old `Optional[str]` could not make safely. `""` and +# `None` are both falsey, so `if status:` read a *refused* `git status` as a +# clean tree -- the same sentinel bug as the one above, one layer down, and it +# would have survived the discovery fix on its own. +GitAnswer = Union[GitSaid, GitRefused] + + +def _git(repo: Path, *args: str) -> GitAnswer: + """Ask git about *repo* — and only about *repo*. + + ``--git-dir`` and ``--work-tree`` are the fix for devlaunch#171 and are not + optional decoration. Passing ``cwd=`` alone leaves git's repository + discovery switched on, and discovery walks up the parent chain: on a clone + whose ``.git`` is unusable git does not refuse, it finds an **ancestor** + repository and answers about that. Naming the git directory switches + discovery off entirely, so the only repository git can reach is this one and + an unusable ``.git`` becomes a refusal — which is what the caller needs. + + Verified against real git (2.55.0) rather than assumed, on each shape a + broken clone actually takes: a ``.git`` directory holding garbage, an empty + ``.git`` directory, a ``.git`` with HEAD and nothing else, a real clone with + its object store deleted, and a truncated gitfile. All five refuse here; + all five answered about the ancestor under plain ``cwd=``. A healthy clone + and a *linked worktree* (whose ``.git`` is a gitfile, which git follows) + both still answer normally, so pinning the clone down costs nothing. + + ``GIT_CEILING_DIRECTORIES`` was the other candidate and is not used: it + bounds discovery instead of switching it off, so it has to be an absolute + path that matches what git resolved the clone's parent to, and when it does + not match it fails *open* — back to the ancestor, silently. ``--git-dir`` + fails closed. + + ``cwd=repo`` is kept even though it no longer selects the repository, so + that ``git status`` keeps printing paths relative to the clone root exactly + as before. Only trailing newlines are trimmed, never leading whitespace. A full ``strip()`` here was wrong in a way that took real use to notice: the first @@ -64,31 +229,45 @@ def _git(repo: Path, *args: str) -> Optional[str]: :func:`_name_a_few` then reported ``ixi.lock``. Untracked entries start ``??`` and were unharmed, which is exactly why the tests missed it. """ + root = repo.resolve() try: result = subprocess.run( - ["git", *args], cwd=repo, capture_output=True, text=True, check=False, timeout=30 + ["git", f"--git-dir={root / '.git'}", f"--work-tree={root}", *args], + cwd=repo, + capture_output=True, + text=True, + check=False, + timeout=30, ) except (OSError, subprocess.SubprocessError) as e: logger.debug(f"git {' '.join(args)} in {repo}: {e}") - return None + return GitRefused(str(e)) if result.returncode != 0: - logger.debug(f"git {' '.join(args)} in {repo}: {result.stderr.strip()}") - return None - return result.stdout.rstrip("\n") + stderr = result.stderr.strip() + logger.debug(f"git {' '.join(args)} in {repo}: {stderr}") + return GitRefused(stderr or f"git {' '.join(args)} exited {result.returncode}") + return GitSaid(result.stdout.rstrip("\n")) def read_clone(clone: Path) -> CloneState: """Report what *clone* holds. The only function here that talks to git. - A directory that is not there, or is not a repository, holds nothing: there - is no work in it to lose. That is the truth about it rather than a special - case, and it is what lets a caller clear away a workspace whose clone was - already removed by hand. + A directory that is **not there** holds nothing: there is no work in it to + lose. That is the truth about it rather than a special case, and it is what + lets a caller clear away a workspace whose clone was already removed by + hand. + + A directory that *is* there and that git cannot read as a repository is a + different answer, and used to be given the same one. It holds whatever files + are in it, and with no repository to consult nothing has established that + they exist anywhere else — so it is a :class:`CouldNotTell`, and the delete + stops. See this module's docstring for what that cost before it did. """ if not clone.is_dir(): - return CloneState(branch=None, unsaved=None) - branch = _git(clone, "rev-parse", "--abbrev-ref", "HEAD") - return CloneState(branch=branch or None, unsaved=_unsaved(clone, branch)) + return CloneState(branch=None, unsaved=NothingToLose()) + head = _git(clone, "rev-parse", "--abbrev-ref", "HEAD") + branch = head.output or None if isinstance(head, GitSaid) else None + return CloneState(branch=branch, unsaved=_unsaved(clone, branch)) def _name_a_few(changed: List[str], limit: int = 3) -> str: @@ -111,8 +290,8 @@ def _name_a_few(changed: List[str], limit: int = 3) -> str: return ", ".join(names) -def _unsaved(clone: Path, branch: Optional[str]) -> Optional[str]: - """What deleting *clone* would destroy, in words, or ``None`` if nothing. +def _unsaved(clone: Path, branch: Optional[str]) -> Unsaved: + """What deleting *clone* would destroy, in words — or that git could not say. Two kinds of loss, reported together because someone deciding whether to force a delete wants both: @@ -124,11 +303,28 @@ def _unsaved(clone: Path, branch: Optional[str]) -> Optional[str]: *any* remote ref rather than this branch's upstream, so work that was pushed under another name, or merged and fetched back, is correctly not counted as lost. + + ``git status`` is asked first and doubles as the repository probe: with + ``--git-dir`` naming the clone (see :func:`_git`) it succeeds on any + repository git can read — including one with no commits yet — and refuses + on every unusable ``.git``. A refusal here is therefore not "clean", it is + :class:`CouldNotTell`, and so is a refusal from the ``git log`` below: once + the repository has been shown readable, a command that then fails has + failed for a reason nobody here can account for, and accounting for it by + saying "nothing to lose" is the bug this module exists to not have. + + *branch* being ``None`` after a successful ``status`` means HEAD names no + commit — a clone of an empty repository, or one checked out to a ref that + does not exist yet. There is no commit to be unpushed, so there is nothing + to ask ``git log`` about. """ - losses: List[str] = [] status = _git(clone, "status", "--porcelain") - if status: - changed = status.splitlines() + if isinstance(status, GitRefused): + return CouldNotTell(f"git could not read {clone}: {status.reason}") + + losses: List[str] = [] + if status.output: + changed = status.output.splitlines() losses.append(f"{len(changed)} uncommitted change(s) ({_name_a_few(changed)})") if branch: # Argument order is load-bearing: `--not` flips the sense of every ref @@ -136,15 +332,21 @@ def _unsaved(clone: Path, branch: Optional[str]) -> Optional[str]: # --remotes ` excludes the branch as well and is silently always # empty — which would report every clone as safe to delete. unpushed = _git(clone, "log", "--oneline", branch, "--not", "--remotes") - if unpushed: - losses.append(f"{len(unpushed.splitlines())} unpushed commit(s)") - return " and ".join(losses) if losses else None + if isinstance(unpushed, GitRefused): + return CouldNotTell( + f"git could not list unpushed commits on {branch} in {clone}: {unpushed.reason}" + ) + if unpushed.output: + losses.append(f"{len(unpushed.output.splitlines())} unpushed commit(s)") + return WouldLose(" and ".join(losses)) if losses else NothingToLose() -def holds_unsaved_work(clone: Path) -> Optional[str]: - """What would be lost by deleting *clone*, or ``None`` if nothing would be. +def holds_unsaved_work(clone: Path) -> Unsaved: + """What would be lost by deleting *clone*, as far as git can be made to say. The guard `dl rm` consults. Thin on purpose: the interesting behaviour - is in :func:`read_clone`, and this is the name the guard reads by. + is in :func:`read_clone`, and this is the name the guard reads by. Total — + every path returns one of the three arms, and none of them means "go ahead" + by default. """ return read_clone(clone).unsaved diff --git a/test/test_workspace_state.py b/test/test_workspace_state.py index 454c88d7..c374edee 100644 --- a/test/test_workspace_state.py +++ b/test/test_workspace_state.py @@ -13,9 +13,10 @@ The git tests use real repositories with a local bare as their remote -- a local path is a real git remote, so push, fetch and the remote-tracking refs all behave exactly as they do over ssh, with no network. Mocking git here would only -prove this file agrees with itself, and the one bug these tests caught before -release (an argument order that reported every clone as safe to delete) was -invisible to everything except a real `git log`. +prove this file agrees with itself, and the two bugs these tests caught (an +argument order that reported every clone as safe to delete, and git's discovery +walking up into an ancestor repository) were both invisible to everything except +a real git. """ import json @@ -27,7 +28,15 @@ import pytest from devlaunch.dl import main -from devlaunch.workspace_state import CloneState, holds_unsaved_work, read_clone +from devlaunch.workspace_state import ( + CloneState, + CouldNotTell, + NothingToLose, + WouldLose, + holds_unsaved_work, + read_clone, + unsaved_as_json, +) def git(*args: str, cwd: Path) -> str: @@ -66,33 +75,74 @@ def clone(remote: Path, tmp_path: Path) -> Path: return work +@pytest.fixture +def ancestor(tmp_path: Path) -> Path: + """A repository that is clean, fully pushed, and ignores `.cache/`. + + The dotfiles-in-`$HOME` case, and the reason devlaunch#171 only shows itself + on a tidy host: git's discovery walking up out of a broken clone lands here, + and a repository with nothing to report answers "nothing to report". + """ + host = tmp_path / "host" + host.mkdir() + git("init", "-q", "-b", "main", ".", cwd=host) + (host / ".gitignore").write_text(".cache/\n") + commit(host, "seed") + origin = tmp_path / "host-origin.git" + git("init", "-q", "--bare", str(origin), cwd=tmp_path) + git("remote", "add", "origin", str(origin), cwd=host) + git("push", "-q", "-u", "origin", "main", cwd=host) + # The premise, asserted rather than assumed: if this repo were dirty or had + # an unpushed commit the guard would fire for the wrong reason and the bug + # these tests are about would be invisible. + assert git("status", "--porcelain", cwd=host) == "" + assert git("log", "--oneline", "main", "--not", "--remotes", cwd=host) == "" + return host + + +@pytest.fixture +def broken_clone_under_ancestor(ancestor: Path) -> Path: + """A clone whose `.git` is unusable, holding scratch work, nested in a repo. + + What an interrupted delete, a truncated write or a half-copied cache leaves + behind. The directory is there and holds a file that exists nowhere else. + """ + clone = ancestor / ".cache" / "devlaunch" / "ws" + clone.mkdir(parents=True) + (clone / ".git").mkdir() + (clone / ".git" / "HEAD").write_text("garbage\n") + (clone / "scratch.md").write_text("half a plan\n") + return clone + + class TestWhatACloneHolds: def test_a_pushed_branch_with_a_clean_tree_holds_nothing_unsaved(self, clone): - assert read_clone(clone) == CloneState(branch="feature", unsaved=None) + assert read_clone(clone) == CloneState(branch="feature", unsaved=NothingToLose()) def test_an_unpushed_commit_is_unsaved(self, clone): (clone / "more.txt").write_text("more\n") commit(clone, "more") unsaved = holds_unsaved_work(clone) - assert unsaved and "1 unpushed commit(s)" in unsaved + assert isinstance(unsaved, WouldLose) and "1 unpushed commit(s)" in unsaved.description def test_uncommitted_changes_are_unsaved(self, clone): (clone / "feature.txt").write_text("edited\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "1 uncommitted change(s)" in unsaved + assert isinstance(unsaved, WouldLose) and "1 uncommitted change(s)" in unsaved.description def test_untracked_files_are_unsaved_too(self, clone): # An agent's scratch notes are not less lost for never having been added. (clone / "notes.md").write_text("half a plan\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "uncommitted" in unsaved + assert isinstance(unsaved, WouldLose) and "uncommitted" in unsaved.description def test_both_kinds_of_loss_are_reported_together(self, clone): (clone / "more.txt").write_text("more\n") commit(clone, "more") (clone / "dirty.txt").write_text("dirty\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "uncommitted" in unsaved and "unpushed" in unsaved + assert isinstance(unsaved, WouldLose) + assert "uncommitted" in unsaved.description and "unpushed" in unsaved.description def test_a_branch_whose_commits_are_on_the_remote_under_another_name_is_saved( self, clone, remote @@ -102,18 +152,150 @@ def test_a_branch_whose_commits_are_on_the_remote_under_another_name_is_saved( # upstream is what gets this right. git("push", "origin", "feature:review/feature", cwd=clone) git("branch", "-m", "feature", "renamed", cwd=clone) - assert holds_unsaved_work(clone) is None + assert holds_unsaved_work(clone) == NothingToLose() def test_a_clone_that_is_not_there_holds_nothing(self, tmp_path): # A half-finished delete, or a directory removed by hand. There is no # work in it to lose, and nothing here may crash on it. - assert read_clone(tmp_path / "absent") == CloneState(branch=None, unsaved=None) + assert read_clone(tmp_path / "absent") == CloneState(branch=None, unsaved=NothingToLose()) - def test_a_directory_that_is_not_a_repository_holds_nothing(self, tmp_path): + def test_a_clone_nested_in_a_repository_still_answers_about_itself(self, ancestor, remote): + """A healthy clone under a repository is not confused with its ancestor. + + The other half of devlaunch#171: pinning the clone down must not cost + the ordinary answer. `dl`'s cache lives under `$XDG_CACHE_HOME`, which + on a great many machines is inside a dotfiles repository. + """ + work = ancestor / ".cache" / "devlaunch" / "ws" + git("clone", "-q", str(remote), str(work), cwd=ancestor) + git("checkout", "-q", "-b", "feature", cwd=work) + (work / "mine.txt").write_text("mine\n") + state = read_clone(work) + assert state.branch == "feature" + assert isinstance(state.unsaved, WouldLose) and "mine.txt" in state.unsaved.description + + +class TestWhenGitCannotBeAsked: + """devlaunch#171: a refusal is an answer of its own, and it is not "clean". + + `_git` used to run with `cwd=` alone -- no `--git-dir`, no `--work-tree`, no + ceiling -- so git's repository discovery walked up the parent chain. A clone + whose `.git` was unusable did not make git refuse; it made git answer about + an **ancestor** repository. On a tidy ancestor that answer is "nothing to + report", `holds_unsaved_work` returned `None`, and `None` meant delete + freely. The scratch file in the clone went with it. + + Two things had to change and both are pinned here: git is now asked about + one directory and cannot leave it, and "could not tell" is an arm of its own + that refuses the delete exactly as "would lose" does. + """ + + def test_an_unusable_git_is_could_not_tell_not_nothing_to_lose( + self, broken_clone_under_ancestor + ): + unsaved = holds_unsaved_work(broken_clone_under_ancestor) + assert isinstance(unsaved, CouldNotTell), unsaved + # And it must say so of *this* directory, not of the one git wandered + # into: the reason is what a person reads before deciding to force. + assert str(broken_clone_under_ancestor) in unsaved.reason + + def test_it_does_not_borrow_the_ancestors_branch_either(self, broken_clone_under_ancestor): + # The shipped bug reported `branch='main'` -- the ancestor's checked-out + # branch -- and `dl --ls --json` printed it as this clone's `checkedOut`. + assert read_clone(broken_clone_under_ancestor).branch is None + + def test_an_unusable_git_with_no_ancestor_at_all_is_also_could_not_tell(self, tmp_path): + # The same directory with nothing above it to walk into. git refuses + # either way now, so the answer does not depend on what the machine + # happens to have in a parent directory. + clone = tmp_path / "ws" + clone.mkdir() + (clone / ".git").mkdir() + (clone / ".git" / "HEAD").write_text("garbage\n") + (clone / "scratch.md").write_text("half a plan\n") + assert isinstance(holds_unsaved_work(clone), CouldNotTell) + + def test_a_directory_that_is_not_a_repository_cannot_be_judged(self, tmp_path): + """A present directory that is not a repository is not an empty one. + + This function used to answer `None` here, documented as "a directory + that is not there, or is not a repository, holds nothing". Half of that + is true and stays true -- a directory that is *not there* holds nothing, + which is what lets a caller clear away a workspace whose clone was + removed by hand. The other half was the bug: a directory that *is* + there and is not a repository holds whatever files are in it, and git, + having no repository to read, cannot say whether they exist anywhere + else. That is a refusal, and a refusal is not permission. + """ plain = tmp_path / "plain" plain.mkdir() (plain / "file.txt").write_text("not a repo\n") - assert read_clone(plain) == CloneState(branch=None, unsaved=None) + state = read_clone(plain) + assert state.branch is None + assert isinstance(state.unsaved, CouldNotTell) + + def test_a_half_removed_clone_is_could_not_tell(self, clone): + """An interrupted delete: a real clone with its object store gone. + + Named separately from the garbage-`.git` case because it is the one the + issue describes reaching in the wild, and because it is the shape where + the *files* are still all there to lose. + """ + (clone / "scratch.md").write_text("half a plan\n") + subprocess.run(["rm", "-rf", str(clone / ".git" / "objects")], check=True) + assert isinstance(holds_unsaved_work(clone), CouldNotTell) + + def test_a_readable_repo_whose_remote_refs_are_broken_is_could_not_tell(self, clone): + """The second refusal, which the first would otherwise hide. + + `git status` succeeds -- it never looks at remote-tracking refs -- so + the repository probe passes and the clone reads as clean right up until + `git log … --not --remotes` is asked, which refuses on a ref pointing at + an object that is not there. Answering "nothing to lose" on the strength + of the half that worked is the same bug in a narrower place: the unpushed + commits were never counted. A real broken ref, not a patched subprocess, + because the point is what git does. + """ + bogus = clone / ".git" / "refs" / "remotes" / "origin" / "bogus" + bogus.parent.mkdir(parents=True, exist_ok=True) + bogus.write_text("0123456789abcdef0123456789abcdef01234567\n") + unsaved = holds_unsaved_work(clone) + assert isinstance(unsaved, CouldNotTell), unsaved + assert "unpushed commits" in unsaved.reason and "feature" in unsaved.reason + + def test_git_that_cannot_be_run_at_all_is_could_not_tell(self, clone): + # No git on PATH, a fork that fails: the process-level refusal, which + # never reaches a return code to inspect. + with patch("devlaunch.workspace_state.subprocess.run", side_effect=OSError("no git")): + unsaved = holds_unsaved_work(clone) + assert isinstance(unsaved, CouldNotTell) and "no git" in unsaved.reason + + +class TestTheAnswersAreTotal: + """Nothing may reach a caller as an answer the caller does not name. + + Three arms, and every place that reads them says which arm it is handling. + A fourth would raise here rather than being read as permission to delete -- + which is what the `Optional[str]` this replaced did with anything it had no + case for. + """ + + def test_an_arm_nobody_handles_is_refused_rather_than_rendered(self): + with pytest.raises(AssertionError, match="Unhandled unsaved-work answer"): + unsaved_as_json("nothing to lose, honest") # type: ignore[arg-type] + + def test_would_lose_cannot_be_built_with_nothing_to_say(self): + # "workspace holds ." reads as a bug in dl rather than as a reason to + # stop, and the arm for having nothing to report already exists. + with pytest.raises(ValueError): + WouldLose("") + + def test_each_arm_renders_as_one_key_that_names_it(self): + assert unsaved_as_json(NothingToLose()) == {"nothingToLose": True} + assert unsaved_as_json(WouldLose("1 unpushed commit(s)")) == { + "wouldLose": "1 unpushed commit(s)" + } + assert unsaved_as_json(CouldNotTell("git said no")) == {"couldNotTell": "git said no"} def _entry(workspace_id: str, source: Path) -> dict: @@ -172,14 +354,32 @@ def test_it_reports_the_repo_branch_and_that_nothing_is_unsaved(self, tmp_path, assert ours["devlaunch"] is True assert ours["repo"] == "blooop/r" assert ours["branch"] == "feature" - assert ours["unsaved"] is None + assert ours["unsaved"] == {"nothingToLose": True} assert ours["state"] == "Stopped" def test_unsaved_work_is_reported_so_a_caller_can_leave_it_alone(self, tmp_path, clone, capsys): (clone / "scratch.txt").write_text("half-finished\n") _code, report = self._run(tmp_path, clone, capsys) ours = next(w for w in report if w["id"] == "r-feature-aaa") - assert ours["unsaved"] and "uncommitted" in ours["unsaved"] + assert "uncommitted" in ours["unsaved"]["wouldLose"] + + def test_a_clone_git_cannot_read_is_not_reported_as_nothing_to_lose( + self, tmp_path, broken_clone_under_ancestor, capsys + ): + """The JSON surface of devlaunch#171. + + One key, and the key says which kind of answer it is -- the shape + `disk` already uses, for the same reason. A caller cannot arrive at + "nothing would be lost" by reading a field that is absent, and the old + `null` cannot be mistaken for it either, because `null` here now means + only "not `dl`'s clone to inspect". + """ + _code, report = self._run(tmp_path, broken_clone_under_ancestor, capsys) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert "nothingToLose" not in ours["unsaved"] + assert str(broken_clone_under_ancestor) in ours["unsaved"]["couldNotTell"] + # And not the ancestor's branch, reported as this clone's. + assert ours["checkedOut"] is None def test_a_workspace_devlaunch_did_not_make_says_so_instead_of_guessing( self, tmp_path, clone, capsys @@ -338,7 +538,7 @@ def test_the_size_column_lines_up_with_its_heading(self, tmp_path, capsys): class TestTheDeleteGuard: """`dl rm` refuses to destroy the only copy of something.""" - def _run(self, tmp_path, clone, argv): + def _run(self, tmp_path, clone, argv, record_read_raises=None): cache = tmp_path / "cache" / "devlaunch" listing = json.dumps( [_entry("r-feature-aaa", cache / "repos" / "blooop" / "r" / "r-feature-aaa")] @@ -356,10 +556,14 @@ def devpod(args, **kwargs): deleted.append(args[1]) return subprocess.CompletedProcess(args, 0, "", "") + manager = _clone_manager(records) + if record_read_raises is not None: + manager.storage.get_worktree_by_workspace_id.side_effect = record_read_raises + with ( patch("devlaunch.dl._get_cache_dir", return_value=cache), patch("devlaunch.dl.run_devpod", side_effect=devpod), - patch("devlaunch.dl._get_clone_manager", return_value=_clone_manager(records)), + patch("devlaunch.dl._get_clone_manager", return_value=manager), patch("devlaunch.dl.update_cache_background"), ): code = main(argv) @@ -391,6 +595,70 @@ def test_uncommitted_changes_stop_it_too(self, tmp_path, clone): assert code == 1 assert deleted == [] + def test_a_clone_git_cannot_read_stops_the_delete_too( + self, tmp_path, broken_clone_under_ancestor, caplog + ): + """devlaunch#171, at the surface it destroys things from. + + This is the whole ticket: on the shipped code this returned 0 and + deleted the workspace, because git had answered about the ancestor + repository and the answer was "nothing to report". "Could not tell" + must refuse exactly as "would lose" does -- and say `--force`, because + a refusal a person cannot get past is a bug of its own. + """ + code, deleted = self._run(tmp_path, broken_clone_under_ancestor, ["r-feature-aaa", "rm"]) + assert code == 1 + assert deleted == [] + assert "--force" in caplog.text + assert str(broken_clone_under_ancestor) in caplog.text + # The clone is still there, with the work still in it. + assert (broken_clone_under_ancestor / "scratch.md").exists() + + def test_force_still_gets_past_a_refusal_it_cannot_explain( + self, tmp_path, broken_clone_under_ancestor + ): + # The caller who means it is not blocked by dl declining to guess. + code, deleted = self._run( + tmp_path, broken_clone_under_ancestor, ["r-feature-aaa", "rm", "--force"] + ) + assert code == 0 + assert deleted == ["r-feature-aaa"] + + def test_a_workspace_whose_record_cannot_be_read_is_not_deleted_either( + self, tmp_path, clone, caplog + ): + """A record dl cannot read is not a record saying "nothing here". + + The same conflation one level up: `_unsaved_work_in` used to answer + `None` both for "this is not dl's workspace, it has no clone to + protect" and for "dl could not read its own metadata", and `None` meant + delete freely. + """ + code, deleted = self._run( + tmp_path, clone, ["r-feature-aaa", "rm"], record_read_raises=OSError("disk gone") + ) + assert code == 1 + assert deleted == [] + assert "--force" in caplog.text + + def test_an_answer_this_guard_does_not_name_stops_the_delete(self, tmp_path, clone): + # The exhaustiveness check, at the surface that destroys things. A + # fourth arm added later must arrive here as a crash, not as consent: + # the `if unsaved:` this replaced would have deleted on anything falsey + # and stopped on anything else, without either being a decision. + with patch("devlaunch.dl._unsaved_work_in", return_value="looks empty to me"): + with pytest.raises(AssertionError, match="Unhandled unsaved-work answer"): + self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + + def test_a_clone_already_removed_by_hand_is_still_deleted(self, tmp_path, clone): + # The reason the "not there" arm answers `NothingToLose` rather than + # refusing: clearing up after a half-finished delete must not need + # `--force`. + subprocess.run(["rm", "-rf", str(clone)], check=True) + code, deleted = self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + assert code == 0 + assert deleted == ["r-feature-aaa"] + class TestNamingWhatIsUnsaved: """A count cannot be judged; a name can. @@ -406,7 +674,7 @@ class TestNamingWhatIsUnsaved: def test_the_changed_paths_are_named(self, clone): (clone / "pixi.lock").write_text("churned by the container's own build\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "pixi.lock" in unsaved + assert isinstance(unsaved, WouldLose) and "pixi.lock" in unsaved.description def test_a_modified_tracked_file_keeps_its_first_letter(self, clone): # The regression that took real use to find. `git status --porcelain` @@ -416,17 +684,17 @@ def test_a_modified_tracked_file_keeps_its_first_letter(self, clone): # unharmed, which is why every test here passed while the feature was # printing nonsense. Asserted on the exact rendering, not on a substring. (clone / "feature.txt").write_text("edited by the container's build\n") - assert holds_unsaved_work(clone) == "1 uncommitted change(s) (feature.txt)" + assert holds_unsaved_work(clone) == WouldLose("1 uncommitted change(s) (feature.txt)") def test_a_long_list_is_cut_short_rather_than_dumped(self, clone): for i in range(6): (clone / f"file{i}.txt").write_text("x\n") unsaved = holds_unsaved_work(clone) - assert unsaved and "6 uncommitted change(s)" in unsaved + assert isinstance(unsaved, WouldLose) and "6 uncommitted change(s)" in unsaved.description # Three names and an ellipsis: enough to recognise, not a wall of text. - assert unsaved.count(",") == 3 and "…" in unsaved + assert unsaved.description.count(",") == 3 and "…" in unsaved.description def test_a_renamed_path_keeps_both_halves(self, clone): git("mv", "feature.txt", "renamed.txt", cwd=clone) unsaved = holds_unsaved_work(clone) - assert unsaved and "renamed.txt" in unsaved + assert isinstance(unsaved, WouldLose) and "renamed.txt" in unsaved.description From 88c1b4c56bceb2c43bd45a82f84915884fd1ef91 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 10 Aug 2026 12:17:33 +0100 Subject: [PATCH 2/5] Say null once, pin the work tree, and stop overstating the evidence Four review items on the #171 fix, none of them in the fix itself. `dl --ls --json` gated `unsaved` on the metadata record while `disk` gated on the clone directory, so a clone under the cache with no record printed `devlaunch: true`, a measured `disk`, and `unsaved: null` -- and `null` is documented as "no clone of dl's here". #171's own sentinel, one layer out, and the divergence PR #165 already closed for `disk`. `unsaved`, `path` and `checkedOut` now fall back to `_measurable_clone`, so `unsaved: null` and `devlaunch: false` are the same set by construction. `--work-tree` was load-bearing and unguarded: dropping it left the suite green, but `core.worktree` pointing elsewhere makes `git status --porcelain` print nothing at rc 0 on a clone holding an untracked file. Test added that goes red without the flag. `read_clone` asked `Path.is_dir()`, which cannot express "I was not allowed to look" and answers differently per Python: <=3.12 raised PermissionError out of the listing, 3.13+ returns False and reads as nothing-to-lose. The errno is read directly now. Two docstrings claimed more than was measured: `_git` said all five broken shapes answered about the ancestor under plain `cwd=` (four did; a truncated gitfile is a hard error in git and was never part of the bug), and `CloneState` asserted an invariant its own new test disproves. --- CHANGELOG.md | 21 ++++- README.md | 11 ++- devlaunch/dl.py | 30 +++++-- devlaunch/workspace_state.py | 68 +++++++++++++-- test/test_workspace_state.py | 157 +++++++++++++++++++++++++++++++++-- 5 files changed, 264 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab31e08a..1b1fba11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -266,11 +266,30 @@ table and should be judged on its own merits. **Breaking, in `dl --ls --json`:** `unsaved` was a string or `null` and is now an object with exactly one key — `{"nothingToLose": true}`, `{"wouldLose": ""}` or `{"couldNotTell": ""}` — the shape `disk` - already uses. It is `null` only where `repo` and `branch` are: a workspace + 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: on ≤3.12 it + re-raised `PermissionError`, so `dl rm` failed closed by crashing and + `dl --ls --json` became a traceback for the whole listing because of one + workspace; on 3.13+ 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 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 diff --git a/README.md b/README.md index f30259e0..428025eb 100644 --- a/README.md +++ b/README.md @@ -470,10 +470,13 @@ 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` — `unsaved` is `null` there, and only there: there -is no clone of `dl`'s to protect, and it has no business inspecting your -checkout. +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 rm` refuses when the clone holds unsaved work — and when it cannot tell**, so a caller that forgets to read the field is still caught: diff --git a/devlaunch/dl.py b/devlaunch/dl.py index 8cfe2dd0..01c69d49 100644 --- a/devlaunch/dl.py +++ b/devlaunch/dl.py @@ -481,8 +481,8 @@ def workspaces_as_json(with_size: bool = False) -> int: - `unsaved` is the field a caller must not ignore: an object with exactly one key, and the key says which of three answers it is -- `{"nothingToLose": true}`, `{"wouldLose": ""}`, or - `{"couldNotTell": ""}`. It is null only where `repo` and `branch` - are: this is not dl's clone and it has nothing to inspect. `dl rm` + `{"couldNotTell": ""}`. It is null exactly where `devlaunch` is + false: there is no clone of dl's own here to inspect. `dl rm` refuses on the last two as well, so a caller that forgets is still caught -- but a caller that reads it can leave the workspace alone instead of arguing with a refusal. @@ -492,6 +492,8 @@ def workspaces_as_json(with_size: bool = False) -> int: deleted with work in it (devlaunch#171). The break is deliberate and it breaks the safe way: a reader that tested the old field for truthiness now sees a truthy object for every arm, so it leaves workspaces alone. + - `path` is the directory `unsaved` and `checkedOut` describe, and is null + on the same workspaces `unsaved` is. - `state` is devpod's, one `devpod status` per workspace, which is why this is a command someone runs rather than something on the fast path. - `disk` appears only when `--size` was asked for, and describes dl's own @@ -507,7 +509,24 @@ def workspaces_as_json(with_size: bool = False) -> int: for ws in workspaces: mine = is_devlaunch_clone(ws, cache_dir) record = clone_mgr.storage.get_worktree_by_workspace_id(ws.id) if mine else None - clone_path = Path(record.local_path) if record else None + # Which directory this row is about. The record's path when there is a + # record -- that is the one `dl rm`'s guard reads, and a listing + # that described a different directory from the guard would be worse + # than useless to the caller deciding whether to call it. Otherwise the + # clone dl owns on disk, which is the same directory `disk` measures + # below and the same one `--purge` deletes. + # + # Gating this on the record alone was the sentinel bug of devlaunch#171 + # surviving one layer out. A clone under the cache that dl has no record + # for -- a metadata write that failed, a record pruned, a cache restored + # without one -- reported `devlaunch: true` and a measured `disk`, and + # `unsaved: null` beside them. `null` is documented as "not dl's clone", + # which that clone is not: dl says it is its own in the same object. So + # `null` still meant both "not mine" and "mine, never examined", which + # is the conflation this ticket exists to remove. This is the identical + # divergence #165 fixed for `disk` (see the comment on `_measurable_clone` + # below); `unsaved` was left behind by it. + clone_path = Path(record.local_path) if record else _measurable_clone(ws, cache_dir) state = workspace_state.read_clone(clone_path) if clone_path else None row: Dict[str, Any] = { "id": ws.id, @@ -524,8 +543,9 @@ def workspaces_as_json(with_size: bool = False) -> int: "lastUsed": ws.last_used, # A nested object with one key, and the key says which kind of # answer it is -- the shape `disk` uses, for the same reason - # (devlaunch#171). `null` keeps only its other meaning: there is no - # clone of dl's own here to inspect. + # (devlaunch#171). `null` keeps only its other meaning, and keeps it + # exactly: `clone_path` above is None precisely when `mine` is + # false, so `unsaved: null` and `devlaunch: false` are the same set. "unsaved": workspace_state.unsaved_as_json(state.unsaved) if state else None, } if with_size: diff --git a/devlaunch/workspace_state.py b/devlaunch/workspace_state.py index a08c003a..07574fe5 100644 --- a/devlaunch/workspace_state.py +++ b/devlaunch/workspace_state.py @@ -52,6 +52,8 @@ """ import logging +import os +import stat import subprocess from dataclasses import dataclass from pathlib import Path @@ -162,8 +164,18 @@ class CloneState: """What one workspace clone holds, as far as git can tell. ``branch`` is what the clone has checked out, or ``None`` when git could not - say — which is every case where ``unsaved`` is a :class:`CouldNotTell`, and - also an unborn HEAD. It is reported beside the recorded branch rather than + say — an unusable ``.git``, or an unborn HEAD. + + The two fields are independent, and an earlier draft of this docstring said + otherwise ("``None`` in every case where ``unsaved`` is a + :class:`CouldNotTell`"). A clone git *can* read as a repository but whose + remote-tracking refs are broken gives ``CloneState(branch='feature', + unsaved=CouldNotTell(...))``: ``git status`` answered, so the branch is + known, and only the later ``git log … --not --remotes`` refused. That is the + shape ``test_a_readable_repo_whose_remote_refs_are_broken_is_could_not_tell`` + builds, and the behaviour is right — it was the invariant that was wrong. + + ``branch`` is reported beside the recorded branch rather than instead of it (``dl --ls --json`` prints both), so a clone an agent moved off its branch is visible as such. """ @@ -207,10 +219,27 @@ def _git(repo: Path, *args: str) -> GitAnswer: Verified against real git (2.55.0) rather than assumed, on each shape a broken clone actually takes: a ``.git`` directory holding garbage, an empty ``.git`` directory, a ``.git`` with HEAD and nothing else, a real clone with - its object store deleted, and a truncated gitfile. All five refuse here; - all five answered about the ancestor under plain ``cwd=``. A healthy clone - and a *linked worktree* (whose ``.git`` is a gitfile, which git follows) - both still answer normally, so pinning the clone down costs nothing. + its object store deleted, and a truncated gitfile. All five refuse here. + **Four of the five** answered about the ancestor under plain ``cwd=``: the + truncated gitfile did not, because git treats an unreadable gitfile as a + hard error (``fatal: invalid gitfile format``) rather than continuing + discovery upward, so that one shape was never part of the bug — it is listed + because it is a shape a broken clone takes, not because ``cwd=`` mishandled + it. A healthy clone and a *linked worktree* (whose ``.git`` is a gitfile, + which git follows) both still answer normally, so pinning the clone down + costs nothing. + + ``--work-tree`` earns its place separately from ``--git-dir``, and the suite + goes red without it. ``core.worktree`` in the clone's own config points the + work tree at another directory, and ``--git-dir`` alone honours it: on a + clone holding an untracked file, ``git status --porcelain`` then prints + **nothing at rc 0** — "nothing to lose", about a directory nobody asked + about. That is devlaunch#171's failure class reached by a second route, and + it is the fail-*open* one. ``core.bare = true`` is the neighbouring shape and + fails closed instead (``fatal: this operation must be run in a work tree``, + which is a refusal), which is why only the ``core.worktree`` shape + demonstrates the flag is load-bearing. Pinned by + ``TestGitIsPinnedToItsWorkTreeToo`` in ``test/test_workspace_state.py``. ``GIT_CEILING_DIRECTORIES`` was the other candidate and is not used: it bounds discovery instead of switching it off, so it has to be an absolute @@ -262,8 +291,33 @@ def read_clone(clone: Path) -> CloneState: are in it, and with no repository to consult nothing has established that they exist anywhere else — so it is a :class:`CouldNotTell`, and the delete stops. See this module's docstring for what that cost before it did. + + A directory dl cannot even *look at* is a third answer, and ``Path.is_dir()`` + has no way to give it — which is why the ``os.stat`` below is written out + rather than left as ``clone.is_dir()``. ``is_dir()`` collapses every failure + into ``False``, and it does not even do that consistently across the + versions this package supports: on Python ≤3.12 it swallows ENOENT, ENOTDIR, + EBADF and ELOOP and *re-raises* the rest, so a clone whose parent is mode + ``000`` raised ``PermissionError`` straight out of here (``dl rm`` + failed closed by crashing; ``dl --ls --json`` became a traceback for the + whole listing because of one workspace). On 3.13+ the same call returns + ``False`` instead, which read as "not there, so nothing to lose" — a clone + that may be full of work, reported as free to delete, because dl was not + allowed to look. One sentinel each way, from the same expression. + + ``os.stat`` raises for all of them and the errno says which: ENOENT and + ENOTDIR mean there is no directory there to hold anything, and everything + else means dl was stopped before it could find out, which is exactly a + :class:`CouldNotTell`. The answer is now the same on every supported Python. """ - if not clone.is_dir(): + try: + present = stat.S_ISDIR(os.stat(clone).st_mode) + except (FileNotFoundError, NotADirectoryError): + return CloneState(branch=None, unsaved=NothingToLose()) + except OSError as e: + logger.debug(f"could not look at {clone}: {e}") + return CloneState(branch=None, unsaved=CouldNotTell(f"could not look at {clone}: {e}")) + if not present: return CloneState(branch=None, unsaved=NothingToLose()) head = _git(clone, "rev-parse", "--abbrev-ref", "HEAD") branch = head.output or None if isinstance(head, GitSaid) else None diff --git a/test/test_workspace_state.py b/test/test_workspace_state.py index c374edee..f3d8af5f 100644 --- a/test/test_workspace_state.py +++ b/test/test_workspace_state.py @@ -63,11 +63,10 @@ def remote(tmp_path: Path) -> Path: return origin -@pytest.fixture -def clone(remote: Path, tmp_path: Path) -> Path: +def _make_clone(remote: Path, work: Path) -> Path: """A workspace clone on a pushed branch, as `dl` would leave one.""" - work = tmp_path / "ws" - git("clone", str(remote), str(work), cwd=tmp_path) + work.parent.mkdir(parents=True, exist_ok=True) + git("clone", str(remote), str(work), cwd=work.parent) git("checkout", "-b", "feature", cwd=work) (work / "feature.txt").write_text("work\n") commit(work, "feature") @@ -75,6 +74,24 @@ def clone(remote: Path, tmp_path: Path) -> Path: return work +@pytest.fixture +def clone(remote: Path, tmp_path: Path) -> Path: + return _make_clone(remote, tmp_path / "ws") + + +@pytest.fixture +def clone_in_the_cache(remote: Path, tmp_path: Path) -> Path: + """The same clone, at the path `dl` actually puts one: under its own cache. + + Ownership is established from the source path devpod kept, so this is a clone + `dl` can prove is its own with or without a metadata record of it. It is the + path `TestTheJsonListing` hands devpod as the workspace source. + """ + return _make_clone( + remote, tmp_path / "cache" / "devlaunch" / "repos" / "blooop" / "r" / "r-feature-aaa" + ) + + @pytest.fixture def ancestor(tmp_path: Path) -> Path: """A repository that is clean, fully pushed, and ignores `.cache/`. @@ -159,6 +176,20 @@ def test_a_clone_that_is_not_there_holds_nothing(self, tmp_path): # work in it to lose, and nothing here may crash on it. assert read_clone(tmp_path / "absent") == CloneState(branch=None, unsaved=NothingToLose()) + def test_a_path_with_a_file_at_it_rather_than_a_clone_also_holds_nothing(self, tmp_path): + # Neither a clone nor a directory: the same answer as nothing at all, + # which is what this gave before the errno was read directly. + (tmp_path / "ws").write_text("not a clone\n") + assert read_clone(tmp_path / "ws") == CloneState(branch=None, unsaved=NothingToLose()) + + def test_a_clone_under_something_that_is_not_a_directory_holds_nothing(self, tmp_path): + # ENOTDIR rather than ENOENT: a parent component is a file. Still no + # clone at that path, so still nothing in it to lose. + (tmp_path / "file").write_text("not a directory\n") + assert read_clone(tmp_path / "file" / "ws") == CloneState( + branch=None, unsaved=NothingToLose() + ) + def test_a_clone_nested_in_a_repository_still_answers_about_itself(self, ancestor, remote): """A healthy clone under a repository is not confused with its ancestor. @@ -271,6 +302,64 @@ def test_git_that_cannot_be_run_at_all_is_could_not_tell(self, clone): assert isinstance(unsaved, CouldNotTell) and "no git" in unsaved.reason +class TestGitIsPinnedToItsWorkTreeToo: + """`--git-dir` names the repository; `--work-tree` names the files. + + The flags are not one belt and one brace. `core.worktree` in a clone's own + config points the work tree at another directory, and `--git-dir` alone + honours it: `git status --porcelain` then reports on *that* directory, which + on a clone holding an untracked file means empty output at rc 0 -- "nothing + to lose", about a directory nobody asked about. Devlaunch#171's failure + class reached by a second route, and the fail-*open* one. + + Dropping `--work-tree` and keeping `--git-dir` left the rest of this suite + green, so this class exists to make that mutation red. Its neighbour + `core.bare = true` does not: with it, `--git-dir` alone says `fatal: this + operation must be run in a work tree`, which is a refusal and therefore + already safe. Only the `core.worktree` shape shows the flag is load-bearing. + """ + + def test_a_clone_pointed_at_another_work_tree_still_reports_its_own_files( + self, clone, tmp_path + ): + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + git("config", "core.worktree", str(elsewhere), cwd=clone) + (clone / "an-hour-of-work.md").write_text("half a plan\n") + unsaved = holds_unsaved_work(clone) + assert isinstance(unsaved, WouldLose), unsaved + assert "an-hour-of-work.md" in unsaved.description + + +class TestADirectoryThatCannotBeLookedAt: + """Not knowing whether the clone is there is not knowing what it holds. + + A clone whose parent directory `dl` has no search permission on. This used + to be `clone.is_dir()`, which gave two different wrong answers depending on + which Python was running it: on ≤3.12 it re-raised `PermissionError`, so + `dl rm` failed closed by crashing and `dl --ls --json` became a + traceback for the whole listing because of one workspace; on 3.13+ the same + call returns `False`, which read as "not there, so nothing to lose" -- a + clone that may be full of work, reported as free to delete. This suite runs + on both, so a version-independent assertion is the point of this class. + """ + + def test_a_clone_behind_a_closed_door_is_could_not_tell(self, tmp_path): + if os.geteuid() == 0: + pytest.skip("root is refused by nothing, so the closed door would open") + parent = tmp_path / "locked" + clone = parent / "ws" + clone.mkdir(parents=True) + (clone / "an-hour-of-work.md").write_text("half a plan\n") + parent.chmod(0o000) + try: + state = read_clone(clone) + finally: + parent.chmod(0o700) + assert isinstance(state.unsaved, CouldNotTell), state + assert str(clone) in state.unsaved.reason + + class TestTheAnswersAreTotal: """Nothing may reach a caller as an answer the caller does not name. @@ -322,7 +411,7 @@ def _clone_manager(records: dict) -> MagicMock: class TestTheJsonListing: """`dl --ls --json`: the facts a cleanup tool decides from.""" - def _run(self, tmp_path, clone, capsys): + def _run(self, tmp_path, clone, capsys, recorded=True): cache = tmp_path / "cache" / "devlaunch" listing = json.dumps( [ @@ -330,7 +419,9 @@ def _run(self, tmp_path, clone, capsys): _entry("someone-elses", tmp_path / "projects" / "other"), ] ) - records = {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} + records = ( + {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} if recorded else {} + ) def devpod(args, **kwargs): if args[:1] == ["list"]: @@ -381,6 +472,55 @@ def test_a_clone_git_cannot_read_is_not_reported_as_nothing_to_lose( # And not the ancestor's branch, reported as this clone's. assert ours["checkedOut"] is None + def test_a_clone_dl_owns_but_has_no_record_for_is_still_inspected( + self, tmp_path, clone_in_the_cache, capsys + ): + """`unsaved: null` must mean "not `dl`'s clone", and only that. + + A clone under the cache that `dl` has no metadata record for -- a + metadata write that failed, a record pruned, a cache restored without + one -- is still `dl`'s clone: `devlaunch: true` says so, `--size` + measures it, and `--purge` deletes it. This used to gate on the record + while its neighbours gated on the clone directory, so that row read + `devlaunch: true` with a measured `disk` and `unsaved: null` beside + them, and `null` is documented as "no clone of `dl`'s here to inspect". + That is devlaunch#171's own sentinel surviving one layer out: `null` + carrying both "not mine" and "mine, never examined". Exactly the + divergence PR #165 closed for `disk`, which `unsaved` was left behind + by. + """ + (clone_in_the_cache / "an-hour-of-work.md").write_text("half a plan\n") + _code, report = self._run(tmp_path, clone_in_the_cache, capsys, recorded=False) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert ours["devlaunch"] is True + # No record, so nothing to report for the two fields that come from one. + assert ours["repo"] is None and ours["branch"] is None + # But the clone is there, and so is the work in it. + assert "an-hour-of-work.md" in ours["unsaved"]["wouldLose"] + assert ours["path"] == str(clone_in_the_cache) + assert ours["checkedOut"] == "feature" + # And on the same listing, `null` still means the one thing it means. + foreign = next(w for w in report if w["id"] == "someone-elses") + assert foreign["devlaunch"] is False and foreign["unsaved"] is None + + def test_one_unreachable_clone_does_not_take_the_whole_listing_down( + self, tmp_path, clone_in_the_cache, capsys + ): + # The harm `read_clone`'s stat guard prevents, at the surface it was + # visible from: one workspace behind a closed door used to raise out of + # the loop and the caller got a traceback instead of a listing. + if os.geteuid() == 0: + pytest.skip("root is refused by nothing, so the closed door would open") + clone_in_the_cache.parent.chmod(0o000) + try: + _code, report = self._run(tmp_path, clone_in_the_cache, capsys) + finally: + clone_in_the_cache.parent.chmod(0o700) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert "couldNotTell" in ours["unsaved"], ours["unsaved"] + # And the rest of the listing is still there. + assert next(w for w in report if w["id"] == "someone-elses")["devlaunch"] is False + def test_a_workspace_devlaunch_did_not_make_says_so_instead_of_guessing( self, tmp_path, clone, capsys ): @@ -521,6 +661,11 @@ def test_the_two_renderings_measure_the_same_workspaces(self, tmp_path, capsys): ours = next(row for row in report if row["id"] == "r-feature-aaa") assert ours["devlaunch"] is True assert 2 * 1024 * 1024 <= ours["disk"]["exclusiveBytes"] < 3 * 1024 * 1024 + # And the field a cleanup tool must not ignore is answered on the same + # row, for the same reason: `unsaved: null` beside a measured `disk` on + # a row that says `devlaunch: true` is `null` meaning two things again. + # (This clone is a plain directory, so the answer is a refusal.) + assert "couldNotTell" in ours["unsaved"] _code, out = self._run(tmp_path, ["--ls", "--size"], capsys, recorded=False) row = next(line for line in out.splitlines() if line.startswith("r-feature-aaa")) From 395e527ca46e17fa06ff1cdcdd19d98254e0b839 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 10 Aug 2026 13:40:36 +0100 Subject: [PATCH 3/5] Run the Python the boundary is about, and pin the shape that fails open Four mechanical items from the second two-axis review of #173. No change to what the fix does. The version boundary was wrong and stated as fact. `Path.is_dir()` on a clone behind a mode-000 parent was run on this repo's own environments: 3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise `PermissionError`; 3.14.6 returns `False`. The pivot is 3.14, not 3.13, in `workspace_state.py`, `test_workspace_state.py` and CHANGELOG. The previous claim was interpolated between two executed endpoints, in a commit titled "stop overstating the evidence". It survived because no CI job ran the side of it that fails open: the `ci` matrix stopped at py313 and the default (3.14) environment ran only `test-e2e`, so `pixi run ci` -- the command a developer is told to run -- was the one selection GitHub never ran. `default` is now a matrix leg. Executed: with `present = clone.is_dir()` back inside the existing `try`, the closed-door tests are red on 3.14 and **green on 3.10**, because there `is_dir()` re-raises into the `except OSError` arm. That leg is the only one that catches it. The `--work-tree` test pinned two filenames rather than the safety property. Executed at git 2.55.0: with `core.worktree` pointing at an *empty* directory, `--git-dir` alone prints " D README.md\n D feature.txt" at rc 0 -- a `WouldLose` about the wrong files, which is a refusal. The fail-open needs the other work tree to mirror HEAD, and then it prints nothing at rc 0: `NothingToLose()` on a clone holding an hour of work. The test builds the mirrored shape, so dropping the flag now fails on `isinstance(..., WouldLose)`; a second test pins that a clean clone is not reported dirty by the other tree's absences. The docstring said "nothing at rc 0" for the shape that does not produce it; it now says both. `unsaved: null` iff `devlaunch: false` was two questions that had to agree, and deleting either gate left the suite green. `_measurable_clone` is now asked once, `mine` is `clone_path is not None`, and a record only moves the row from one directory of dl's to another. The input that broke it -- a record that outlives the reason its workspace counts as dl's -- now has a test. README claimed `dl rm` refuses when the clone holds unsaved work. For a dl-owned clone with no record it does not: the guard reads the record, finds none, and answers `NothingToLose`, so the workspace goes and the clone stays at exit 0. No work is destroyed -- the delete reads the same absent record -- but it is not a refusal, and README now says what happens. Pinned by a test, with the fake's `remove_workspace_by_id` teaching both halves of the real one rather than returning a truthy Mock that removes nothing. `os.stat` is total over `OSError` but raises `ValueError` on a NUL-containing path, which took the whole listing down for one bad record -- the harm the stat guard exists to prevent. Caught, with a test that is red on 3.10 and 3.14 alike. `Path("")` reporting about dl's cwd is left alone: it needs a hand-edited metadata.json, and the delete already falls back to the derived path for an empty `local_path`, so it is the guard-and-delete-disagree family recorded on #174 rather than this one. pixi run ci: 1276 passed, 23 deselected, exit 0, on py310, py311, py312, py313 and default (3.14.6). workspace_state.py 100%. --- .github/workflows/ci.yml | 29 ++++- CHANGELOG.md | 23 ++-- README.md | 17 ++- devlaunch/dl.py | 52 +++++---- devlaunch/workspace_state.py | 67 ++++++++---- test/test_workspace_state.py | 202 +++++++++++++++++++++++++++++++---- 6 files changed, 313 insertions(+), 77 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a084b0b3..2ee845ed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,25 @@ 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 CONTRIBUTING tells a developer to run, was the one + # selection no CI job ran. + # + # That 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. Its test asserted the same thing + # on every version, so it passed either way -- and the boundary was + # documented a whole minor version out, through two rounds of review, + # because no job ran the side of it that fails open. + # + # `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. + environment: [py310, py311, py312, py313, default] steps: - name: Checkout uses: actions/checkout@v6 @@ -66,8 +84,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,8 +121,9 @@ 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 + # 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b1fba11..6ce168ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -282,13 +282,22 @@ table and should be judged on its own merits. 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: on ≤3.12 it - re-raised `PermissionError`, so `dl rm` failed closed by crashing and - `dl --ls --json` became a traceback for the whole listing because of one - workspace; on 3.13+ 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. + say so. It gave a different wrong answer on each supported Python: up to and + including 3.13 it re-raised `PermissionError`, so `dl 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 the leg that returned `False` — the fail-open + one — was exercised by no CI job, and this entry said "3.13+" for two rounds + of review. - 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 diff --git a/README.md b/README.md index 428025eb..88ca4504 100644 --- a/README.md +++ b/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 @@ -478,8 +478,9 @@ 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 rm` refuses when the clone holds unsaved work — and when it -cannot tell**, so a caller that forgets to read the field is still caught: +**`dl rm` refuses to delete a clone it would lose work from — when +the clone holds unsaved work, and when it cannot tell**, so a caller that forgets +to read the field is still caught: ``` $ dl blooop/repo@feature rm @@ -498,6 +499,16 @@ 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, including when it cannot prove there is another copy. Say `--force` if you mean it. +The guard reads `dl`'s metadata record, because the recorded directory is the +one the delete would remove. 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 finished work and removes those. diff --git a/devlaunch/dl.py b/devlaunch/dl.py index 01c69d49..9526ee5b 100644 --- a/devlaunch/dl.py +++ b/devlaunch/dl.py @@ -507,26 +507,35 @@ def workspaces_as_json(with_size: bool = False) -> int: clone_mgr = _get_clone_manager() report: List[Dict[str, Any]] = [] for ws in workspaces: - mine = is_devlaunch_clone(ws, cache_dir) + # One question asked once, and `devlaunch`, `path` and `unsaved` all read + # its answer. `_measurable_clone` returns dl's own clone directory and + # returns None exactly when the workspace is not dl's, so `mine` is + # derived from the same value the other two are rather than computed + # beside them -- which is what makes "`unsaved: null` iff `devlaunch: + # false`" hold by construction instead of by a second gate that has to + # agree with the first. It was two gates, and dropping either one of them + # left the whole suite green. + clone_path = _measurable_clone(ws, cache_dir) + mine = clone_path is not None record = clone_mgr.storage.get_worktree_by_workspace_id(ws.id) if mine else None - # Which directory this row is about. The record's path when there is a - # record -- that is the one `dl rm`'s guard reads, and a listing - # that described a different directory from the guard would be worse - # than useless to the caller deciding whether to call it. Otherwise the - # clone dl owns on disk, which is the same directory `disk` measures - # below and the same one `--purge` deletes. - # - # Gating this on the record alone was the sentinel bug of devlaunch#171 - # surviving one layer out. A clone under the cache that dl has no record - # for -- a metadata write that failed, a record pruned, a cache restored - # without one -- reported `devlaunch: true` and a measured `disk`, and - # `unsaved: null` beside them. `null` is documented as "not dl's clone", - # which that clone is not: dl says it is its own in the same object. So - # `null` still meant both "not mine" and "mine, never examined", which - # is the conflation this ticket exists to remove. This is the identical - # divergence #165 fixed for `disk` (see the comment on `_measurable_clone` - # below); `unsaved` was left behind by it. - clone_path = Path(record.local_path) if record else _measurable_clone(ws, cache_dir) + if record is not None: + # A record moves the row onto the recorded directory -- that is the + # one `dl rm`'s guard reads, and a listing that described a + # different directory from the guard would be worse than useless to + # the caller deciding whether to call it. It never makes the row + # about a workspace dl does not own, because there is no record to + # read unless `mine` already said so. + # + # Reading the record *first* and falling back to the clone was the + # sentinel bug of devlaunch#171 surviving one layer out. A clone + # under the cache that dl has no record for -- a metadata write that + # failed, a record pruned, a cache restored without one -- reported + # `devlaunch: true` and a measured `disk`, and `unsaved: null` beside + # them. `null` is documented as "not dl's clone", which that clone is + # not: dl says it is its own in the same object. This is the + # identical divergence #165 fixed for `disk` (see the comment on + # `_measurable_clone` below); `unsaved` was left behind by it. + clone_path = Path(record.local_path) state = workspace_state.read_clone(clone_path) if clone_path else None row: Dict[str, Any] = { "id": ws.id, @@ -544,8 +553,9 @@ def workspaces_as_json(with_size: bool = False) -> int: # A nested object with one key, and the key says which kind of # answer it is -- the shape `disk` uses, for the same reason # (devlaunch#171). `null` keeps only its other meaning, and keeps it - # exactly: `clone_path` above is None precisely when `mine` is - # false, so `unsaved: null` and `devlaunch: false` are the same set. + # exactly: `mine` above is defined as `clone_path is not None`, so + # `unsaved: null` and `devlaunch: false` are the same set by + # construction rather than by agreement. "unsaved": workspace_state.unsaved_as_json(state.unsaved) if state else None, } if with_size: diff --git a/devlaunch/workspace_state.py b/devlaunch/workspace_state.py index 07574fe5..975e33a2 100644 --- a/devlaunch/workspace_state.py +++ b/devlaunch/workspace_state.py @@ -231,15 +231,29 @@ def _git(repo: Path, *args: str) -> GitAnswer: ``--work-tree`` earns its place separately from ``--git-dir``, and the suite goes red without it. ``core.worktree`` in the clone's own config points the - work tree at another directory, and ``--git-dir`` alone honours it: on a - clone holding an untracked file, ``git status --porcelain`` then prints - **nothing at rc 0** — "nothing to lose", about a directory nobody asked - about. That is devlaunch#171's failure class reached by a second route, and - it is the fail-*open* one. ``core.bare = true`` is the neighbouring shape and - fails closed instead (``fatal: this operation must be run in a work tree``, - which is a refusal), which is why only the ``core.worktree`` shape - demonstrates the flag is load-bearing. Pinned by - ``TestGitIsPinnedToItsWorkTreeToo`` in ``test/test_workspace_state.py``. + work tree at another directory, and ``--git-dir`` alone honours it — so + ``git status --porcelain`` compares the clone's index against *that* + directory. What comes back depends on what is in it, and only one of the two + outcomes is a fail-open. Both were run against git 2.55.0 on the fixture the + tests build (a clone on a pushed branch, ``README.md`` and ``feature.txt`` + tracked, an untracked ``an-hour-of-work.md`` in the clone): + + - the other directory does **not** hold HEAD's files — an empty directory is + the easiest case — and git prints ``" D README.md\\n D feature.txt"`` at + rc 0. That is a :class:`WouldLose` naming two files that are not missing, + about a directory nobody asked about: wrong, and worth fixing, but it is a + *refusal*, so it does not destroy anything. + - the other directory **mirrors HEAD** — a second checkout of the same + commit, which is what ``core.worktree`` is normally pointed at — and git + prints **nothing at rc 0**. A :class:`NothingToLose` on a clone holding an + hour of work that exists nowhere else. That is devlaunch#171's failure + class reached by a second route, and it is the fail-*open* one. + + So the mirrored shape is the one that makes ``--work-tree`` load-bearing, + and it is the one ``TestGitIsPinnedToItsWorkTreeToo`` builds in + ``test/test_workspace_state.py``. ``core.bare = true`` is the neighbouring + shape and fails closed as well (``fatal: this operation must be run in a + work tree``, a refusal). ``GIT_CEILING_DIRECTORIES`` was the other candidate and is not used: it bounds discovery instead of switching it off, so it has to be an absolute @@ -296,25 +310,42 @@ def read_clone(clone: Path) -> CloneState: has no way to give it — which is why the ``os.stat`` below is written out rather than left as ``clone.is_dir()``. ``is_dir()`` collapses every failure into ``False``, and it does not even do that consistently across the - versions this package supports: on Python ≤3.12 it swallows ENOENT, ENOTDIR, - EBADF and ELOOP and *re-raises* the rest, so a clone whose parent is mode - ``000`` raised ``PermissionError`` straight out of here (``dl rm`` - failed closed by crashing; ``dl --ls --json`` became a traceback for the - whole listing because of one workspace). On 3.13+ the same call returns - ``False`` instead, which read as "not there, so nothing to lose" — a clone - that may be full of work, reported as free to delete, because dl was not - allowed to look. One sentinel each way, from the same expression. + versions this package supports: up to and including Python 3.13 it swallows + ENOENT, ENOTDIR, EBADF and ELOOP and *re-raises* the rest, so a clone whose + parent is mode ``000`` raised ``PermissionError`` straight out of here + (``dl rm`` failed closed by crashing; ``dl --ls --json`` became a + traceback for the whole listing because of one workspace). On 3.14 the same + call returns ``False`` instead, which read as "not there, so nothing to + lose" — a clone that may be full of work, reported as free to delete, + because dl was not allowed to look. One sentinel each way, from the same + expression. + + The boundary is 3.14, and it is written from execution rather than from the + changelog: the mode-``000`` parent was run on this repo's own environments, + **3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise; 3.14.6 returns ``False``**. + Patch levels between those were not run, so "3.13 and earlier" is the + minor-version claim those five interpreters support, not a claim about every + release. Every one of them is now in the ``ci`` matrix, 3.14 included — it + was not, which is how a boundary off by a whole minor version survived here + for two rounds of review. ``os.stat`` raises for all of them and the errno says which: ENOENT and ENOTDIR mean there is no directory there to hold anything, and everything else means dl was stopped before it could find out, which is exactly a :class:`CouldNotTell`. The answer is now the same on every supported Python. + + ``ValueError`` is caught alongside ``OSError`` because ``os.stat`` is total + over the first and not over the second: a path with a NUL byte in it — which + a hand-edited or truncated ``metadata.json`` can put in a record — is not an + errno, it is a ``ValueError`` raised before the syscall. Uncaught it takes + down the whole of ``dl --ls --json`` for one bad record, which is the exact + harm the stat guard was written to stop. """ try: present = stat.S_ISDIR(os.stat(clone).st_mode) except (FileNotFoundError, NotADirectoryError): return CloneState(branch=None, unsaved=NothingToLose()) - except OSError as e: + except (OSError, ValueError) as e: logger.debug(f"could not look at {clone}: {e}") return CloneState(branch=None, unsaved=CouldNotTell(f"could not look at {clone}: {e}")) if not present: diff --git a/test/test_workspace_state.py b/test/test_workspace_state.py index f3d8af5f..582a41d5 100644 --- a/test/test_workspace_state.py +++ b/test/test_workspace_state.py @@ -21,6 +21,7 @@ import json import os +import shutil import subprocess from pathlib import Path from unittest.mock import MagicMock, patch @@ -307,41 +308,94 @@ class TestGitIsPinnedToItsWorkTreeToo: The flags are not one belt and one brace. `core.worktree` in a clone's own config points the work tree at another directory, and `--git-dir` alone - honours it: `git status --porcelain` then reports on *that* directory, which - on a clone holding an untracked file means empty output at rc 0 -- "nothing - to lose", about a directory nobody asked about. Devlaunch#171's failure - class reached by a second route, and the fail-*open* one. - - Dropping `--work-tree` and keeping `--git-dir` left the rest of this suite - green, so this class exists to make that mutation red. Its neighbour - `core.bare = true` does not: with it, `--git-dir` alone says `fatal: this - operation must be run in a work tree`, which is a refusal and therefore - already safe. Only the `core.worktree` shape shows the flag is load-bearing. + honours it -- so `git status --porcelain` compares the clone's index against + *that* directory. Dropping `--work-tree` and keeping `--git-dir` left the + rest of this suite green, so this class exists to make that mutation red. + + What the mutation does depends on what the other directory holds, and the + first version of this class pinned the wrong one of the two. Run against git + 2.55.0: + + - the other directory does not hold HEAD's files (an empty directory) and + git prints `" D README.md\\n D feature.txt"` at rc 0 -- a `WouldLose` + naming two files that are not missing. Wrong, but a *refusal*: it destroys + nothing, and a test that only checks which filenames appear is red under + the mutation for a reason that is not the safety property. + - the other directory mirrors HEAD (a second checkout of the same commit, + which is what `core.worktree` is normally pointed at) and git prints + nothing at rc 0 -- `NothingToLose()` on a clone holding an hour of work + that exists nowhere else. Devlaunch#171's failure class reached by a + second route, and the fail-*open* one. + + `test_a_clone_whose_other_work_tree_mirrors_head...` builds the second shape, + so it goes red on the arm rather than on a filename. Its neighbour + `core.bare = true` is not a third case: with it, `--git-dir` alone says + `fatal: this operation must be run in a work tree` at rc 128, which is a + refusal and therefore already safe. """ - def test_a_clone_pointed_at_another_work_tree_still_reports_its_own_files( - self, clone, tmp_path - ): - elsewhere = tmp_path / "elsewhere" + @staticmethod + def _pointed_elsewhere(clone: Path, elsewhere: Path, mirror_head: bool) -> None: elsewhere.mkdir() + if mirror_head: + # A checkout of the same commit, built from what git says is tracked + # rather than from a list written here, so it mirrors HEAD by + # construction and stays mirrored if the fixture gains a file. + for tracked in git("ls-files", cwd=clone).splitlines(): + (elsewhere / tracked).write_bytes((clone / tracked).read_bytes()) git("config", "core.worktree", str(elsewhere), cwd=clone) + + def test_a_clone_whose_other_work_tree_mirrors_head_still_reports_its_own_work( + self, clone, tmp_path + ): + """The fail-open, and the assertion that matters is the arm. + + Without `--work-tree` git looks at the mirror, finds it identical to the + index, and says nothing at rc 0: `NothingToLose()` on the clone below, + which holds a file that exists nowhere else. `isinstance` is what goes + red; the filename is checked afterwards because a `WouldLose` about the + wrong directory would be a different defect. + """ + self._pointed_elsewhere(clone, tmp_path / "elsewhere", mirror_head=True) (clone / "an-hour-of-work.md").write_text("half a plan\n") unsaved = holds_unsaved_work(clone) assert isinstance(unsaved, WouldLose), unsaved assert "an-hour-of-work.md" in unsaved.description + def test_a_clean_clone_is_not_made_dirty_by_the_other_work_trees_absences( + self, clone, tmp_path + ): + # The other outcome, pinned so it cannot be mistaken for the one above: + # an empty other work tree makes `--git-dir` alone report HEAD's files as + # deleted, so this clone -- which is clean -- would be refused for work + # it does not hold. Also red under the mutation, and for the complaint a + # user would actually file. + self._pointed_elsewhere(clone, tmp_path / "elsewhere", mirror_head=False) + assert isinstance(holds_unsaved_work(clone), NothingToLose) + class TestADirectoryThatCannotBeLookedAt: """Not knowing whether the clone is there is not knowing what it holds. A clone whose parent directory `dl` has no search permission on. This used to be `clone.is_dir()`, which gave two different wrong answers depending on - which Python was running it: on ≤3.12 it re-raised `PermissionError`, so - `dl rm` failed closed by crashing and `dl --ls --json` became a - traceback for the whole listing because of one workspace; on 3.13+ the same - call returns `False`, which read as "not there, so nothing to lose" -- a - clone that may be full of work, reported as free to delete. This suite runs - on both, so a version-independent assertion is the point of this class. + which Python was running it: up to and including 3.13 it re-raised + `PermissionError`, so `dl rm` failed closed by crashing and + `dl --ls --json` became a traceback for the whole listing because of one + workspace; on 3.14 the same call returns `False`, which read as "not there, + so nothing to lose" -- a clone that may be full of work, reported as free to + delete. A version-independent assertion is the point of this class. + + The boundary is 3.14 and it was executed, not read off a changelog: the + mode-`000` parent below was run on this repo's own environments and + **3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise; 3.14.6 returns `False`**. It + was written here as "3.13+" for two rounds of review, which is a whole minor + version out, and this suite could not catch that because the `ci` matrix in + `.github/workflows/ci.yml` ran py310 through py313 only -- the leg that + returns `False`, the fail-*open* one this class exists for, was exercised by + no CI job. The matrix now includes the `default` environment, which is 3.14, + so "this suite runs on both sides of the boundary" is true of GitHub rather + than only of whoever ran it locally. """ def test_a_clone_behind_a_closed_door_is_could_not_tell(self, tmp_path): @@ -359,6 +413,15 @@ def test_a_clone_behind_a_closed_door_is_could_not_tell(self, tmp_path): assert isinstance(state.unsaved, CouldNotTell), state assert str(clone) in state.unsaved.reason + def test_a_recorded_path_that_is_not_a_path_at_all_is_could_not_tell(self, tmp_path): + # `os.stat` is total over `OSError` and not over `ValueError`: a NUL byte + # in the path is rejected before the syscall, and a hand-edited or + # truncated `metadata.json` is how one gets into a record. Uncaught it + # takes the whole of `dl --ls --json` down for one bad row, which is the + # harm the stat guard was written to stop. + state = read_clone(Path(f"{tmp_path / 'ws'}\0truncated")) + assert isinstance(state.unsaved, CouldNotTell), state + class TestTheAnswersAreTotal: """Nothing may reach a caller as an answer the caller does not name. @@ -405,13 +468,31 @@ def __init__(self, owner, repo, branch, local_path): def _clone_manager(records: dict) -> MagicMock: manager = MagicMock() manager.storage.get_worktree_by_workspace_id.side_effect = records.get + + def remove_workspace_by_id(workspace_id: str) -> bool: + """What `WorkspaceCloneManager.remove_workspace_by_id` does, and only that. + + Both halves, because the interesting one is the refusal: with no record + it removes nothing and returns False (`worktree/workspace_clone.py`), so + a workspace dl has no record for keeps its clone whatever `rm` returns. + A `MagicMock` returns a truthy `Mock` there and deletes nothing, which + makes "the clone is still on disk" true of every test for the wrong + reason. + """ + record = records.get(workspace_id) + if record is None: + return False + shutil.rmtree(record.local_path, ignore_errors=True) + return True + + manager.remove_workspace_by_id.side_effect = remove_workspace_by_id return manager class TestTheJsonListing: """`dl --ls --json`: the facts a cleanup tool decides from.""" - def _run(self, tmp_path, clone, capsys, recorded=True): + def _run(self, tmp_path, clone, capsys, recorded=True, foreign_recorded=False): cache = tmp_path / "cache" / "devlaunch" listing = json.dumps( [ @@ -422,6 +503,14 @@ def _run(self, tmp_path, clone, capsys, recorded=True): records = ( {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} if recorded else {} ) + if foreign_recorded: + # dl wrote a record and the workspace still reads as someone else's. + # `is_devlaunch_clone`'s docstring names the way in: a `config.toml` + # pointing `repos_dir` outside the cache puts dl's clones somewhere + # `--purge` does not reach, so they are foreign by the only test that + # keeps ownership and deletion answering to the same directory -- + # and the record dl wrote for them outlives the config change. + records["someone-elses"] = FakeRecord("blooop", "other", "main", str(clone)) def devpod(args, **kwargs): if args[:1] == ["list"]: @@ -531,6 +620,38 @@ def test_a_workspace_devlaunch_did_not_make_says_so_instead_of_guessing( # And nothing inspected it: dl has no clone of its own to protect there. assert foreign["unsaved"] is None + def test_a_leftover_record_cannot_make_a_foreign_workspace_dls_own( + self, tmp_path, clone, capsys + ): + """`unsaved: null` iff `devlaunch: false`, on the input that can break it. + + The two fields used to come from two questions -- `is_devlaunch_clone` + for `devlaunch`, the metadata record for the directory `unsaved` + describes -- and the claim that they name the same set was a claim that + the two agree. They do not have to: a record can outlive the reason its + workspace counts as dl's, so a `devlaunch: false` row with a record + behind it reported a `wouldLose` about a directory the same row called + `path: null`, on a checkout dl has no business inspecting. Nothing here + caught that -- deleting the ownership gate on the record lookup left the + suite green. + + `mine` is now defined as "there is a clone dl owns here", and the record + can only move the row from one directory of dl's to another, so this row + cannot be reached by a record at all. + """ + _code, report = self._run(tmp_path, clone, capsys, foreign_recorded=True) + foreign = next(w for w in report if w["id"] == "someone-elses") + assert foreign["devlaunch"] is False + assert foreign["unsaved"] is None + # And none of the fields a record would have filled in, either -- above + # all `path`, which is what `unsaved` would have been about. + assert foreign["path"] is None + assert foreign["repo"] is None and foreign["branch"] is None + assert foreign["checkedOut"] is None + # The row that is dl's is unaffected: this is not "inspect nothing". + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert ours["devlaunch"] is True and ours["unsaved"] == {"nothingToLose": True} + def test_a_checked_out_branch_that_differs_from_the_record_is_reported_too( self, tmp_path, clone, capsys ): @@ -683,12 +804,14 @@ def test_the_size_column_lines_up_with_its_heading(self, tmp_path, capsys): class TestTheDeleteGuard: """`dl rm` refuses to destroy the only copy of something.""" - def _run(self, tmp_path, clone, argv, record_read_raises=None): + def _run(self, tmp_path, clone, argv, record_read_raises=None, recorded=True): cache = tmp_path / "cache" / "devlaunch" listing = json.dumps( [_entry("r-feature-aaa", cache / "repos" / "blooop" / "r" / "r-feature-aaa")] ) - records = {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} + records = ( + {"r-feature-aaa": FakeRecord("blooop", "r", "feature", str(clone))} if recorded else {} + ) deleted = [] def devpod(args, **kwargs): @@ -795,6 +918,39 @@ def test_an_answer_this_guard_does_not_name_stops_the_delete(self, tmp_path, clo with pytest.raises(AssertionError, match="Unhandled unsaved-work answer"): self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + def test_a_clone_dl_has_no_record_for_is_left_alone_rather_than_refused( + self, tmp_path, clone_in_the_cache, caplog + ): + """What the guard does *not* cover, pinned so README cannot overstate it. + + A clone under dl's cache with no metadata record. `dl --ls --json` + reports what it holds -- `wouldLose`, correctly -- but `rm` does not + refuse: the guard reads the record, there is none, and "dl has no record + of a clone here" is answered `NothingToLose`. So the workspace goes and + the clone stays, exit 0, no `--force` asked for. + + No work is destroyed, which is why this is not a fail-open and not fixed + here: the delete reads the same absent record and removes nothing + (`worktree/workspace_clone.py`). But it is not a refusal either, and + README said it was. What is asserted below is exactly what happens. + """ + (clone_in_the_cache / "an-hour-of-work.md").write_text("half a plan\n") + code, deleted = self._run( + tmp_path, clone_in_the_cache, ["r-feature-aaa", "rm"], recorded=False + ) + assert code == 0 + assert deleted == ["r-feature-aaa"] + assert "--force" not in caplog.text + # The work is still there, and now nothing dl records points at it. + assert (clone_in_the_cache / "an-hour-of-work.md").read_text() == "half a plan\n" + + def test_a_recorded_clone_is_removed_with_the_workspace(self, tmp_path, clone): + # The other half, so the assertion above is a fact about the record and + # not about a fake that never removes anything. + code, deleted = self._run(tmp_path, clone, ["r-feature-aaa", "rm"]) + assert code == 0 and deleted == ["r-feature-aaa"] + assert not clone.exists() + def test_a_clone_already_removed_by_hand_is_still_deleted(self, tmp_path, clone): # The reason the "not there" arm answers `NothingToLose` rather than # refusing: clearing up after a half-finished delete must not need From dc9c9c1bfa9d6e79411a90bcd0a899f4fc10dfdc Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 10 Aug 2026 14:43:13 +0100 Subject: [PATCH 4/5] Say what was executed, and pin the row that was only argued Four corrections, all text, plus one test. The causal claim that the missing 3.14 leg is why the boundary was wrong is false, and was falsified by running it: `pixi run ci` on 3.14.6 at the revision carrying the wrong text is green. The tests here assert the same answer on every version -- that is the point of them -- so no leg catches a boundary written down wrongly. Somebody executing the mode-`000` parent did. The leg stays: it catches the reversion, which is the reason `ci.yml` already gave a paragraph earlier. Removed from `workspace_state.py`, the test docstring, `ci.yml` and the CHANGELOG entry that chained the same "so". `read_clone`'s ValueError paragraph had its referents inverted: `os.stat` is total over `OSError` and not over `ValueError`, which is what its own next clause says. Named both rather than saying "the first and the second". `ci.yml` cited a CONTRIBUTING file that does not exist; the instruction is in README's Development section. And the mutation the last round left green -- skipping `read_clone` for a clone directory that is genuinely absent -- now has a row. An absent clone is `NothingToLose()`, which is why `rm` can clear away a workspace whose clone was removed by hand, and the listing has to say the same: not asking writes `unsaved: null` onto a row that says `devlaunch: true` and names a `path`, which is devlaunch#171's sentinel one layer out again. Red under that mutation with 1276 others green, and the failing assertion is the `unsaved` one. NITs: README's `rm` lead said "when the clone holds unsaved work" where the guard reads the *recorded* clone, which the paragraph below it corrects; `_measurable_clone`'s docstring still called itself the directory `--size` walks when three more fields read it; an orphaned line-wrap in `ci.yml`; and a note that `default` is the only unpinned leg, so the matrix loses 3.14 the day 3.15 solves. --- .github/workflows/ci.yml | 38 +++++++++++++++++++++--------------- CHANGELOG.md | 10 ++++++---- README.md | 6 ++++-- devlaunch/dl.py | 19 ++++++++++++------ devlaunch/workspace_state.py | 20 +++++++++++-------- test/test_workspace_state.py | 38 ++++++++++++++++++++++++++++++------ 6 files changed, 89 insertions(+), 42 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2ee845ed..4ebceb86 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,20 +44,27 @@ jobs: # 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 CONTRIBUTING tells a developer to run, was the one - # selection no CI job ran. + # ci`, the command README's Development section tells a developer to + # run, was the one selection no CI job ran. # - # That 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. Its test asserted the same thing - # on every version, so it passed either way -- and the boundary was - # documented a whole minor version out, through two rounds of review, - # because no job ran the side of it that fails open. + # 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. + # 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 @@ -123,13 +130,12 @@ jobs: # A ruleset names required checks as literal strings, so requiring the jobs # 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. + # 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ce168ad..eb5bfeaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -294,10 +294,12 @@ table and should be judged on its own merits. 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 the leg that returned `False` — the fail-open - one — was exercised by no CI job, and this entry said "3.13+" for two rounds - of review. + 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 diff --git a/README.md b/README.md index 88ca4504..5e22ef4d 100644 --- a/README.md +++ b/README.md @@ -479,8 +479,10 @@ 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 rm` refuses to delete a clone it would lose work from — when -the clone holds unsaved work, and when it cannot tell**, so a caller that forgets -to read the field is still caught: +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 the guard reads the directory the delete would remove; the case with no +record is neither, and is described below.) ``` $ dl blooop/repo@feature rm diff --git a/devlaunch/dl.py b/devlaunch/dl.py index 9526ee5b..6e5d6dde 100644 --- a/devlaunch/dl.py +++ b/devlaunch/dl.py @@ -1270,15 +1270,22 @@ def is_devlaunch_clone(workspace: Workspace, cache_dir: pathlib.Path) -> bool: def _measurable_clone(workspace: Workspace, cache_dir: pathlib.Path) -> Optional[pathlib.Path]: - """The directory `--ls --size` may walk for *workspace*, or None. + """Devlaunch's own clone directory for *workspace*, or None when it has none. + + Where the boundary "is this dl's to touch?" is kept, for every surface that + needs it. `--ls --size` and the table's `SIZE` column walk it, and a + `--ls --json` row derives three more fields from this one answer: + `devlaunch` (there is a clone of dl's), `path` (which directory) and + `unsaved` (what is in it). Asking once is what stops those from disagreeing + about the same workspace -- `disk` and then `unsaved` each used to ask a + question of their own and each came apart from its neighbours in turn + (devlaunch#165, then #171). Only devlaunch's own clones. `dl ./path` opens somebody's project directory and `dl ` opens a source devlaunch never cloned; walking either would - put an unbounded stat of a stranger's tree behind a listing command, and the - bytes would not be devlaunch's disk in any case. Measuring what devlaunch - put on disk is the scope, and this is the only place that boundary is kept: - the table and the JSON listing both ask here, so the two cannot come to - different conclusions about the same workspace. + put an unbounded stat of a stranger's tree behind a listing command, the + bytes would not be devlaunch's disk in any case, and dl has no business + reporting on the state of a checkout it did not make. Ownership is the same predicate `--purge` deletes by, not the presence of a metadata record. A record can be missing from a cache that is very much on diff --git a/devlaunch/workspace_state.py b/devlaunch/workspace_state.py index 975e33a2..0e992dcd 100644 --- a/devlaunch/workspace_state.py +++ b/devlaunch/workspace_state.py @@ -325,9 +325,12 @@ def read_clone(clone: Path) -> CloneState: **3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise; 3.14.6 returns ``False``**. Patch levels between those were not run, so "3.13 and earlier" is the minor-version claim those five interpreters support, not a claim about every - release. Every one of them is now in the ``ci`` matrix, 3.14 included — it - was not, which is how a boundary off by a whole minor version survived here - for two rounds of review. + release. Every one of them is now in the ``ci`` matrix, 3.14 included; it + was not. The wrong boundary stood here for two rounds of review and what + corrected it was somebody running the mode-``000`` parent rather than + reading about it: this function's tests assert the same answer on every + version, which is the point of them, so they are green on both sides of the + line wherever this paragraph puts it. ``os.stat`` raises for all of them and the errno says which: ENOENT and ENOTDIR mean there is no directory there to hold anything, and everything @@ -335,11 +338,12 @@ def read_clone(clone: Path) -> CloneState: :class:`CouldNotTell`. The answer is now the same on every supported Python. ``ValueError`` is caught alongside ``OSError`` because ``os.stat`` is total - over the first and not over the second: a path with a NUL byte in it — which - a hand-edited or truncated ``metadata.json`` can put in a record — is not an - errno, it is a ``ValueError`` raised before the syscall. Uncaught it takes - down the whole of ``dl --ls --json`` for one bad record, which is the exact - harm the stat guard was written to stop. + over ``OSError`` and not over ``ValueError``: a path with a NUL byte in it + — which a hand-edited or truncated ``metadata.json`` can put in a record — + is not an errno, it is a ``ValueError`` raised before the syscall, so no + errno arm can be written that catches it. Uncaught it takes down the whole + of ``dl --ls --json`` for one bad record, which is the exact harm the stat + guard was written to stop. """ try: present = stat.S_ISDIR(os.stat(clone).st_mode) diff --git a/test/test_workspace_state.py b/test/test_workspace_state.py index 582a41d5..ac821afb 100644 --- a/test/test_workspace_state.py +++ b/test/test_workspace_state.py @@ -390,12 +390,14 @@ class TestADirectoryThatCannotBeLookedAt: mode-`000` parent below was run on this repo's own environments and **3.10.20, 3.11.15, 3.12.13 and 3.13.14 raise; 3.14.6 returns `False`**. It was written here as "3.13+" for two rounds of review, which is a whole minor - version out, and this suite could not catch that because the `ci` matrix in - `.github/workflows/ci.yml` ran py310 through py313 only -- the leg that - returns `False`, the fail-*open* one this class exists for, was exercised by - no CI job. The matrix now includes the `default` environment, which is 3.14, - so "this suite runs on both sides of the boundary" is true of GitHub rather - than only of whoever ran it locally. + version out. No test here would have said so: what this class asserts is the + same on every version -- that is the point of it -- so it is green whichever + side of the boundary the prose names, and what corrected it was somebody + running the mode-`000` parent on 3.14 rather than reading about it. The `ci` + matrix in `.github/workflows/ci.yml` ran py310 through py313 only and now + includes the `default` environment, which is 3.14, so "this suite runs on + both sides of the boundary" is true of GitHub rather than only of whoever + ran it locally. """ def test_a_clone_behind_a_closed_door_is_could_not_tell(self, tmp_path): @@ -592,6 +594,30 @@ def test_a_clone_dl_owns_but_has_no_record_for_is_still_inspected( foreign = next(w for w in report if w["id"] == "someone-elses") assert foreign["devlaunch"] is False and foreign["unsaved"] is None + def test_a_clone_that_is_simply_gone_is_still_read_rather_than_assumed(self, tmp_path, capsys): + """An absent clone is an answer `read_clone` gives, not one a caller may skip. + + `dl rm` clears away a workspace whose clone was removed by hand + precisely because an absent directory is `NothingToLose()` rather than a + refusal, and this row has to say the same thing about it. Not calling + `read_clone` for a directory that is not on disk -- a plausible "don't + shell out to git for something that isn't there" edit, and the one + mutation of `dl.py`'s single-question restructure that the rest of this + class leaves green -- writes `unsaved: null` onto a row that says + `devlaunch: true` and names a `path` on the lines above it. That is + devlaunch#171's sentinel again: `null` meaning "mine, not examined" + rather than "not mine". + """ + gone = tmp_path / "cache" / "devlaunch" / "repos" / "blooop" / "r" / "r-feature-aaa" + assert not gone.exists() + _code, report = self._run(tmp_path, gone, capsys) + ours = next(w for w in report if w["id"] == "r-feature-aaa") + assert ours["devlaunch"] is True + assert ours["path"] == str(gone) + assert ours["unsaved"] == {"nothingToLose": True} + # Nothing there to have a branch checked out, and no guess about one. + assert ours["checkedOut"] is None + def test_one_unreachable_clone_does_not_take_the_whole_listing_down( self, tmp_path, clone_in_the_cache, capsys ): From b883c4a66b5de46b822d502ddde7af9b57ca1184 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 10 Aug 2026 15:13:27 +0100 Subject: [PATCH 5/5] Say what the guard reads, not what the delete removes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two clauses claimed the recorded directory is "the one the delete would remove". It is the one the *guard* reads, which is not the same thing: `remove_workspace_by_id` falls back to a derived path when the recorded one is not on disk, so the two can name different directories. That divergence is older than this guard and is tracked as #174 — which this PR's own body scopes out. Stating it as settled in the README told a reader the opposite of the ticket. Both instances corrected, and the second now points at #174 rather than writing over it. README only; no code, no behaviour. --- README.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 5e22ef4d..e986228f 100644 --- a/README.md +++ b/README.md @@ -481,8 +481,8 @@ is inspected and reported like any other.) **`dl 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 the guard reads the directory the delete would remove; the case with no -record is neither, and is described below.) +because that is the directory the guard reads; the case with no record is +neither, and is described below.) ``` $ dl blooop/repo@feature rm @@ -501,9 +501,11 @@ 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, including when it cannot prove there is another copy. Say `--force` if you mean it. -The guard reads `dl`'s metadata record, because the recorded directory is the -one the delete would remove. One case is therefore neither a refusal nor a -delete: a clone under `dl`'s cache that has **no** record — a metadata write +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