From f403eea2377be0c057f3e96258ed3c95814aec01 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 16:21:27 -0500 Subject: [PATCH 1/3] fix(gate): rule 3d named a remedy that cannot reach the worktree it refused (BACKLOG #1057) The refusal was right and the sentence after it was not. A caller 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 /-. new.ps1:79-81 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; anything else fails its Test-Path and throws "No such worktree". * `prune-merged.ps1` excludes anything with a `.claude/worktrees/` path segment OUTRIGHT, by its own header, and -Name cannot reach them either. The 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. Verified against the INSTALLED gate, not only the repo copy. Same defect as #1032 one rule over, where 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. The deny now branches on worktree family. The family neither script can serve gets a literal `git -C "" worktree remove ""` plus a line saying why prune-merged.ps1 does not apply. The sibling family KEEPS prune-merged.ps1 -- it is dry-run by default and consults occupancy, so it is strictly better there than a bare git command, and the fix must not become "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: the right tool, named correctly, and still not pasteable. The gate has already resolved the path by then, so it now names the directory. REMEDY TEXT 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. Failure direction is pinned by test rather than asserted: a junction or UNC spelling breaks the prefix match, classifies NOT-sibling, and the not-sibling remedy is valid for every family including siblings. The dangerous direction needs a non-sibling to SPURIOUSLY match -, which an unresolved alias makes less likely, not more. #1064's measured rule 3d fail-open is deliberately not touched here. TWO ASSERTIONS IN THE NEW TESTS EXIST BECAUSE THE FIRST VERSIONS MEASURED THE WRONG THING: * every deny opens by quoting the command back, so searching the WHOLE reason for a runnable `git ... worktree remove` matched the refusal's own echo, and the own-tree case for the Claude-managed family went GREEN over a deny that offered nothing runnable. Assertions are now scoped to the text after "What to do instead:"; * the remedy deliberately says prune-merged.ps1 CANNOT help here, so a bare token search fails on the prohibition itself. 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. Negative control run, not assumed: reverted, 4 of the 6 cases go red and the 2 that stay green are the controls. Full gate suite 247 passed. Rule 3d is byte-identical between main and claude/worktree-gate-backlog-061bdc's tip, checked before writing, so this cannot revert or duplicate that session's in-flight work. This closes the REMEDY half of #1057. The occupancy half is untouched and the item stays open: rule 3d still has no occupancy or authorship signal, so an unoccupied worktree is still refused rather than allowed. --- docs/BACKLOG.md | 2 +- scripts/hooks/worktree_gate.ps1 | 71 +++++- tests/test_worktree_gate_remedy_families.py | 234 ++++++++++++++++++++ 3 files changed, 300 insertions(+), 7 deletions(-) create mode 100644 tests/test_worktree_gate_remedy_families.py diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 8f5a5835..4097b6d8 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5466,7 +5466,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. diff --git a/scripts/hooks/worktree_gate.ps1 b/scripts/hooks/worktree_gate.ps1 index fda62b3d..f5efb14d 100644 --- a/scripts/hooks/worktree_gate.ps1 +++ b/scripts/hooks/worktree_gate.ps1 @@ -640,10 +640,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. @@ -659,8 +720,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. "@ } @@ -675,9 +736,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..85a15115 --- /dev/null +++ b/tests/test_worktree_gate_remedy_families.py @@ -0,0 +1,234 @@ +# 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 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 +def repo(tmp_path: Path) -> 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. + """ + + 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. + """ + link = tmp_path / "viajunction" + made = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(link), str(repo.sibling)], capture_output=True, text=True + ) + 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}" + ) From 360653281d3ea66c534e32560ec2c9a1c9ccfcd3 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 19:34:42 -0500 Subject: [PATCH 2/3] test(gate): the junction case ERRORED on Linux instead of skipping (BACKLOG #1057) `test (ubuntu-latest, py3.14)` failed on PR #261. The junction test shells out to `cmd /c mklink /J` and guarded with `if made.returncode != 0: pytest.skip(...)`. There is no `cmd` on Linux, so subprocess.run RAISES FileNotFoundError before there is a returncode to inspect -- the guard never runs and the test errors. PLATFORM-MASKED, AND WINDOWS IS THE PLATFORM THAT CANNOT SEE IT: the guard is only reachable where `cmd` exists, so a green local quartet on Windows was structurally incapable of catching this. Same shape rule 3d's own comment already records, where a construct "passed on Windows and failed on the Linux CI leg". Gate on the platform FIRST, and keep the OSError arm: `cmd` existing does not mean junction creation is permitted, so the two guards catch different things and both are load-bearing. VERIFIED BY REPRODUCING THE LINUX CONDITION, not by re-running on Windows: cmd absent, unfixed -> FileNotFoundError, test ERRORS (the CI failure, reproduced) cmd absent, fixed -> SKIPPED via the OSError arm os.name=posix, fixed -> SKIPPED via the platform gate Windows, real cmd -> 6 passed, unchanged The first probe written for this was WRONG and is worth recording: patching `os.name` alone does not reproduce Linux, because `cmd` still exists here -- the unfixed test happily created a junction and PASSED. A control that cannot see the defect is not a control. Making subprocess.run raise for `cmd` is the faithful reproduction, and it discriminates. That also settles the OSError arm empirically rather than by argument: with `cmd` absent and os.name still "nt", the platform gate does NOT fire and the OSError arm is what prevents the error. Full gate suite 247 passed. Patch authored by the coordinator under route (c); the claim gate correctly refused their commit because another worktree held #1057, so it is committed here rather than forced past. It then refused THIS worktree too, for the same reason and equally correctly -- entitlement is keyed on the committing worktree, and building the fix in a fresh worktree moved me out of my own claim. Salvaged the documented way: released my own claim without -Force, re-took it here. --- tests/test_worktree_gate_remedy_families.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/test_worktree_gate_remedy_families.py b/tests/test_worktree_gate_remedy_families.py index 85a15115..6c5836d0 100644 --- a/tests/test_worktree_gate_remedy_families.py +++ b/tests/test_worktree_gate_remedy_families.py @@ -35,6 +35,7 @@ from __future__ import annotations +import os import re import shutil import subprocess @@ -214,10 +215,24 @@ def test_a_junction_spelling_of_a_sibling_fails_toward_the_universal_remedy( 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" - made = subprocess.run( - ["cmd", "/c", "mklink", "/J", str(link), str(repo.sibling)], capture_output=True, text=True - ) + 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}") From 41a724bbde36ae2a1360eefe07a6fe8ec1611aa7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 7 Aug 2026 00:49:58 -0500 Subject: [PATCH 3/3] perf(test): module-scope the remedy-families fixture; file #1084 (BACKLOG #1057) `test (windows-2025, py3.14)` timed out on PR #261: "The action 'Tests (pytest)' has timed out after 36 minutes". Not a flake and not a defect in the gate change -- the leg has almost no headroom and this file's 249 lines consumed what was left. #261 05:00:57 -> 05:38:03 = 37m06s step timed out at 36m #260 04:59:22 -> 05:32:31 = 33m09s success #259 22:00:10 -> 22:38:30 = 38m20s success step_timeout is 36 for windows-2025, read from ci.yml's matrix rather than inferred from the error text. A suite habitually finishing near 33 against a 36 cap has ~17% headroom. THE TELL THAT IT IS ELAPSED TIME AND NOT CONTENT: the timeout named tests/test_tls_floor_probe.py, while #261 touches worktree_gate.ps1, its tests and one BACKLOG line -- nothing TLS-related. Location and change do not line up. Also ruled out: environmental (the same leg is SUCCESS on 13 other open PRs), and the exit-139 native-crash class of #264 (this is a clean timeout, not a crash). THE FIX HERE IS TO MAKE THIS FILE CHEAPER, NOT TO RAISE THE CAP. step_timeout is deliberately held under job_timeout (46) so a process-level deadlock below pytest surfaces as a STEP failure rather than a job kill; ci.yml says so explicitly. Raising it would trade a real diagnostic for a green tick. The `repo` fixture is subprocess-bound (six `git` spawns) and READ-ONLY -- every test feeds a payload to the hook, which DENIES before git runs, so no worktree is ever actually removed against it. Function scope paid for it six times: 36 process launches, cheap on Linux and expensive on a hosted Windows runner. Module-scoped it removes 30 of those. Measured locally: 10.66s -> 5.26s, a 51% cut, with setup no longer appearing in the top durations at all. The docstring records WHY sharing is safe rather than asserting that it is, and says what would make it unsafe: a case that really removes a worktree needs its own function-scoped fixture. Files #1084 for the underlying condition, which outlives this file. The margin is invisible until it is gone -- nothing reports "the Windows leg finished at 92% of its budget" -- and the measured spread across three PRs the same day (33m09s / 37m06s / 38m20s) means runner variance alone is comparable to the headroom, so a re-run can flip the verdict with nothing changed. That is also why #261 must not be merged on a lucky green. Diagnosis by the coordinator, who ruled out three alternative explanations against evidence before accepting the elapsed-time one, and who recorded a prediction before re-running rather than after. --- docs/BACKLOG.md | 27 +++++++++++++++++++++ tests/test_worktree_gate_remedy_families.py | 16 ++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index a85e831e..ae4df660 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5740,3 +5740,30 @@ environments.py:79 `if not base_dir: return cwd` <-- and base_dir i **Related:** #1063 (the same script, anchoring rather than reporting), #1000 (a gate whose green does not say what it was green about). **Source:** observed 2026-08-06 while arming a fresh worktree during #1063's fix. Held unfiled as marginal, and filed on the owner's instruction. + +## 1084. The Windows test leg runs at ~92% of its `step_timeout`, so any test addition tips it into a timeout that reads as a flake + +> ๐Ÿ”ข **Filed 2026-08-07 โ€” not started.** Value **7/10** ยท Difficulty **4/10** ยท _do it_. `windows-2025` and `windows-2022` carry `step_timeout: 36` (`ci.yml`, matrix `W22`/`W25`) against a `Tests (pytest)` step that habitually finishes near **33 minutes** โ€” about **17% headroom**, and less than that on a slow runner. Measured 2026-08-07: adding one 249-line subprocess-bound test file took the leg from 33m09s to **37m06s** and it died with *"The action 'Tests (pytest)' has timed out after 36 minutes"*. The failure names no test and points at whatever file was executing when the clock ran out, so it reads as an unrelated flake in a random module. + +**Cluster:** CI capacity / diagnosability. **Priority:** P2. **Verdict:** build. **Severity:** no product effect. The cost is that a **blocking, required** check fails for a reason unconnected to the change, and the obvious responses are all wrong. + +**The three wrong answers this failure invites, and why each is wrong.** + +1. **"Flake โ€” re-run it."** This is how a systematic overrun gets logged as a flake for months. This repo's two famous flakes were a livelock and a test that was right (#1014 and its neighbour), so the prior here is bad. +2. **"The change broke something."** The timeout names the file that happened to be running โ€” measured here as `tests/test_tls_floor_probe.py` on a PR that touched only `worktree_gate.ps1`, its tests, and one BACKLOG line. **Location and change do not line up**, which is the tell that the elapsed clock, not the content, is the cause. +3. **"Raise `step_timeout`."** The reflex, and it trades away a real diagnostic. `step_timeout` (36) is held deliberately under `job_timeout` (46) so a process-level deadlock **below** pytest surfaces as a *step* failure rather than a job kill โ€” `ci.yml` says so in as many words. Raising it to buy a green tick removes the signal the gap exists to produce. + +**What distinguishes this from an ordinary slow suite: the margin is invisible until it is gone.** Nothing reports "the Windows leg finished at 92% of its budget". A PR that adds two minutes is indistinguishable, at review time, from one that adds none โ€” and the one that tips it over is blamed. The measured spread across three PRs on the same day was 33m09s / 37m06s / 38m20s, so **runner variance alone is comparable to the headroom**, which also means a re-run can flip the verdict without anything changing. + +**Fix directions, offered not decided.** + +- **Report the margin.** Emit the step's elapsed time and its percentage of `step_timeout` in the job summary. Cheapest by far, changes no behaviour, and turns an invisible cliff into a number that trends. Probably do this regardless of what else is chosen. +- **Split the Windows leg.** Two steps under one job, each with its own budget. Keeps the deadlock diagnostic and roughly halves the exposure; costs matrix complexity. +- **Make the subprocess-bound gate tests cheaper.** They dominate: `tests/test_worktree_gate*.py` spawn `pwsh` and `git` per case, which is seconds each on a hosted Windows runner and milliseconds on Linux. Module-scoping one read-only fixture in `test_worktree_gate_remedy_families.py` cut that file 51% locally (10.66s โ†’ 5.26s) by removing 30 of its 36 `git` spawns; the same shape is available in its siblings. This is the highest-yield option and the least structural. +- **Do NOT** simply raise the cap, per (3) above. + +**A negative control per #1000.** Whatever is built, it must be shown to fail on a suite that genuinely overruns โ€” a margin report that never goes red, or a split that silently swallows a hang, would be the same class of green-that-cannot-see this cluster keeps producing. + +**Related:** #1057 (the change that surfaced it โ€” its test file was the straw, not the cause), #1014 (a "flake" that was a real failure), #1000 (a control whose green is not evidence). + +**Source:** found 2026-08-07 by the coordinator, from a `test (windows-2025)` failure on PR #261. Diagnosed by comparing job wall time across three PRs rather than by reading the failing test, and by reading `step_timeout` out of `ci.yml` rather than inferring it from the error text. Three alternative explanations โ€” environmental, content-related, and the exit-139 native-crash class of #264 โ€” were each ruled out against evidence before the elapsed-time explanation was accepted. diff --git a/tests/test_worktree_gate_remedy_families.py b/tests/test_worktree_gate_remedy_families.py index 6c5836d0..963c8197 100644 --- a/tests/test_worktree_gate_remedy_families.py +++ b/tests/test_worktree_gate_remedy_families.py @@ -63,13 +63,25 @@ def shell(command: str, cwd: Path | str) -> dict[str, Any]: } -@pytest.fixture -def repo(tmp_path: Path) -> SimpleNamespace: +@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(