From 3c62df3c24bc9969da4fa02adc45d358bad2017d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 27 Aug 2026 18:47:57 -0500 Subject: [PATCH] fix(coord): claim.ps1 warned about the release rule at claim time and went silent at release time (BACKLOG #1358) `claim.ps1` anchors `$repo` on `$PSScriptRoot`, so a release is recorded against the tree the SCRIPT COPY lives in and ownership is judged there too. That anchoring is correct and deliberate (BACKLOG #1060): only the tool can be invoked from somewhere else, and the commit hook that reads cwd is right to, because for a hook cwd IS the committing worktree. THE DEFECT IS NOT THE ANCHORING. IT IS THAT THE WARNING ABOUT IT WAS UNREACHABLE FROM `-Release`. The divergence note existed and named the exact rule -- "-Release must be run against that same worktree" -- but it was written inline at the END of the script, after the `-Take` success block. `-Release` exits some 170 lines earlier. So the one sentence telling an operator that a release belongs to a particular tree was shown to them when they CLAIMED and withheld when they RELEASED. The failure is silent in the worst direction: the release SUCCEEDS, and `released_by` records a tree the operator was never standing in. Extracted the note into `Write-DivergenceNote` -- one divergence test, three call sites. Only the trailing sentence varies, because the true statement differs per path: a take is recorded to `$repo`, whereas a release is both recorded to it AND adjudicated against it. It reads `$repo` from script scope at call time rather than taking it as a parameter, since a second copy of "which tree is this" is precisely the drift the note exists to report. THE THIRD CALL SITE EARNS THE MOST AND WAS NOT IN THE ITEM. The refusal path prints "it is held by another worktree". Under divergence the ownership test ran against the SCRIPT's tree, so "another worktree" can be the operator's OWN, with the foreign thing being the copy of the script they invoked. Unwarned, that reads as a real cross-session collision and invites a `-Force` -- the one action that whole block exists to talk them out of. Verified, with the scope stated. `ruff check` + `ruff format --check` clean on the changed test; `mypy` strict clean over 267 engine sources; 96 tests pass across every suite that drives claim.ps1 (test_script_root_anchoring, test_coord_claim_{release_history,liveness,reconcile, refresh,adjudicate}, test_claim_check), run with the lane venv whose interpreter resolves into this checkout. RED-FIRST, AND THE POSITIVE CONTROL IS PART OF THE CHANGE RATHER THAN A NOTE ABOUT IT. Asserting only that `-Release` prints nothing cannot separate "this path is silent" from "the note never fires anywhere" -- an absent feature and a broken assertion render the same empty string. So the new pair pins BOTH: that the note fires on `-Take` from a foreign cwd, and that it now also fires on `-Release` from one. Before the fix the control passed and the sibling failed. Three mutants, all killed, each with a DISTINCT red set, and each scored only after a sha256 check that it actually changed the file: dropping the `-Release` note reds the sibling while the control still passes; dropping the `-Take` note reds the control while the sibling still passes; inverting the divergence test reds both. The first two failing on OPPOSITE tests is what shows these are two cases rather than one wearing two names. No ledger row added: #1358 already exists on origin/main at docs/BACKLOG.md, and its banner is not mine to flip. Co-Authored-By: Claude Opus 5 --- scripts/coord/claim.ps1 | 38 ++++++++++---- tests/test_script_root_anchoring.py | 77 +++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/scripts/coord/claim.ps1 b/scripts/coord/claim.ps1 index df1294292..5fca3b631 100644 --- a/scripts/coord/claim.ps1 +++ b/scripts/coord/claim.ps1 @@ -75,6 +75,25 @@ if (-not $repo) { throw "scripts/coord/ is not inside a git repository: $PSScrip # because functions sourced inside a function do not survive it. try { . "$PSScriptRoot/occupancy.ps1" } catch { } $repo = $repo.Trim() + +# ONE divergence test, three call sites (BACKLOG #1358). The note used to be written inline at the very +# end of the script, which put it after the `-Take` success block and therefore made it UNREACHABLE from +# `-Release` -- the script stated the release rule at claim time and went silent at the moment the +# operator applied it. `$Subject` is the only part that varies, because the true sentence differs: a take +# is recorded to $repo, whereas a release is BOTH recorded to it and adjudicated against it. +# +# Deliberately reads $repo at CALL time from script scope rather than taking it as a parameter: a second +# copy of "which tree is this" is exactly the drift this note exists to report. +function Write-DivergenceNote([Parameter(Mandatory)][string]$Subject) { + $cwdTop = (& git rev-parse --path-format=absolute --show-toplevel 2>$null) + if (-not $cwdTop) { return } + $a = ($cwdTop.Trim() -replace '\\', '/').TrimEnd('/') + $b = ($repo -replace '\\', '/').TrimEnd('/') + if ($a -ieq $b) { return } + Write-Host " NOTE: your shell is in $a, but this script lives in $b," -ForegroundColor Yellow + Write-Host " so the $Subject" -ForegroundColor Yellow +} + $common = (& git -C $repo rev-parse --path-format=absolute --git-common-dir).Trim() $claims = Join-Path $common "mefor-coord/claims" New-Item -ItemType Directory -Force -Path $claims | Out-Null @@ -349,6 +368,12 @@ if ($Release) { Write-Host " Confirm with that session before using -Force." } } + # BACKLOG #1358, and this is the placement that earns the most. The refusal above says the claim + # is "held by another worktree" -- but under divergence the ownership test ran against the SCRIPT's + # tree, so "another worktree" can be the operator's OWN, with the foreign thing being the copy of + # this script they invoked. Without the note that reads as a genuine cross-session collision and + # invites a -Force, which is the one action the whole block exists to talk them out of. + Write-DivergenceNote "ownership was judged against it, NOT against your shell's tree -- re-run this from $repo before concluding anyone else holds it." exit 1 } # RECORD FIRST, then act. Both orders can lie once and only one lie is recoverable: removing first @@ -403,6 +428,9 @@ if ($Release) { Write-Host " TOOK OVER a claim held by $($info.Claim.worktree) [$($info.Claim.branch)]." -ForegroundColor Yellow } Write-Host " recorded in $history" + # BACKLOG #1358. The release SUCCEEDS either way, so without this the history records a tree the + # operator was never standing in and nothing anywhere says so. + Write-DivergenceNote "release was recorded there, and ownership was judged against it rather than against your shell's tree." exit 0 } @@ -571,13 +599,5 @@ Write-Host " release when done: pwsh -NoProfile -File scripts\coord\claim.ps1 # Same note alloc.ps1 prints, for the same reason (BACKLOG #1060): anchoring is correct but surprising, # and a claim recorded to a worktree the caller is not standing in otherwise surfaces only as a refused # commit later. Silent on the ordinary same-tree invocation. -$cwdTop = (& git rev-parse --path-format=absolute --show-toplevel 2>$null) -if ($cwdTop) { - $a = ($cwdTop.Trim() -replace '\\', '/').TrimEnd('/') - $b = ($repo -replace '\\', '/').TrimEnd('/') - if ($a -ine $b) { - Write-Host " NOTE: your shell is in $a, but this script lives in $b, so the claim is" -ForegroundColor Yellow - Write-Host " recorded to $b. -Release must be run against that same worktree." -ForegroundColor Yellow - } -} +Write-DivergenceNote 'claim is recorded there. -Release must be run against that same worktree.' exit 0 diff --git a/tests/test_script_root_anchoring.py b/tests/test_script_root_anchoring.py index 2714680aa..3875c6c38 100644 --- a/tests/test_script_root_anchoring.py +++ b/tests/test_script_root_anchoring.py @@ -217,6 +217,83 @@ def test_claim_records_the_checkout_it_lives_in_not_the_cwd(tmp_path: Path) -> N ) +def _run_claim(script_tree: Path, cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(script_tree / "scripts" / "coord" / "claim.ps1"), + *args, + ], + cwd=str(cwd), + capture_output=True, + text=True, + timeout=180, + ) + + +#: The divergence note's own load-bearing sentence, asserted as a SUBSTRING of the rendered line. +#: +#: Not the whole line: it interpolates two absolute paths and PowerShell hard-wraps host output at the +#: console width, so an equality assertion would fail on formatting rather than on behaviour. This +#: fragment carries the instruction a reader has to act on and nothing that varies. +_DIVERGENCE = "but this script lives in" + + +def test_the_divergence_note_FIRES_on_take_from_a_foreign_cwd(tmp_path: Path) -> None: + """POSITIVE CONTROL for the test below, and it must be read as one. + + Asserting only that ``-Release`` prints nothing cannot distinguish "this path is silent" from "the + note never fires anywhere" -- an absent feature and a broken assertion print the same empty string. + So this pins that the mechanism WORKS on the path that has it, in the same fixture, with the same + divergence, before its sibling asserts the path that does not. + """ + named = _coord_checkout(tmp_path / "Named", drafted=4242, boundary=1200) + caller = _coord_checkout(tmp_path / "Caller", drafted=7777, boundary=1900) + + proc = _run_claim(named, caller, "-Take", "note-probe", "-Note", "divergence fixture") + + assert proc.returncode == 0, proc.stderr or proc.stdout + assert _DIVERGENCE in proc.stdout, ( + "the -Take divergence note did not fire, so this control cannot support its sibling:\n" + f"{proc.stdout}" + ) + + +def test_the_divergence_note_ALSO_fires_on_release_from_a_foreign_cwd(tmp_path: Path) -> None: + """BACKLOG #1358 -- the note names the release rule and was printed only at CLAIM time. + + ``-Release`` resolves ownership and records ``released_by`` from ``$repo``, which is anchored on + ``$PSScriptRoot`` and NOT on the caller's cwd. That is correct and deliberate; the defect is that the + warning about it was unreachable from this path. The note lives at the end of the script, after the + ``-Take`` success block, and ``-Release`` exits some 170 lines earlier -- so the one message that + states *"-Release must be run against that same worktree"* was shown to the operator at claim time + and withheld at the moment they actually did it. + + Its absence is silent in the worst way: the release SUCCEEDS, and the history records a tree the + operator was never standing in. + """ + named = _coord_checkout(tmp_path / "Named", drafted=4242, boundary=1200) + caller = _coord_checkout(tmp_path / "Caller", drafted=7777, boundary=1900) + + # Claim from INSIDE the named tree, so the take is the ordinary same-tree case and this test turns + # only on the release. A divergent take would print the note here and confuse which call emitted it. + taken = _run_claim(named, named, "-Take", "release-note-probe", "-Note", "fixture") + assert taken.returncode == 0, taken.stderr or taken.stdout + assert _DIVERGENCE not in taken.stdout, "the same-tree take must stay silent" + + released = _run_claim(named, caller, "-Release", "release-note-probe") + + assert released.returncode == 0, released.stderr or released.stdout + assert "Released claim" in released.stdout, released.stdout + assert _DIVERGENCE in released.stdout, ( + "-Release recorded the claim against the SCRIPT's tree and said nothing about it:\n" + f"{released.stdout}" + ) + + def test_setup_leak_gate_does_not_reintroduce_an_unanchored_toplevel(tmp_path: Path) -> None: """A spelling guard for the regression, paired with the behavioural test above.