diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index ceee0981..f0a496af 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5689,7 +5689,7 @@ and `enforce_admins` governs **protected branches**. Re-enabling it would refuse ## 1057. Rule 3d has no occupancy signal, so it cannot tell an abandoned worktree from a live one -> ๐Ÿ”ข **Filed 2026-08-05 โ€” not started.** Value **4/10** ยท Difficulty **5/10** ยท _fill-in_. #1041 fixed rule 3d's *false* claim; this is the *missing* one underneath it. The rule refuses every governed worktree that is not the caller's own, because it has no way to ask whether anyone is actually using it. The refusal is right by default โ€” a needless refusal costs a message, a wrong allow deletes a live session's work โ€” but it is unactionable for a caller cleaning up a worktree it created itself, who must escalate to a human for something it was entitled to do. +> ๐Ÿšง **PARTLY SHIPPED 2026-08-06 โ€” the REMEDY slice is in; the occupancy body is NOT, and this item stays open for it.** Value **4/10** ยท Difficulty **5/10** ยท _fill-in_. What shipped is the half nobody had noticed: rule 3d refused correctly and then handed the caller a command that **throws**. `remove.ps1 -Name` resolves only to `/-` โ€” `new.ps1:79-81` *asserts* that shape, so the sibling family is the only one it can produce โ€” and `prune-merged.ps1` excludes anything with a `.claude/worktrees/` path segment **outright**, by its own header, with `-Name` unable to reach them either. Census on this clone: **45 sibling worktrees, 8 Claude-managed, 4 other โ€” and all six live sessions sat in the 8**, so the remedy failed for the population that actually reaches the rule. Verified against the **installed** gate, not only the repo copy. The deny now branches on worktree family and, for anything the two scripts cannot serve, prints a literal `git -C "" worktree remove ""` plus a line saying why `prune-merged.ps1` does not apply; the sibling family keeps `prune-merged.ps1`, which is dry-run by default and consults occupancy, so the fix is not "stop naming the scripts" โ€” a test pins that. Writing the test enlarged the defect: the own-tree branch printed the literal placeholder `-Name ` for **both** families, so it named the tool correctly and still could not be pasted; the gate has the resolved path and now uses it. **This is a remedy-text change only** โ€” which worktrees rule 3d refuses is untouched, and nothing security-relevant reads the classification, which is what makes a misclassification cheap here and expensive in rule 3c. **Same defect as #1032, one rule over** (there, rule 3b printed a `new.ps1` command `new.ps1` refuses to run). **Still open, and unchanged below:** rule 3d has no occupancy or authorship signal, so an *unoccupied* worktree is still refused rather than allowed โ€” the three structural options, the measured 0-of-24 occupancy result and the fail-closed constraint all stand as written. See also **#1064**, a measured rule 3d fail-open confirmed 2026-08-06, deliberately not touched here. Original filing follows. #1041 fixed rule 3d's *false* claim; this is the *missing* one underneath it. The rule refuses every governed worktree that is not the caller's own, because it has no way to ask whether anyone is actually using it. The refusal is right by default โ€” a needless refusal costs a message, a wrong allow deletes a live session's work โ€” but it is unactionable for a caller cleaning up a worktree it created itself, who must escalate to a human for something it was entitled to do. **Cluster:** Session-drift controls / refusal accuracy. **Priority:** P3. **Verdict:** build (medium). **Severity:** no data loss, and no security effect โ€” the rule fails *closed*. The cost is a correct refusal the reader cannot act on, and the standing invitation to route around a guard that says no to legitimate work. @@ -6553,12 +6553,22 @@ Killed at the cap (`Tests (pytest)` step conclusion `failure`, all at 36:0x): **Scope note โ€” the two failures on this leg today are DIFFERENT and must not be merged into one cause.** PR #253 is a genuine timeout (no test failed) and PR #256 (`fix-1013`) is an assertion failure in `tests/test_connscale_cpu_probe.py` and `tests/test_connscale_smoke.py`, whose own comment already records this leg failing twice in one job on 2026-07-30. Only the first belongs to this item. -## THIS ITEM SUPERSEDES #1084, AND THE TWO MUST NOT BOTH LAND UNRECONCILED +## THIS ITEM SUPERSEDES #1084, AND #1084 WAS DROPPED RATHER THAN LANDED -> โš ๏ธ **Forward-looking citation, flagged as such.** At the time of writing, **#1084 is not on `main`** โ€” it -> exists only on `origin/claude/gate-3d-remedy-1057` (**PR #261**, open). So this cross-reference does not -> resolve yet and will only do so once #261 lands. It is stated anyway because the collision is invisible -> from any worktree reading `docs/BACKLOG.md`, which is exactly how it was nearly missed. +> **SETTLED 2026-08-08 โ€” and the outcome is the opposite of what this block used to predict.** It +> previously read *"#1084 is not on `main`; it exists only on `origin/claude/gate-3d-remedy-1057` +> (PR #261, open), so this cross-reference will only resolve once #261 lands."* **#261 landed WITHOUT +> #1084.** The item was dropped during #261's merge resolution, on the owner's instruction, on exactly +> the grounds set out below. **#1084 never reached `main` and no longer will**, so a reader grepping for +> it will not find it โ€” that absence is the resolution, not a gap. The number stays allocated and burned +> rather than reused. +> +> This item is therefore the survivor, and per the paragraph below the reconciliation is written into it +> here rather than left for a reader to infer. **Nothing further is owed to #1084.** +> +> *This block is retained rather than deleted because the collision was invisible from any worktree +> reading `docs/BACKLOG.md` โ€” which is exactly how it was nearly missed โ€” and whoever finds it by +> grepping `1084` deserves the outcome and not only the diagnosis.* **#1084 is the same defect, found the same day, on the same leg and the same cap.** This item's kill table literally contains #1084's originating run (PR #261, `31149117314`). It is **not** a duplicate in the diff --git a/scripts/hooks/worktree_gate.ps1 b/scripts/hooks/worktree_gate.ps1 index 1fc96789..dde433eb 100644 --- a/scripts/hooks/worktree_gate.ps1 +++ b/scripts/hooks/worktree_gate.ps1 @@ -722,10 +722,71 @@ What to do instead: # leg, where the lookup returned nothing, $isSelf went false, and BOTH branches emitted the # generic deny -- byte-identical, which is exactly what the non-vacuity test below asserts # against. Platform-masked, and caught only because that test compares the two denies. - $victimTop = Get-ComparablePath "$(& git -C $victimRaw rev-parse --show-toplevel 2>$null)".Trim() + $victimTopRaw = "$(& git -C $victimRaw rev-parse --show-toplevel 2>$null)".Trim() + if (-not $victimTopRaw) { $victimTopRaw = $victimRaw } + $victimTop = Get-ComparablePath $victimTopRaw $selfTop = Get-ComparablePath "$(& git -C $cwdRaw rev-parse --show-toplevel 2>$null)".Trim() $isSelf = $victimTop -and $selfTop -and ($victimTop -eq $selfTop) + # WHICH FAMILY IS THE VICTIM IN? The remedy has to be one that can actually reach it, and until + # now neither of the two this rule named could (BACKLOG #1057): + # + # * `remove.ps1 -Name ` resolves to /-. new.ps1 ASSERTS that + # shape after deriving it, so the - sibling family is the only one it can + # produce and the only one remove.ps1 can resolve; handed anything else it fails Test-Path + # and throws "No such worktree". + # * `prune-merged.ps1` excludes anything with a `.claude/worktrees/` path segment OUTRIGHT -- + # its own header says so, and -Name cannot reach them either. That exclusion is deliberate: + # those are the trees a live session gets relocated into. + # + # Census on this clone 2026-08-06: 45 sibling worktrees, 8 Claude-managed, 4 other -- and every + # live session sat in the 8. So the refused caller was reliably handed a command that throws and + # a tool that reports nothing about their tree. Same defect as #1032, one rule over: there, rule + # 3b printed a new.ps1 command new.ps1 refuses to run. A refusal the reader cannot act on is the + # standing invitation to route around the guard, which costs more than the refusal buys. + # + # THIS BRANCHES THE REMEDY STRING ONLY. Which worktrees rule 3d refuses is untouched, and no + # security decision reads $isSibling -- which is what makes a misclassification cheap here and + # not in rule 3c. FAILURE DIRECTION, pinned by test rather than asserted: a junction or UNC + # spelling breaks the prefix match, classifies NOT-sibling, and the not-sibling remedy is a + # literal `git worktree remove ` that is valid for EVERY family, siblings included. + # The dangerous direction needs a non-sibling to SPURIOUSLY match `-`, which an + # unresolved alias makes less likely rather than more. + $govLeaf = Split-Path $govWt.Display -Leaf + $sibPrefix = "$($govWt.Compare)-" + $isSibling = $victimTop -and $victimTop.StartsWith($sibPrefix) -and + -not $victimTop.Substring($sibPrefix.Length).Contains('/') + # NAME the directory rather than printing ``. The gate has just resolved the + # path; leaving the caller to substitute a placeholder into a command is a second chance to get + # it wrong, and it is the reason the own-tree branch was unrunnable for the sibling family too. + $sibName = if ($isSibling) { (Split-Path $victimTopRaw -Leaf).Substring($govLeaf.Length + 1) } else { $null } + $removeCmd = if ($isSibling) { + "pwsh -NoProfile -File $($govWt.Display)\scripts\worktree\remove.ps1 -Name $sibName" + } + else { + "git -C `"$($govWt.Display)`" worktree remove `"$victimTopRaw`"" + } + # The sibling family KEEPS prune-merged.ps1 and that is not politeness: it is dry-run by default, + # it consults occupancy, and it re-reads its fence immediately before each removal. For the family + # it covers it is strictly better than a bare `git worktree remove`, so the fix must not become + # "stop naming the scripts" -- a test pins that it is still offered here. + $cleanupBullet = if ($isSibling) { + @" + * Cleaning up merged worktrees is a maintenance job with its own dry-run-by-default tool. Run it and + READ what it proposes before applying anything: + pwsh -NoProfile -File $($govWt.Display)\scripts\worktree\prune-merged.ps1 +"@ + } + else { + @" + * prune-merged.ps1 CANNOT help with this one, so do not reach for it: it skips anything under + .claude/worktrees and anything that is not a - sibling, by design, and its -Name cannot + reach them either. If this tree really must go, that is the user's call and this is the command -- + it is not yours to run: + $removeCmd +"@ + } + if ($isSelf) { Write-Deny -Rule "3d" -Detail "git worktree $wtVerb (own worktree)" -Reason @" BLOCKED: 'git worktree $wtVerb $victimRaw' acts on THE WORKTREE THIS SESSION IS RUNNING IN. @@ -741,8 +802,8 @@ What to do instead: * Finish and COMMIT anything you still want. A commit survives the tree being deleted; a dirty tree does not. * Then ask the user, in these words: "I am finished in $victimRaw and it can be removed once this - session ends." Removal is theirs to run from the main checkout: - pwsh -NoProfile -File $($govWt.Display)\scripts\worktree\remove.ps1 -Name + session ends." Removal is theirs to run from OUTSIDE this tree: + $removeCmd * If you only wanted to leave it, just stop using it -- an unused worktree costs disk, not correctness. "@ } @@ -757,9 +818,7 @@ uncommitted work in them. There is no undo, and the session using it finds out w fails. That asymmetry is why the default is refusal even though the tree may well be abandoned. What to do instead: - * Cleaning up merged worktrees is a maintenance job with its own dry-run-by-default tool. Run it and - READ what it proposes before applying anything: - pwsh -NoProfile -File $($govWt.Display)\scripts\worktree\prune-merged.ps1 +$cleanupBullet * To find out whether a worktree is still in use, look rather than delete: git -C "$($govWt.Display)" worktree list * If you are certain it is abandoned and must go now, that is the user's call, not yours. Say so: diff --git a/tests/test_worktree_gate_remedy_families.py b/tests/test_worktree_gate_remedy_families.py new file mode 100644 index 00000000..963c8197 --- /dev/null +++ b/tests/test_worktree_gate_remedy_families.py @@ -0,0 +1,261 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Rule 3d's deny must name a remedy that can actually reach the worktree it refused (BACKLOG #1057). + +The refusal itself is not in question here and none of these tests touch it. What is in question is the +sentence after it: a caller who is told "no" is handed a command to run instead, and for the population +that most often reaches this rule that command *throws*. + +* ``remove.ps1 -Name `` resolves to ``/-`` and refuses anything else. + ``new.ps1`` ASSERTS that shape after deriving it, so the ``-`` sibling family is the only + one it can produce and the only one ``remove.ps1`` can resolve. +* ``prune-merged.ps1`` excludes anything with a ``.claude/worktrees/`` path segment OUTRIGHT -- its own + header says so, and ``-Name`` cannot reach them either. That exclusion is deliberate: those are the + trees a live session gets relocated into. + +Census on this clone 2026-08-06: 45 sibling worktrees, 8 Claude-managed, 4 other -- and all six live +sessions sat in the 8. So the remedy failed for the population that actually hits the rule. + +**Same defect as BACKLOG #1032, one rule over.** There, rule 3b printed a ``new.ps1`` command that +``new.ps1`` refuses to run. A refusal the reader cannot act on is the standing invitation to route around +the guard, which costs more than the refusal it was trying to buy. + +**The headline test asserts the remedy is RUNNABLE, not that a particular string is absent.** Checking +for the absence of ``prune-merged.ps1`` would pass the moment someone swapped in a different unusable +command. Extracting the ``-Name`` argument the deny actually printed and asserting the path it resolves +to exists is the assertion that would have caught this without knowing the fix. + +**Harness note, so nobody re-derives it.** ``run_gate`` calls ``subprocess.run`` with no ``cwd=``, so the +hook process cwd is pytest's while the payload cwd is a tmp_path. Any rule resolving a RELATIVE path +against the process cwd behaves differently here than in production -- a red test written that way pins +the harness, which happened in this repo and was retracted after four verifiers refuted it. These tests +are immune: every path in them is absolute, and the classification under test reads the victim's absolute +``--show-toplevel``. +""" + +from __future__ import annotations + +import os +import re +import shutil +import subprocess +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import pytest + +from tests.test_worktree_gate import assert_denied, run_gate # reuse the subprocess harness + +pytestmark = pytest.mark.skipif( + shutil.which("pwsh") is None or shutil.which("git") is None, + reason="needs pwsh (PowerShell 7) and git on PATH", +) + + +def shell(command: str, cwd: Path | str) -> dict[str, Any]: + return { + "session_id": "s-1", + "cwd": str(cwd), + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_input": {"command": command}, + } + + +@pytest.fixture(scope="module") +def repo(tmp_path_factory: pytest.TempPathFactory) -> SimpleNamespace: + """A governed primary with one worktree of EACH family. + + ``sibling`` is what new.ps1 produces and what remove.ps1 can resolve. ``managed`` is where the + Claude Code harness puts a session, and is the family both remedy tools refuse. + + MODULE-SCOPED BECAUSE IT IS SUBPROCESS-BOUND AND READ-ONLY. Building it costs six `git` spawns, + and function scope paid that six times over -- 36 process launches, which is cheap on Linux and + expensive on a hosted Windows runner. Nothing here mutates it: every test feeds a payload to the + hook, which DENIES before git ever runs, so no `worktree remove` is ever executed against this + tree. Sharing it is safe for that specific reason, not by convention -- if a case is ever added + that really removes a worktree, it needs its own function-scoped fixture rather than this one. + + This is not a micro-optimisation. `test (windows-2025)` has a 36-minute `step_timeout` against a + suite that habitually finishes near 33, and the first version of this file pushed that leg to + 37m06s and timed it out. See BACKLOG #1084. + """ + tmp_path = tmp_path_factory.mktemp("remedy_families") + + def git(*args: str, cwd: Path | None = None) -> None: + subprocess.run( + ["git", *args], + cwd=str(cwd) if cwd else None, + check=True, + capture_output=True, + text=True, + ) + + primary = tmp_path / "Primary" + git("init", "-b", "main", str(primary)) + git("config", "user.email", "t@example.com", cwd=primary) + git("config", "user.name", "t", cwd=primary) + (primary / "seed.txt").write_text("seed\n", encoding="utf-8") + git("add", "-A", cwd=primary) + git("commit", "-m", "seed", cwd=primary) + + sibling = tmp_path / "Primary-wt" + git("worktree", "add", "-b", "sib-branch", str(sibling), cwd=primary) + + managed = primary / ".claude" / "worktrees" / "cm" + git("worktree", "add", "-b", "cm-branch", str(managed), cwd=primary) + + repos = tmp_path / "repos.txt" + repos.write_text(f"{primary}\n", encoding="utf-8") + return SimpleNamespace(primary=primary, sibling=sibling, managed=managed, repos=repos) + + +def _remedy(reason: str) -> str: + """Only the lines AFTER "What to do instead:". The echoed command at the top is not a remedy. + + THIS SCOPING IS LOAD-BEARING AND ITS ABSENCE ALREADY PRODUCED A FALSE GREEN. Every rule 3d deny + opens by quoting the command back: ``BLOCKED: 'git worktree remove ' acts on ...``. Searching + the WHOLE reason for a runnable ``git ... worktree remove`` therefore matches the refusal's own echo, + and the own-tree case for the Claude-managed family went green over a deny that offered nothing + runnable at all. Measured on the unfixed gate while writing these tests. + """ + _, sep, tail = reason.partition("What to do instead:") + return tail if sep else "" + + +def _remove_ps1_targets(remedy: str, primary: Path) -> list[Path]: + """Every path a `remove.ps1 -Name X` line would actually act on. + + ``\\S+`` rather than a name charset ON PURPOSE, so the placeholder the gate prints today + (``-Name ``) is CAUGHT and resolves to a path that does not exist. A remedy the + caller cannot paste and run is not a remedy; making the regex skip placeholders would define the + defect out of the test. + """ + parent, leaf = primary.parent, primary.name + return [parent / f"{leaf}-{m}" for m in re.findall(r"remove\.ps1\s+-Name\s+(\S+)", remedy)] + + +@pytest.mark.parametrize("standing_in", ["victim", "primary"]) +@pytest.mark.parametrize("family", ["sibling", "managed"]) +def test_the_remedy_rule_3d_prints_can_actually_reach_the_worktree( + repo: SimpleNamespace, family: str, standing_in: str +) -> None: + """THE HEADLINE. Both deny branches, both families: whatever command the deny names must work. + + Parametrised over where the session is standing because rule 3d has two branches -- "this IS your own + tree" and "this is not" -- and the defect was in BOTH. A test covering one branch would have gone + green over a remedy that still threw on the other. + """ + victim = getattr(repo, family) + cwd = victim if standing_in == "victim" else repo.primary + reason = assert_denied(run_gate(shell(f'git worktree remove "{victim}"', cwd=cwd), repo.repos)) + remedy = _remedy(reason) + assert remedy, "every deny must offer something; there is no 'What to do instead:' section" + + for target in _remove_ps1_targets(remedy, repo.primary): + assert target.exists(), ( + f"the deny offered `remove.ps1 -Name` resolving to {target}, which does not exist -- " + f"running it throws 'No such worktree'. Victim was {victim}" + ) + + if family == "managed": + # Neither script can serve this family: prune-merged.ps1 excludes the path segment outright + # (its own header says so) and remove.ps1 -Name cannot resolve a path outside -*. + # + # THE PROPERTY IS "NO RUNNABLE INVOCATION", NOT "THE NAME NEVER APPEARS" -- the same distinction + # test_coord_claim_liveness.py already draws for -Force, and for the same reason. The remedy + # deliberately says prune-merged.ps1 CANNOT help here, so a bare token search fails on the + # prohibition itself. An assertion that a warning about a tool is indistinguishable from an offer + # of it is measuring the wrong thing, and it would push the fix toward deleting the warning. + assert not re.search(r"pwsh[^\n]*prune-merged\.ps1", remedy), ( + "prune-merged.ps1 was offered as a command; it skips anything under .claude/worktrees" + ) + assert not re.search(r"pwsh[^\n]*remove\.ps1", remedy), ( + "remove.ps1 was offered as a command; -Name cannot resolve outside -" + ) + # ...so the remedy must contain a literal git command, and it must NAME THE VICTIM, which is + # what distinguishes an offered route from the refusal quoting itself back. + # + # Separators folded on both sides: `git rev-parse --show-toplevel` returns FORWARD slashes on + # Windows while pathlib renders backslashes, so a literal compare fails on a path that is + # correct. Folding is the honest compare here -- it is the same path, differently spelled. + assert re.search(r"worktree\s+remove", remedy), ( + "a family neither script can reach still needs a runnable route" + ) + assert str(victim).replace("\\", "/").casefold() in remedy.replace("\\", "/").casefold(), ( + f"the route must NAME the victim, not leave it to be substituted: {victim}\n{remedy}" + ) + + +def test_the_sibling_family_keeps_the_tooling_it_has(repo: SimpleNamespace) -> None: + """Non-vacuity for the test above. The fix must NOT become 'stop naming the scripts'. + + prune-merged.ps1 is dry-run-by-default and consults occupancy; for the family it does cover it is a + strictly better answer than a bare `git worktree remove`, and losing it would trade one unusable + remedy for another. + """ + reason = assert_denied( + run_gate(shell(f'git worktree remove "{repo.sibling}"', cwd=repo.primary), repo.repos) + ) + own = assert_denied( + run_gate(shell(f'git worktree remove "{repo.sibling}"', cwd=repo.sibling), repo.repos) + ) + assert own != reason, "the two branches must stay distinguishable" + assert "prune-merged.ps1" in _remedy(reason), ( + "the family prune-merged.ps1 DOES cover must keep being sent there -- it is dry-run by default " + "and consults occupancy, which a bare `git worktree remove` does not" + ) + targets = _remove_ps1_targets(_remedy(own), repo.primary) + assert targets, "the own-tree branch must still offer remove.ps1 for the family it can resolve" + assert targets == [repo.sibling], ( + f"remove.ps1 -Name must name the ACTUAL directory, not a placeholder: got {targets}" + ) + + +def test_a_junction_spelling_of_a_sibling_fails_toward_the_universal_remedy( + repo: SimpleNamespace, tmp_path: Path +) -> None: + """The classifier's failure DIRECTION, pinned rather than asserted in a comment. + + A junction or UNC spelling can break the `-` prefix match. That misclassifies a + sibling as 'other', and the 'other' remedy is a literal `git worktree remove ` -- valid for + every family, siblings included. So the failure costs a less idiomatic suggestion and nothing else. + + The dangerous direction is the opposite: a non-sibling classified AS sibling emits a `remove.ps1 + -Name` that throws. That needs a path to SPURIOUSLY match the prefix, which an unresolved junction + makes less likely rather than more. Unlike rule 3c, no security decision rides on this -- the refusal + is unchanged and only the remedy string branches. + """ + # `cmd` exists only on Windows, and a returncode guard cannot express that: on Linux + # subprocess.run RAISES FileNotFoundError before there is a returncode to inspect, so the test + # ERRORS instead of skipping. Gate on the platform first -- the same shape rule 3d's own comment + # records, where a construct "passed on Windows and failed on the Linux CI leg". OSError is still + # caught because `cmd` existing does not mean junction creation is permitted. + if os.name != "nt": + pytest.skip( + "junctions are a Windows filesystem feature; `cmd /c mklink` does not exist here" + ) + link = tmp_path / "viajunction" + try: + made = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(link), str(repo.sibling)], + capture_output=True, + text=True, + ) + except OSError as exc: # cmd present but unusable + pytest.skip(f"could not invoke mklink: {exc}") + if made.returncode != 0: + pytest.skip(f"could not create a junction: {made.stderr or made.stdout}") + + reason = assert_denied( + run_gate(shell(f'git worktree remove "{link}"', cwd=repo.primary), repo.repos) + ) + remedy = _remedy(reason) + for target in _remove_ps1_targets(remedy, repo.primary): + assert target.exists(), f"junction spelling produced an unrunnable remedy: {target}" + # Non-vacuity: the assertion above is satisfied by an empty list, so pin that SOMETHING runnable was + # offered. Either classification is acceptable here -- what must not happen is a remedy that throws. + assert re.search(r"worktree\s+remove|remove\.ps1|prune-merged\.ps1", remedy), ( + f"junction spelling produced a deny with no route at all:\n{remedy}" + )