From f4ade2a98d360dd3f867ec8dc244c0ab1962ad34 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 24 Aug 2026 20:29:48 -0500 Subject: [PATCH 1/2] feat(coord): untracked is an INDEX fact, lost is a CONTENT fact (BACKLOG #1298) The archive dialog warns that untracked files "will be permanently discarded". It reasons from "not in THIS WORKTREE'S index" straight to "will be lost", and skips the question that decides it: is the content somewhere else. The two come apart for an ordinary and constant reason. A tree branched behind `main` does not have the files that landed since, so a copy of one is untracked THERE while tracked on `main` -- recoverable, and warned about anyway. Every session branched behind `main` meets this, on every file that landed since, and the prompt arrives when a seat is finishing. REPRODUCED BEFORE BUILDING, on a worktree detached at 720f9436, one commit before tests/test_ci_retry_native_crash.py landed at 6e758a87, holding main's copy of that file: git status --porcelain -> ?? tests/test_ci_retry_native_crash.py git hash-object -> 5498a64cad057ed729d7220591246f75f2d21f15 git rev-parse origin/main: -> 5498a64cad057ed729d7220591246f75f2d21f15 Identical. The dialog called that permanent loss. scripts/coord/recoverable.ps1 answers it: every untracked file classified RECOVERABLE, AT-RISK (absent from the ref), or AT-RISK (on the ref but MODIFIED here). Exit 1 if any is at risk, so it works as a check and not only as something to read. THE THIRD VERDICT IS WHY AN EXISTENCE CHECK IS NOT ENOUGH. "Is it on main" answers YES for a file whose local edit is the only thing that would be lost. I nearly shipped a test that missed it -- my first contrast case used a NEW FILENAME, which exercises "absent from main", not "on main but modified". Different arms; I redid it against the same path. TWO RULES, BOTH ABOUT WHICH WAY TO BE WRONG: * Anything the script cannot read is reported AT-RISK, never clean -- the direction occupancy.ps1 states for its own fence. A false AT-RISK costs a look; a false RECOVERABLE costs the file. * -NoFetch is safe for the same reason: a stale ref can only fail to contain something that has since landed, so it can only move a file toward AT-RISK. It cannot invent a match. The ref and its sha are printed with every run, because a verdict quoted without the ref it was computed against cannot be re-checked by whoever reads it. --untracked-files=all, because git's default collapses an untracked DIRECTORY to one entry and every file beneath it would go unexamined while the run still printed a verdict. -z, because porcelain v1 QUOTES paths containing spaces and parsing that form is a second, silently different unescaper. Both are pinned by tests. THE PARTITION GUARD WAS RUN AS A POSITIVE CONTROL, NOT ASSUMED. With the new module unregistered, tests/test_tooling_partition.py went RED naming it; after adding the manifest line, `-m tooling` selects all 7 and `-m "not tooling"` deselects all 7. An unclassified test module is BACKLOG #1262's defect and it does not announce itself. Verification, with scope: tests/test_coord_recoverable.py (7), plus test_tooling_partition.py, test_ci_tooling_gate.py, test_citation_line_check.py, test_dangling_citation_check.py and test_backlog_citation_check.py -- 114 passed, on .venv/Scripts/python.exe (CPython 3.14.6 non-freethreaded, the seven CI extras). ruff 0.15.22 (== the constraints.lock pin) check and format --check clean; mypy strict clean. recoverable.ps1 parses with 0 AST errors. NOT the full suite. SCOPE THE ROW STATES AND THIS COMMIT HONOURS: the dialog is the Claude Code harness and is NOT this repository's code. Nothing here changes its wording; this answers the question it raises but cannot itself answer. No test was removed or weakened. The ledger row is not mine to author and is not in this commit. Co-Authored-By: Claude Opus 5 --- docs/WORKTREES.md | 53 +++++++ scripts/coord/recoverable.ps1 | 190 +++++++++++++++++++++++ tests/test_coord_recoverable.py | 265 ++++++++++++++++++++++++++++++++ tests/tooling_manifest.txt | 1 + 4 files changed, 509 insertions(+) create mode 100644 scripts/coord/recoverable.ps1 create mode 100644 tests/test_coord_recoverable.py diff --git a/docs/WORKTREES.md b/docs/WORKTREES.md index a9e9fded..4024b859 100644 --- a/docs/WORKTREES.md +++ b/docs/WORKTREES.md @@ -112,6 +112,59 @@ destructive script in this directory can be **execution-tested** repo); without it the only repository a test could reach was this one, so the branch-delete path was covered by review alone. (BACKLOG #1037.) +### "Will be permanently discarded" is an INDEX test, not a LOSS test — `recoverable.ps1` + +**The archive dialog's warning about untracked files is wrong in the common case, and the common +case is a worktree branched behind `main`.** It reasons from *not in this worktree's index* straight +to *will be lost*, and skips the question that decides it: **is the content somewhere else.** + +A tree cut before a file landed does not have that file in its index. A copy of it sitting in the +tree is therefore untracked **there** while tracked on `main` — recoverable, and named in the +warning anyway. Every session branched behind `main` meets this, on every file that landed since, +and the prompt arrives exactly when a seat is trying to finish. + +Measured on this repository 2026-08-24, on a tree detached at `720f9436`, one commit before +`tests/test_ci_retry_native_crash.py` landed at `6e758a87`, holding `main`'s copy of that file: + +``` +git status --porcelain -> ?? tests/test_ci_retry_native_crash.py +git hash-object -> 5498a64cad057ed729d7220591246f75f2d21f15 +git rev-parse origin/main: -> 5498a64cad057ed729d7220591246f75f2d21f15 +``` + +Identical. The dialog called that permanent loss. + +**Answer it with [`recoverable.ps1`](../scripts/coord/recoverable.ps1) rather than by eye:** + +``` +pwsh -NoProfile -File scripts\coord\recoverable.ps1 # this worktree +pwsh -NoProfile -File scripts\coord\recoverable.ps1 -Worktree

-Json +``` + +It classifies every untracked file into one of three, and exits non-zero if any is at risk: + +| Verdict | Means | +|---|---| +| `RECOVERABLE` | byte-identical to the ref — the warning is wrong about this file | +| `AT-RISK` — absent from the ref | genuinely nowhere else | +| `AT-RISK` — on the ref but MODIFIED | the path is on `main`, **the local edit is not** | + +**The third row is why an existence check is not enough.** "Is it on main" answers *yes* for a file +whose local edit is the only thing that would be lost. + +**Two rules it is built on, and both are about which way to be wrong.** + +- **Anything it cannot read is reported AT-RISK, never clean** — the same direction + [`occupancy.ps1`](../scripts/coord/occupancy.ps1) states for its own fence. A false `AT-RISK` + costs a look; a false `RECOVERABLE` costs the file. +- **`-NoFetch` is safe for the same reason.** A stale ref can only fail to contain something that + has since landed, so it can only move a file from `RECOVERABLE` to `AT-RISK`. It cannot invent a + match. The ref and its sha are printed with every run, because a verdict quoted without the ref it + was computed against cannot be re-checked. + +**The dialog is the Claude Code harness, not this repository's code.** Nothing here changes its +wording; this answers the question it raises but cannot itself answer. (BACKLOG #1298.) + ## Prune the finished ones — `prune-merged.ps1` Worktrees pile up. [`prune-merged.ps1`](../scripts/worktree/prune-merged.ps1) sweeps the finished diff --git a/scripts/coord/recoverable.ps1 b/scripts/coord/recoverable.ps1 new file mode 100644 index 00000000..0903999d --- /dev/null +++ b/scripts/coord/recoverable.ps1 @@ -0,0 +1,190 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +<# +.SYNOPSIS + Answer whether this worktree's UNTRACKED files would actually be lost, which "untracked" does + not tell you (BACKLOG #1298). + +.DESCRIPTION + The archive dialog warns that untracked files "will be permanently discarded". That reasons from + "not in THIS WORKTREE'S INDEX" straight to "will be lost", and skips the only question that + decides it: is the content somewhere else. It is an INDEX test presented as a LOSS test. + + THE TWO ARE DIFFERENT FOR AN ORDINARY AND CONSTANT REASON. A worktree whose base is behind `main` + does not have the files that landed since it branched. Any copy of one of those files sitting in + the tree is untracked THERE while being tracked on `main` -- so the file is fully recoverable and + the warning is wrong. Every session branched behind `main` meets this, on every file that landed + since, and the prompt arrives at exactly the moment a seat is trying to finish. + + REPRODUCED ON THIS REPOSITORY 2026-08-24, which is what establishes the mechanism rather than a + coincidence. A worktree detached at 720f9436, one commit before `tests/test_ci_retry_native_crash.py` + landed at 6e758a87, with `main`'s copy of that file placed in it: + + git status --porcelain -> ?? tests/test_ci_retry_native_crash.py + worktree blob -> 5498a64cad057ed729d7220591246f75f2d21f15 + origin/main blob -> 5498a64cad057ed729d7220591246f75f2d21f15 + + Identical. The dialog called that permanent loss. + +.NOTES + THE DIALOG IS THE CLAUDE CODE HARNESS AND IS NOT THIS REPOSITORY'S CODE. Nothing here can change + its wording, and a change that tries has misread the item. This script exists so a human or a + session can answer the question the dialog raises but cannot itself answer. + + ANYTHING THIS SCRIPT CANNOT READ IS REPORTED AT RISK, NEVER CLEAN. That is the same direction + occupancy.ps1 states for its own fence -- an unplaceable record makes the whole fence unavailable, + because the failure that cannot be attributed is exactly the one that might be destroyed. Here the + cost of the two errors is not symmetric: a false AT RISK costs a look, a false RECOVERABLE costs + the file. + + NOT FETCHING ERRS THE SAME WAY, WHICH IS WHY -NoFetch IS SAFE. A stale `origin/main` can only + fail to contain something that has since landed, so it can only move a file from RECOVERABLE to + AT RISK. It cannot invent a match. The ref actually compared against is printed with every run, + because a verdict without the ref it was computed against is not a result. + +.PARAMETER Worktree + The worktree to examine. Defaults to the current one. + +.PARAMETER Ref + What to consider "somewhere else". Defaults to origin/main. + +.PARAMETER NoFetch + Skip the fetch. See the note above: this can only over-warn. + +.PARAMETER Json + Emit the rows as JSON instead of a table. + +.EXAMPLE + pwsh -NoProfile -File scripts\coord\recoverable.ps1 + pwsh -NoProfile -File scripts\coord\recoverable.ps1 -Worktree C:\path\to\wt -NoFetch +#> +[CmdletBinding()] +param( + [string]$Worktree, + [string]$Ref = "origin/main", + [switch]$NoFetch, + [switch]$Json +) + +$ErrorActionPreference = 'Stop' + +# NOTE ON $LASTEXITCODE, since every git call below leans on it: a non-zero exit is a RESULT here, +# not an error. `cat-file -e` exits 1 for a path that is simply absent from the ref, which is one of +# the three verdicts. $ErrorActionPreference='Stop' does not turn a native exit code into a throw, +# so the calls are made directly and their exit code read immediately after. + +if (-not $Worktree) { $Worktree = (Get-Location).Path } +if (-not (Test-Path -LiteralPath $Worktree)) { throw "no such worktree: $Worktree" } +$script:Root = (Resolve-Path -LiteralPath $Worktree).Path + +$top = & git -C $script:Root rev-parse --show-toplevel 2>$null +if ($LASTEXITCODE -ne 0 -or -not $top) { throw "not inside a git worktree: $script:Root" } +$script:Root = $top + +if (-not $NoFetch) { + & git -C $script:Root fetch origin --quiet 2>$null | Out-Null +} + +$refSha = & git -C $script:Root rev-parse --verify --quiet "$Ref" 2>$null +if (-not $refSha) { + throw "cannot resolve $Ref in $script:Root -- refusing to judge anything against a ref that does not exist" +} + +# --untracked-files=all, because the default collapses an untracked DIRECTORY to a single entry and +# every file beneath it would then go unexamined while the run still reported a clean verdict. +# -z, because a path may contain a space, and porcelain v1 QUOTES such paths rather than emitting +# them raw -- parsing the quoted form is a second, silently different unescaper. +$raw = & git -C $script:Root status --porcelain --untracked-files=all -z 2>$null +$entries = @() +if ($raw) { $entries = ($raw -split "`0") | Where-Object { $_ } } + +$rows = @() +foreach ($e in $entries) { + if ($e.Length -lt 4 -or $e.Substring(0, 2) -ne '??') { continue } + $path = $e.Substring(3) + + $verdict = $null + $detail = $null + $wtHash = $null + $refHash = $null + + & git -C $script:Root cat-file -e "${Ref}:${path}" 2>$null | Out-Null + $onRef = ($LASTEXITCODE -eq 0) + + if (-not $onRef) { + $verdict = 'AT-RISK' + $detail = "absent from $Ref" + } + else { + $refHash = & git -C $script:Root rev-parse "${Ref}:${path}" 2>$null + $wtHash = & git -C $script:Root hash-object -- "$path" 2>$null + if ($LASTEXITCODE -ne 0 -or -not $wtHash -or -not $refHash) { + # UNREADABLE COUNTS AS AT RISK. It is on the ref, so it is tempting to call it clean -- + # but we could not read the working copy, so we do not know it is the same content, and + # the one thing we must never do is tell someone a file is safe when we could not look. + $verdict = 'AT-RISK' + $detail = 'UNREADABLE -- could not hash the working copy; treated as at risk, not as clean' + } + elseif ($wtHash -eq $refHash) { + $verdict = 'RECOVERABLE' + $detail = "identical to $Ref" + } + else { + $verdict = 'AT-RISK' + $detail = "on $Ref but MODIFIED here" + } + } + + $rows += [pscustomobject]@{ + Path = $path + Verdict = $verdict + Detail = $detail + WorktreeSha = $wtHash + RefSha = $refHash + } +} + +$atRisk = @($rows | Where-Object { $_.Verdict -eq 'AT-RISK' }) +$recoverable = @($rows | Where-Object { $_.Verdict -eq 'RECOVERABLE' }) + +if ($Json) { + [pscustomobject]@{ + Worktree = $script:Root + Ref = $Ref + RefSha = $refSha + Fetched = (-not $NoFetch) + Untracked = $rows.Count + AtRisk = $atRisk.Count + Recoverable = $recoverable.Count + Rows = $rows + } | ConvertTo-Json -Depth 5 +} +else { + Write-Output "worktree : $script:Root" + # THE REF IS PART OF THE MEASUREMENT, not decoration. A verdict quoted without the ref it was + # computed against cannot be re-checked by whoever reads it. + Write-Output "compared : $Ref at $refSha$(if ($NoFetch) { ' (NOT fetched -- can only over-warn)' })" + Write-Output "" + if ($rows.Count -eq 0) { + Write-Output "no untracked files. Nothing for the archive warning to be about." + } + else { + foreach ($r in ($rows | Sort-Object Verdict, Path)) { + Write-Output ("{0,-12} {1}" -f $r.Verdict, $r.Path) + Write-Output (" {0}" -f $r.Detail) + } + Write-Output "" + Write-Output ("untracked {0}: {1} AT-RISK, {2} recoverable." -f $rows.Count, $atRisk.Count, $recoverable.Count) + if ($atRisk.Count -eq 0) { + Write-Output "Every untracked file here is already on $Ref, byte for byte. The archive warning is wrong about all of them." + } + else { + Write-Output "The AT-RISK rows are the only ones the archive warning actually describes. Commit or copy them first." + } + } +} + +# Exit 1 when anything is at risk, so this can be used as a check and not only read by a human. +# Nothing at risk -- including the no-untracked-files case -- exits 0. +if ($atRisk.Count -gt 0) { exit 1 } +exit 0 diff --git a/tests/test_coord_recoverable.py b/tests/test_coord_recoverable.py new file mode 100644 index 00000000..e894d683 --- /dev/null +++ b/tests/test_coord_recoverable.py @@ -0,0 +1,265 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Untracked is an INDEX fact; lost is a CONTENT fact (BACKLOG #1298). + +The archive dialog warns that untracked files "will be permanently discarded". It reasons from "not +in THIS WORKTREE'S index" straight to "will be lost", and skips the only question that decides it: +is the content somewhere else. + +The two come apart for an ordinary and constant reason. A worktree branched behind ``main`` does not +have the files that landed since, so a copy of one of them sitting in that tree is untracked THERE +while tracked on ``main`` -- fully recoverable, and the warning is wrong about it. + +**Every test below pairs the reassuring answer with the alarming one over the SAME command.** A +helper that always said RECOVERABLE would pass a suite that only ever fed it recoverable files, and +that is the failure mode that matters here: a false RECOVERABLE costs the file, while a false +AT-RISK costs a look. The three arms -- absent, modified, identical -- are asserted to produce three +different verdicts, so the cases cannot funnel to one assertion. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT = ROOT / "scripts" / "coord" / "recoverable.ps1" +TIMEOUT = 120 + +pytestmark = pytest.mark.skipif( + shutil.which("pwsh") is None or os.name != "nt", + reason="recoverable.ps1 needs pwsh on Windows", +) + + +def git(repo: Path, *args: str) -> str: + proc = subprocess.run( + ["git", "-C", str(repo), *args], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=True, + ) + return proc.stdout + + +def run_helper(worktree: Path, ref: str = "origin/main") -> tuple[int, dict[str, object]]: + """Drive the REAL script as a subprocess, as -Json, and return (exit code, payload).""" + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(SCRIPT), + "-Worktree", + str(worktree), + "-Ref", + ref, + "-NoFetch", + "-Json", + ], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + assert proc.stdout.strip(), f"helper produced no stdout:\n{proc.stderr}" + return proc.returncode, dict(json.loads(proc.stdout)) + + +def verdicts(payload: dict[str, object]) -> dict[str, str]: + rows = payload["Rows"] + assert isinstance(rows, list) + out: dict[str, str] = {} + for r in rows: + out[str(r["Path"])] = str(r["Verdict"]) + return out + + +@pytest.fixture +def upstream_and_behind(tmp_path: Path) -> tuple[Path, Path]: + """An 'origin' whose main carries a file, and a clone parked one commit BEHIND it. + + This is the shape the item is about, built rather than described: the behind-tree genuinely does + not have the later file in its index, which is what makes git call a copy of it untracked. + """ + up = tmp_path / "up" + up.mkdir() + git(up, "init", "-q", "-b", "main") + git(up, "config", "user.email", "t@example.invalid") + git(up, "config", "user.name", "t") + (up / "base.txt").write_text("base\n", encoding="utf-8") + git(up, "add", "base.txt") + git(up, "commit", "-qm", "base") + + (up / "landed.txt").write_text("landed content\n", encoding="utf-8") + git(up, "add", "landed.txt") + git(up, "commit", "-qm", "land a file") + + behind = tmp_path / "behind" + git(tmp_path, "clone", "-q", str(up), str(behind)) + git(behind, "config", "user.email", "t@example.invalid") + git(behind, "config", "user.name", "t") + # Park one commit behind: this tree's index has no landed.txt. + first = git(behind, "rev-list", "--max-parents=0", "HEAD").strip() + git(behind, "checkout", "-q", "--detach", first) + assert not (behind / "landed.txt").exists() + return up, behind + + +def test_a_landed_file_in_a_behind_worktree_is_recoverable_not_lost( + upstream_and_behind: tuple[Path, Path], +) -> None: + """The reassuring arm, and the one the dialog gets wrong.""" + _, behind = upstream_and_behind + (behind / "landed.txt").write_text("landed content\n", encoding="utf-8") + + assert "?? landed.txt" in git(behind, "status", "--porcelain"), ( + "precondition: git must call this untracked, or the test is not about the reported case" + ) + + code, payload = run_helper(behind) + print(json.dumps(payload, indent=2)) + assert verdicts(payload)["landed.txt"] == "RECOVERABLE" + assert payload["AtRisk"] == 0 + assert code == 0, "nothing at risk must exit 0, so this is usable as a check" + + +def test_a_file_absent_from_the_ref_is_at_risk( + upstream_and_behind: tuple[Path, Path], +) -> None: + """The alarming arm. Same command, same tree, opposite answer.""" + _, behind = upstream_and_behind + (behind / "genuinely_new.txt").write_text("nobody else has this\n", encoding="utf-8") + + code, payload = run_helper(behind) + print(json.dumps(payload, indent=2)) + assert verdicts(payload)["genuinely_new.txt"] == "AT-RISK" + assert payload["AtRisk"] == 1 + assert code == 1, "anything at risk must exit non-zero" + + +def test_a_file_on_the_ref_but_locally_modified_is_at_risk( + upstream_and_behind: tuple[Path, Path], +) -> None: + """The arm a name-only or existence-only check would miss. + + The path IS on the ref, so 'is it on main' answers yes and a helper that stopped there would + call it recoverable. The local edit is the thing that would actually be lost. + """ + _, behind = upstream_and_behind + (behind / "landed.txt").write_text("landed content\nplus a local edit\n", encoding="utf-8") + + code, payload = run_helper(behind) + print(json.dumps(payload, indent=2)) + assert verdicts(payload)["landed.txt"] == "AT-RISK" + assert code == 1 + + +def test_the_three_arms_do_not_collapse_to_one_answer( + upstream_and_behind: tuple[Path, Path], +) -> None: + """All three at once: a suite whose cases all produce the same string proves nothing. + + This is the test that would catch a helper hard-coded to one verdict, which every single-arm + test above would pass individually. + """ + _, behind = upstream_and_behind + (behind / "landed.txt").write_text("landed content\n", encoding="utf-8") + (behind / "genuinely_new.txt").write_text("nobody else has this\n", encoding="utf-8") + (behind / "base.txt").write_text("base\nlocally edited\n", encoding="utf-8") + + code, payload = run_helper(behind) + got = verdicts(payload) + print(json.dumps(payload, indent=2)) + + assert got["landed.txt"] == "RECOVERABLE" + assert got["genuinely_new.txt"] == "AT-RISK" + # base.txt is TRACKED and modified, so it is not untracked and not this script's subject. + assert "base.txt" not in got, ( + "a tracked-but-modified file is not what the archive warning is about; including it would " + "widen the report past the population the item names" + ) + assert payload["AtRisk"] == 1 + assert payload["Recoverable"] == 1 + assert code == 1 + + +def test_an_untracked_directory_is_expanded_rather_than_reported_as_one_entry( + upstream_and_behind: tuple[Path, Path], +) -> None: + """git's default porcelain collapses an untracked DIRECTORY to a single entry. + + Every file beneath it would then go unexamined while the run still printed a verdict, which is + the silent-undercount shape. --untracked-files=all is what prevents it, and this pins that. + """ + _, behind = upstream_and_behind + nested = behind / "newdir" / "deeper" + nested.mkdir(parents=True) + (nested / "a.txt").write_text("a\n", encoding="utf-8") + (nested / "b.txt").write_text("b\n", encoding="utf-8") + + code, payload = run_helper(behind) + got = verdicts(payload) + print(json.dumps(payload, indent=2)) + + assert "newdir/deeper/a.txt" in got, f"the directory was not expanded: {sorted(got)}" + assert "newdir/deeper/b.txt" in got, f"the directory was not expanded: {sorted(got)}" + assert payload["Untracked"] == 2 + assert code == 1 + + +def test_the_ref_it_compared_against_is_reported( + upstream_and_behind: tuple[Path, Path], +) -> None: + """A verdict without the ref it was computed against cannot be re-checked by its reader.""" + _, behind = upstream_and_behind + (behind / "landed.txt").write_text("landed content\n", encoding="utf-8") + + _, payload = run_helper(behind) + assert payload["Ref"] == "origin/main" + sha = str(payload["RefSha"]) + assert len(sha) == 40, f"expected a full sha, got {sha!r}" + assert sha == git(behind, "rev-parse", "origin/main").strip() + + +def test_an_unresolvable_ref_refuses_rather_than_reporting_everything_clean( + upstream_and_behind: tuple[Path, Path], +) -> None: + """The direction that matters: cannot-compare must never render as nothing-to-worry-about. + + If the ref does not resolve, every file is 'absent from the ref' by construction -- which would + be the correct AT-RISK answer for the wrong reason, and would look identical to a real one. The + script refuses the run instead. + """ + _, behind = upstream_and_behind + (behind / "landed.txt").write_text("landed content\n", encoding="utf-8") + + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(SCRIPT), + "-Worktree", + str(behind), + "-Ref", + "origin/no-such-branch-zzq", + "-NoFetch", + "-Json", + ], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + assert proc.returncode != 0, "an unresolvable ref must not exit 0" + combined = proc.stdout + proc.stderr + assert "refusing" in combined.lower(), f"expected an explicit refusal, got:\n{combined}" diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index 38371023..3dc18ad0 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -62,6 +62,7 @@ tests/test_coord_overlap_attribution.py tests/test_coord_overlap_cache.py tests/test_coord_overlap_signals.py tests/test_coord_presence.py +tests/test_coord_recoverable.py tests/test_coord_dispatch_gate.py tests/test_coord_seat_clock_alarm.py tests/test_coord_throughput.py From 7c3e2e9a59c62fa37d05ab3645ea31f542e69729 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 25 Aug 2026 12:39:50 -0500 Subject: [PATCH 2/2] refactor(coord): apply the quality-review findings to recoverable.ps1 (BACKLOG #1298) Follow-up to the #1298 commit, applying a four-angle review that completed after it. The review ran 4 agents over the change; none returned empty. ONE GIT PROCESS FEWER PER FILE. `rev-parse :` already exits non-zero when the path is absent from the ref, so the `cat-file -e` probe that preceded it asked a question the next line answered on its way to the sha. Two review angles found this independently. Verified: `git rev-parse origin/main:no/such/path` exits 128, `origin/main:README.md` exits 0. AND IT HAS A SHARP EDGE THE EXIT-CODE CHECK IS LOAD-BEARING FOR: rev-parse PRINTS THE SPEC ITSELF TO STDOUT on failure. `origin/main:no/such/zzq.txt` comes back on stdout with exit 128, so a truthiness test on the captured value alone would read a missing path as a valid sha. The guard tests $LASTEXITCODE first and then nulls the variable explicitly. THE PAYLOAD NOW MATCHES WHAT THE DOCS SELL. `Verdict` is deliberately BINARY -- would this be lost -- but three of its four causes are AT-RISK, so the three-way distinction the docs table advertised existed only inside `Detail`, an English sentence with the ref name interpolated into it. A consumer wanting to tell "absent" from "modified" had to parse prose. `Reason` is now a closed set: identical, absent, modified, unreadable. THE ARM THE DOCS CALLED THE WHOLE POINT WAS UNTESTED, and now is not. Mutation-verified with the harness carrying its own controls -- anchor asserted unique, file hash asserted CHANGED before scoring: collapsing `modified` into `absent` (two causes that share the AT-RISK verdict, so no Verdict assertion can see it) reds 1 of 7. Before this commit that mutation SURVIVED. STATED ONCE, NOT THREE TIMES. The measured reproduction with its shas lived in the script header, the docs subsection and the test docstring -- six sha citations maintained in two files. The docs copy is now the only one; the header points at it. (SDS-3.5.) THE DOCS SUBSECTION IS A `##`, NOT A `###` UNDER "Remove one". The question governs every way a tree goes away -- remove.ps1, prune-merged.ps1, a rescue, the archive dialog, a manual delete -- so a 53-line subsection nested under the 32-line section about the one script it does NOT concern is hidden from exactly the reader looking for it. Also: dead `$verdict = $null` / `$detail = $null` removed (every branch assigns both), while the `$wtHash` / `$refHash` resets are KEPT with a comment saying why -- without them a hash from the previous file survives into a row nobody hashed. The two pairs looked identical, which is how the next reader deletes all four. And the test's two inline row unpackers are replaced by one `by_path` helper, so the isinstance dance is written once. DELIBERATELY NOT APPLIED, and each is reported rather than silently dropped: * BATCHING THE GIT CALLS. The two angles disagree and both measured. REUSE: 300 untracked files cost 41,220 ms today against 427 ms batched, and `alloc.ps1:126` already documents this exact mistake. EFFICIENCY: the real population is N<=2, where batching buys nothing and costs the per-file UNREADABLE isolation. Both are right; which wins depends on whether this is ever pointed at a large tree, and that is not settled here. * MOVING THE FILE to scripts/worktree/. The altitude case is strong -- zero mefor-coord hits, no seat, claim, lock, mail or registry read, and its documented neighbours are remove.ps1 and prune-merged.ps1. But the dispatch brief named scripts/coord/ explicitly, and moving it against that is a scope call that is not mine to make unasked. * THE LARGEST FINDING, which is out of this item's scope entirely: fleet.ps1:367, prune-merged.ps1:570-577 and unbacked_check.ps1:273 all still assert the index-as-loss claim this script refutes, and unlike the archive dialog they ARE this repository's code. Reported to the dispatcher as content for its own row. Widening a claimed item to three unrelated files -- one of them the most destructive script in the tree -- is the cross-lane collision the file grouping exists to prevent. Verification, with scope: tests/test_coord_recoverable.py, test_tooling_partition.py, test_citation_line_check.py, test_dangling_citation_check.py -- 70 passed, on .venv/Scripts/python.exe (CPython 3.14.6 non-freethreaded, seven CI extras). ruff 0.15.22 (== the constraints.lock pin) check and format --check clean; mypy strict clean. NOT the full suite. No test was removed or weakened. The ledger row is not mine to author and is not in this commit. Co-Authored-By: Claude Opus 5 --- docs/WORKTREES.md | 29 +++++++++++------- scripts/coord/recoverable.ps1 | 52 +++++++++++++++++++++------------ tests/test_coord_recoverable.py | 31 ++++++++++++++++++-- 3 files changed, 81 insertions(+), 31 deletions(-) diff --git a/docs/WORKTREES.md b/docs/WORKTREES.md index 4024b859..7c620754 100644 --- a/docs/WORKTREES.md +++ b/docs/WORKTREES.md @@ -112,7 +112,12 @@ destructive script in this directory can be **execution-tested** repo); without it the only repository a test could reach was this one, so the branch-delete path was covered by review alone. (BACKLOG #1037.) -### "Will be permanently discarded" is an INDEX test, not a LOSS test — `recoverable.ps1` +## "Will be permanently discarded" is an INDEX test, not a LOSS test — `recoverable.ps1` + +> **A `##`, not a `###` under "Remove one".** The question governs every way a tree goes away — +> `remove.ps1`, `prune-merged.ps1`, a rescue, the archive dialog, a manual directory delete — so +> nesting it under the one script it does *not* concern hides it from a reader scanning the headings +> for exactly this. **The archive dialog's warning about untracked files is wrong in the common case, and the common case is a worktree branched behind `main`.** It reasons from *not in this worktree's index* straight @@ -141,16 +146,20 @@ pwsh -NoProfile -File scripts\coord\recoverable.ps1 # this workt pwsh -NoProfile -File scripts\coord\recoverable.ps1 -Worktree

-Json ``` -It classifies every untracked file into one of three, and exits non-zero if any is at risk: - -| Verdict | Means | -|---|---| -| `RECOVERABLE` | byte-identical to the ref — the warning is wrong about this file | -| `AT-RISK` — absent from the ref | genuinely nowhere else | -| `AT-RISK` — on the ref but MODIFIED | the path is on `main`, **the local edit is not** | +It exits non-zero if any file is at risk. Each row carries a **binary `Verdict`** — the only thing a +caller has to act on — and a **`Reason`** from a closed set, because three of the four causes are +`AT-RISK` and "absent" is not the same problem as "modified": -**The third row is why an existence check is not enough.** "Is it on main" answers *yes* for a file -whose local edit is the only thing that would be lost. +| `Verdict` | `Reason` | Means | +|---|---|---| +| `RECOVERABLE` | `identical` | byte-identical to the ref — the warning is wrong about this file | +| `AT-RISK` | `absent` | genuinely nowhere else | +| `AT-RISK` | `modified` | the path is on `main`, **the local edit is not** | +| `AT-RISK` | `unreadable` | the working copy could not be hashed — see the rules below | + +**The `modified` row is why an existence check is not enough.** "Is it on main" answers *yes* for a +file whose local edit is the only thing that would be lost. `Reason` is machine-readable on purpose: +without it a consumer would have to parse the `Detail` sentence, which interpolates the ref name. **Two rules it is built on, and both are about which way to be wrong.** diff --git a/scripts/coord/recoverable.ps1 b/scripts/coord/recoverable.ps1 index 0903999d..450e1870 100644 --- a/scripts/coord/recoverable.ps1 +++ b/scripts/coord/recoverable.ps1 @@ -16,15 +16,11 @@ the warning is wrong. Every session branched behind `main` meets this, on every file that landed since, and the prompt arrives at exactly the moment a seat is trying to finish. - REPRODUCED ON THIS REPOSITORY 2026-08-24, which is what establishes the mechanism rather than a - coincidence. A worktree detached at 720f9436, one commit before `tests/test_ci_retry_native_crash.py` - landed at 6e758a87, with `main`'s copy of that file placed in it: - - git status --porcelain -> ?? tests/test_ci_retry_native_crash.py - worktree blob -> 5498a64cad057ed729d7220591246f75f2d21f15 - origin/main blob -> 5498a64cad057ed729d7220591246f75f2d21f15 - - Identical. The dialog called that permanent loss. + The mechanism was REPRODUCED on this repository before this script was written, on a worktree + detached one commit before a file landed, holding `main`'s copy of it: git called it untracked + while the two blob hashes were identical. **The measurement, with its shas, is stated ONCE in + docs/WORKTREES.md under "Will be permanently discarded" -- read it there.** Six sha citations + maintained in two files is how the two copies drift. .NOTES THE DIALOG IS THE CLAUDE CODE HARNESS AND IS NOT THIS REPOSITORY'S CODE. Nothing here can change @@ -103,41 +99,59 @@ foreach ($e in $entries) { if ($e.Length -lt 4 -or $e.Substring(0, 2) -ne '??') { continue } $path = $e.Substring(3) - $verdict = $null - $detail = $null + # RESET PER ITERATION. Load-bearing: without these, a hash from the PREVIOUS file survives into + # this row on any branch that does not assign it, and the row then reports a sha for a file + # nobody hashed. $verdict and $reason need no reset -- every branch below assigns both. $wtHash = $null $refHash = $null - & git -C $script:Root cat-file -e "${Ref}:${path}" 2>$null | Out-Null - $onRef = ($LASTEXITCODE -eq 0) + # ONE call, not two. `rev-parse :` already exits non-zero when the path is absent + # from the ref, so the `cat-file -e` existence probe that used to sit here asked a question this + # line answers on its way to the sha -- a third of the per-file work for nothing. Verified: + # `git rev-parse origin/main:no/such/path` exits 128, `origin/main:README.md` exits 0. + $refHash = & git -C $script:Root rev-parse "${Ref}:${path}" 2>$null + $onRef = ($LASTEXITCODE -eq 0 -and $refHash) if (-not $onRef) { + $refHash = $null $verdict = 'AT-RISK' - $detail = "absent from $Ref" + $reason = 'absent' } else { - $refHash = & git -C $script:Root rev-parse "${Ref}:${path}" 2>$null $wtHash = & git -C $script:Root hash-object -- "$path" 2>$null - if ($LASTEXITCODE -ne 0 -or -not $wtHash -or -not $refHash) { + if ($LASTEXITCODE -ne 0 -or -not $wtHash) { # UNREADABLE COUNTS AS AT RISK. It is on the ref, so it is tempting to call it clean -- # but we could not read the working copy, so we do not know it is the same content, and # the one thing we must never do is tell someone a file is safe when we could not look. $verdict = 'AT-RISK' - $detail = 'UNREADABLE -- could not hash the working copy; treated as at risk, not as clean' + $reason = 'unreadable' } elseif ($wtHash -eq $refHash) { $verdict = 'RECOVERABLE' - $detail = "identical to $Ref" + $reason = 'identical' } else { $verdict = 'AT-RISK' - $detail = "on $Ref but MODIFIED here" + $reason = 'modified' } } + # REASON IS THE MACHINE-READABLE ARM AND DETAIL IS THE SENTENCE. Verdict answers the only + # question a caller has to act on -- would this be lost -- and is deliberately BINARY. But three + # of its four causes are AT-RISK, and a consumer that wants to tell "absent" from "modified" + # would otherwise have to parse English with the ref name interpolated into it. Reason is a + # closed set: identical, absent, modified, unreadable. + $detail = switch ($reason) { + 'absent' { "absent from $Ref" } + 'modified' { "on $Ref but MODIFIED here" } + 'identical' { "identical to $Ref" } + 'unreadable' { 'UNREADABLE -- could not hash the working copy; treated as at risk, not as clean' } + } + $rows += [pscustomobject]@{ Path = $path Verdict = $verdict + Reason = $reason Detail = $detail WorktreeSha = $wtHash RefSha = $refHash diff --git a/tests/test_coord_recoverable.py b/tests/test_coord_recoverable.py index e894d683..c7bc5bc9 100644 --- a/tests/test_coord_recoverable.py +++ b/tests/test_coord_recoverable.py @@ -73,15 +73,24 @@ def run_helper(worktree: Path, ref: str = "origin/main") -> tuple[int, dict[str, return proc.returncode, dict(json.loads(proc.stdout)) -def verdicts(payload: dict[str, object]) -> dict[str, str]: +def by_path(payload: dict[str, object], field: str) -> dict[str, str]: + """Map path -> one field of its row. One unpacker, so the isinstance dance is written once.""" rows = payload["Rows"] assert isinstance(rows, list) out: dict[str, str] = {} for r in rows: - out[str(r["Path"])] = str(r["Verdict"]) + out[str(r["Path"])] = str(r[field]) return out +def verdicts(payload: dict[str, object]) -> dict[str, str]: + return by_path(payload, "Verdict") + + +def reasons(payload: dict[str, object]) -> dict[str, str]: + return by_path(payload, "Reason") + + @pytest.fixture def upstream_and_behind(tmp_path: Path) -> tuple[Path, Path]: """An 'origin' whose main carries a file, and a clone parked one commit BEHIND it. @@ -159,6 +168,13 @@ def test_a_file_on_the_ref_but_locally_modified_is_at_risk( code, payload = run_helper(behind) print(json.dumps(payload, indent=2)) assert verdicts(payload)["landed.txt"] == "AT-RISK" + # The reason must be `modified`, NOT `absent`. Both are AT-RISK, so Verdict alone cannot tell + # this test from the one above it -- and an existence-only check would have said RECOVERABLE. + got_reasons = reasons(payload) + assert got_reasons["landed.txt"] == "modified", ( + f"expected the on-ref-but-edited reason, got {got_reasons}; if this says 'absent' the ref " + "lookup is failing and every file would be reported at risk for the wrong reason" + ) assert code == 1 @@ -181,6 +197,17 @@ def test_the_three_arms_do_not_collapse_to_one_answer( assert got["landed.txt"] == "RECOVERABLE" assert got["genuinely_new.txt"] == "AT-RISK" + + # AND THE REASONS MUST DIFFER, not just the verdicts. Verdict is deliberately BINARY -- three of + # its four causes are AT-RISK -- so a suite that only asserts Verdict cannot tell "absent from + # the ref" from "on the ref but modified", which is the distinction the docs call the whole + # point. Without this block a consumer would have to parse English with the ref interpolated + # into it, and nothing here would notice if Reason collapsed to one value. + got_reasons = reasons(payload) + assert got_reasons["landed.txt"] == "identical" + assert got_reasons["genuinely_new.txt"] == "absent" + assert len(set(got_reasons.values())) == 2, f"the reasons collapsed to one value: {got_reasons}" + # base.txt is TRACKED and modified, so it is not untracked and not this script's subject. assert "base.txt" not in got, ( "a tracked-but-modified file is not what the archive warning is about; including it would "