diff --git a/scripts/coord/claim.ps1 b/scripts/coord/claim.ps1 index 4e6c28b4..df129429 100644 --- a/scripts/coord/claim.ps1 +++ b/scripts/coord/claim.ps1 @@ -68,6 +68,12 @@ $ErrorActionPreference = "Stop" # Hook right, tool wrong, and only the tool can be invoked from somewhere else. $repo = (& git -C $PSScriptRoot rev-parse --path-format=absolute --show-toplevel 2>$null) if (-not $repo) { throw "scripts/coord/ is not inside a git repository: $PSScriptRoot" } + +# Occupancy is what lets a holder that is a DIRECTORY be told apart from one that is a SESSION +# (BACKLOG #1348). Loaded best-effort and NEVER fatal: if this fails, Get-HolderLiveness keeps its +# older, stricter meaning and refuses, which is the safe direction. Dot-sourced at script scope +# because functions sourced inside a function do not survive it. +try { . "$PSScriptRoot/occupancy.ps1" } catch { } $repo = $repo.Trim() $common = (& git -C $repo rev-parse --path-format=absolute --git-common-dir).Trim() $claims = Join-Path $common "mefor-coord/claims" @@ -154,23 +160,82 @@ function Add-HistoryLine([string]$Line) { # Reports only what it can PROVE. A vanished directory is a fact and the one state safe to act on # unasked. Everything else is 'unknown' or a quiet-hours count -- never "probably fine": a session can # be alive and simply not committing, so silence is not evidence of death. +# A DIRECTORY IS NOT A PERSON, AND THIS FUNCTION USED TO CONFLATE THEM (BACKLOG #1348). +# +# `present` meant "the path exists". It never asked whether a SESSION was in it, so a worktree that +# outlived its session -- the directory still on disk, nobody in it -- rendered identically to a +# lane that is actively building. That is a THIRD state the tool could not represent, and it is the +# one that produces work that is done, correct, and held by nobody: the sanctioned verbs refuse, +# `-Force` is forbidden by CLAUDE.md, and the claim cannot be regularised by anything. +# +# ***THE NEW STATE REPORTS. IT DOES NOT PERMIT.*** `unoccupied` still REFUSES, exactly as `present` +# does, and this is not timidity -- `occupancy.ps1` states the rule it inherits: "there is no +# heartbeat on this host, so nothing here can prove a session is GONE. Occupancy may therefore only +# ever VETO an action; a DEAD/STALE/absent verdict must never by itself authorise one." What the +# item asked for is that the two be DISTINGUISHABLE, not that the second become releasable. +# +# POLARITY, and it is the same rule the blanket-stage guard carries (#1341, #1229): recognition may +# only ever SUPPRESS. Downgrading to `unoccupied` requires a POSITIVE determination -- the occupancy +# probe loaded, reported itself Available, and returned zero vetoing sessions for that exact path. +# Every other outcome, including the probe failing to load at all, stays `present`. A missing +# answer must cost a refusal, never a licence. +$script:OccupancyProbe = $null # $null = not yet tried; $false = unavailable; else the object + +function Get-OccupancyOnce { + if ($null -ne $script:OccupancyProbe) { return $script:OccupancyProbe } + try { + if (-not (Get-Command Get-WorktreeOccupancy -EA SilentlyContinue)) { + $script:OccupancyProbe = $false + return $false + } + $occ = Get-WorktreeOccupancy -Repo $repo + # `Available` false means the fence could not be built -- an unplaceable record, no repo. + # Treat that exactly like a failed probe. + $script:OccupancyProbe = if ($occ -and $occ.Available) { $occ } else { $false } + } + catch { $script:OccupancyProbe = $false } + return $script:OccupancyProbe +} + function Get-HolderLiveness([string]$HeldPath) { try { if (-not (Test-Path -LiteralPath $HeldPath)) { - return [pscustomobject]@{ State = 'gone'; QuietHours = $null } + return [pscustomobject]@{ State = 'gone'; QuietHours = $null; Occupants = $null } } $ct = & git -C $HeldPath log -1 --format=%ct 2>$null if ($ct) { $quiet = [int]((Get-Date) - [System.DateTimeOffset]::FromUnixTimeSeconds([long]$ct).LocalDateTime).TotalHours - return [pscustomobject]@{ State = 'present'; QuietHours = $quiet } + $occ = Get-OccupancyOnce + if ($occ) { + $who = @(Get-WorktreeOccupants -Occupancy $occ -Path $HeldPath) + if ($who.Count -eq 0) { + return [pscustomobject]@{ State = 'unoccupied'; QuietHours = $quiet; Occupants = 0 } + } + return [pscustomobject]@{ State = 'occupied'; QuietHours = $quiet; Occupants = $who.Count } + } + # PROBE UNAVAILABLE -> `present`, which keeps its PRE-#1348 meaning exactly: the path is + # there and that is all this function knows. It still refuses. + # + # THIS SPLIT IS A BUG FIX, NOT A TIDY-UP. The first version of #1348 returned `present` + # for BOTH "the probe looked and found an occupant" and "the probe could not look", and + # then labelled `present` "LIVE SESSION in the holder". On a machine with no Claude + # config root -- every CI runner -- the probe reports Available=false, so the fallback + # fired and the tool ASSERTED A LIVE SESSION IT HAD NEVER OBSERVED. Caught by the + # required windows-2025 leg on PR 585, three tests, deterministic rather than flaky. + # + # One state cannot mean both "I measured this" and "I could not measure this", and the + # deny text is where that conflation becomes a false statement to an operator. `occupied` + # is now the only state that claims a session, and it is reachable only through a probe + # that returned Available with a vetoing occupant for this exact path. + return [pscustomobject]@{ State = 'present'; QuietHours = $quiet; Occupants = $null } } # Present on disk but no commit to date it by -- a brand-new worktree looks exactly like this. - return [pscustomobject]@{ State = 'unknown'; QuietHours = $null } + return [pscustomobject]@{ State = 'unknown'; QuietHours = $null; Occupants = $null } } catch { # Say so rather than returning 'gone'. A failed probe that reported death would turn an # unreadable path into a licence to release someone's live claim. - return [pscustomobject]@{ State = 'failed'; QuietHours = $null } + return [pscustomobject]@{ State = 'failed'; QuietHours = $null; Occupants = $null } } } @@ -199,9 +264,22 @@ function Show-List { $live = Get-HolderLiveness $held switch ($live.State) { 'gone' { $age = " [HOLDER GONE -- worktree no longer exists; release with -Force]" } + 'occupied' { + $age = " [held ${hrs}h; LIVE SESSION in the holder, last committed $($live.QuietHours)h ago]" + if ($live.QuietHours -ge 12) { $age += " -- QUIET but OCCUPIED, ask before releasing" } + } + # The probe could not run -- no Claude config root, or it failed. Says what it knows + # and no more: the path is there. It must NOT claim a session it never looked for. 'present' { - $age = " [held ${hrs}h; holder last committed $($live.QuietHours)h ago]" - if ($live.QuietHours -ge 12) { $age += " -- QUIET, confirm with the holder before releasing" } + $age = " [held ${hrs}h; holder present, last committed $($live.QuietHours)h ago; OCCUPANCY UNKNOWN -- the session probe could not run]" + } + # THE THIRD STATE, and the listing is the surface that matters (BACKLOG #1348). + # -List is what a Cleaner or Dispatcher reads to decide where to spend attention, so + # a directory that outlived its session must not render identically to a lane that + # is building. It still says ROUTE, not release: the refusal is unchanged. + 'unoccupied' { + $age = " [held ${hrs}h; DIRECTORY ONLY -- no live session in it, last commit $($live.QuietHours)h ago]" + $age += " -- ROUTE to the Cleaner/Dispatcher; not releasable on this signal alone" } default { $age = " [held ${hrs}h; holder liveness UNKNOWN -- confirm before releasing]" } } @@ -246,10 +324,26 @@ if ($Release) { } 'present' { Write-Host " HOLDER IS STILL THERE -- that worktree exists and last committed $($live.QuietHours)h ago." -ForegroundColor Red + Write-Host " OCCUPANCY UNKNOWN: the session probe could not run, so this does NOT say whether" + Write-Host " anyone is in it. Treat that as MORE reason to coordinate, not less." + Write-Host " Do NOT -Force it on the strength of a quiet period: a session can be alive and" + Write-Host " simply not committing. Ask that session first -- releasing a live claim is how two" + Write-Host " sessions end up building the same thing." + } + 'occupied' { + Write-Host " HOLDER IS STILL THERE -- that worktree exists, a live session is IN it, and it last committed $($live.QuietHours)h ago." -ForegroundColor Red Write-Host " Do NOT -Force it on the strength of a quiet period: a session can be alive and" Write-Host " simply not committing. Ask that session first -- releasing a live claim is how two" Write-Host " sessions end up building the same thing." } + 'unoccupied' { + Write-Host " HOLDER IS A DIRECTORY, NOT A SESSION -- that worktree exists and last committed $($live.QuietHours)h ago," -ForegroundColor Yellow + Write-Host " but NO live session is placed in it. This is the third state (BACKLOG #1348)." + Write-Host " STILL NOT YOURS TO -Force. Nothing on this host can prove a session is gone: occupancy" + Write-Host " sees a session by the cwd it launched in, so one working here BY ABSOLUTE PATH from" + Write-Host " elsewhere is invisible to it. This is reported so you can ROUTE it, not act on it." + Write-Host " Route to the Cleaner or the Dispatcher -- releasing another worktree's claim is theirs." + } default { Write-Host " HOLDER LIVENESS UNKNOWN -- the worktree exists but could not be dated." -ForegroundColor Yellow Write-Host " Confirm with that session before using -Force." @@ -425,10 +519,26 @@ try { } 'present' { Write-Host " HOLDER IS STILL THERE -- that worktree exists and last committed $($live.QuietHours)h ago." -ForegroundColor Red + Write-Host " OCCUPANCY UNKNOWN: the session probe could not run, so this does NOT say whether" + Write-Host " anyone is in it. Treat that as MORE reason to coordinate, not less." + Write-Host " Do NOT build it in parallel -- that is the duplicate-work this gate exists to stop," + Write-Host " and do NOT -Force it: quiet is not dead. Coordinate with that session or pick" + Write-Host " different work. Its note above says what it is doing." + } + 'occupied' { + Write-Host " HOLDER IS STILL THERE -- that worktree exists, a live session is IN it, and it last committed $($live.QuietHours)h ago." -ForegroundColor Red Write-Host " Do NOT build it in parallel -- that is the duplicate-work this gate exists to stop," Write-Host " and do NOT -Force it: quiet is not dead. Coordinate with that session or pick" Write-Host " different work. Its note above says what it is doing." } + 'unoccupied' { + Write-Host " HOLDER IS A DIRECTORY, NOT A SESSION -- that worktree exists and last committed $($live.QuietHours)h ago," -ForegroundColor Yellow + Write-Host " but NO live session is placed in it. This is the third state (BACKLOG #1348)." + Write-Host " STILL NOT YOURS TO -Force, and the refusal is deliberate: occupancy can VETO but never" + Write-Host " authorise, because nothing here can prove a session is gone. A session working in this" + Write-Host " path BY ABSOLUTE PATH from another cwd does not appear as an occupant." + Write-Host " Hand it to the Cleaner or the Dispatcher with this line; do not build it in parallel." + } default { Write-Host " HOLDER LIVENESS UNKNOWN -- the worktree exists but could not be dated." -ForegroundColor Yellow Write-Host " Treat it as live: coordinate with that session before -Force." diff --git a/tests/test_coord_claim_liveness.py b/tests/test_coord_claim_liveness.py index 9bb784c3..8eeffabb 100644 --- a/tests/test_coord_claim_liveness.py +++ b/tests/test_coord_claim_liveness.py @@ -23,6 +23,7 @@ from __future__ import annotations +import json import os import shutil import subprocess @@ -219,8 +220,192 @@ def test_list_still_flags_a_vanished_holder(repo: Path, tmp_path: Path) -> None: def test_list_still_reports_a_living_holders_quiet_hours(repo: Path, tmp_path: Path) -> None: - """Anti-regression for the shared helper: 'present' must still carry its commit age.""" + """Anti-regression for the shared helper: 'present' must still carry its commit age. + + The wording moved with BACKLOG #1348 -- "holder last committed Nh ago" became "LIVE SESSION in + the holder, last committed Nh ago" -- because the old phrase was true of a directory nobody was + in. The commit age is still there; only the claim about WHO is there is new. + """ peer_holding(repo, tmp_path, "k") out = claim(repo, "-List").stdout - assert "holder last committed" in out + assert "last committed" in out assert "HOLDER GONE" not in out + + +# -------------------------------------------------------------------------------------------------- +# THE THIRD STATE: a holder that is a DIRECTORY rather than a SESSION (BACKLOG #1348) +# -------------------------------------------------------------------------------------------------- + +# `present` used to mean "the path exists". It never asked whether a session was IN it, so a +# worktree that outlived its session rendered identically to a lane actively building. Measured on +# the live board when this landed: 35 holders gone, 8 with a live session, and 23 that were a +# directory with nobody in it -- and all 31 of the last two groups had been rendering the same way. +# +# THE STATE REPORTS. IT DOES NOT PERMIT. Every test below pins BOTH halves, because a fix that +# turned the new state into a licence to release would be worse than the defect: occupancy.ps1's own +# rule is that it "may only ever VETO an action; a DEAD/STALE/absent verdict must never by itself +# authorise one", since a session working in a path by ABSOLUTE PATH from another cwd is invisible +# to a cwd-keyed probe. + + +@pytest.fixture +def repo_with_occupancy(repo: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """The sandbox, plus the occupancy probe claim.ps1 consults, plus a config root it can read. + + The base `repo` fixture deliberately copies ONLY claim.ps1, which is what makes the probe + unavailable there -- and that is a real configuration, not an artifact: a checkout carrying a + partial scripts/coord/ behaves exactly that way. Both are worth testing, so they get separate + fixtures rather than one that hides the difference. + + *** THE PLANTED CONFIG ROOT IS WHY THIS PASSES ON CI, and its absence is why the first version + of these tests did not. *** `Get-WorktreeOccupancy` reports Available=FALSE unless it finds at + least one READABLE SESSION RECORD -- not merely a config root, an actual record. Measured: + + no config root at all Available=False "no Claude config root ... was found" + a config root, empty sessions Available=False "not one readable session record in them" + a config root + one record Available=True Sessions=0 + + A CI runner has no Claude session registry, so the probe was unavailable there and the code fell + back to its unknown-occupancy state -- while these tests asserted the DIRECTORY-ONLY state, which + is reachable only through an available probe. They passed on a developer box and failed on + windows-2025, which is the worst shape a test can have: green exactly where nobody is watching. + + The record carries a DEAD pid, so it makes the probe available while placing NO occupant + anywhere. Occupancy is decided by whether the pid is running (session-registry.ps1), and 999999 + is not a live process on any runner. The cwd points outside the sandbox for the same reason. + """ + src = CLAIM.parent + for name in ("occupancy.ps1", "session-registry.ps1"): + shutil.copy2(src / name, repo / "scripts" / "coord" / name) + + home = tmp_path / "fakehome" + sessions = home / ".claude" / "sessions" + sessions.mkdir(parents=True) + (sessions / "999999.json").write_text( + json.dumps( + { + "cwd": str(tmp_path / "somewhere-else"), + "pid": 999999, + "sessionId": "planted-dead-session", + "startedAt": "2026-01-01T00:00:00Z", + "procStart": "2026-01-01T00:00:00Z", + "kind": "desktop", + "entrypoint": "test", + "name": "planted", + "nameSource": "test", + "peerProtocol": "none", + } + ), + encoding="utf-8", + ) + # Both, because the roots are resolved from the user profile and the two differ across shells. + monkeypatch.setenv("USERPROFILE", str(home)) + monkeypatch.setenv("HOME", str(home)) + return repo + + +def test_a_holder_with_no_live_session_is_distinguishable_from_one_with_a_session( + repo_with_occupancy: Path, tmp_path: Path +) -> None: + """The whole point of #1348: the two must not render identically. + + Nothing is running in the sandbox's peer worktree, so the probe places zero sessions in it. + """ + peer_holding(repo_with_occupancy, tmp_path, "k") + out = claim(repo_with_occupancy, "-List").stdout + assert "DIRECTORY ONLY" in out, out + assert "no live session in it" in out + # and it must not claim someone is there + assert "LIVE SESSION in the holder" not in out + + +def test_the_third_state_still_refuses_a_take(repo_with_occupancy: Path, tmp_path: Path) -> None: + """Distinguishable is not releasable. The refusal is unchanged.""" + peer_holding(repo_with_occupancy, tmp_path, "k") + proc = claim(repo_with_occupancy, "-Take", "k", "-Note", "mine now") + assert proc.returncode != 0, "an unoccupied holder must still block a take" + assert "A DIRECTORY, NOT A SESSION" in proc.stdout + assert "still not yours to -force" in proc.stdout.lower() + + +def test_the_third_state_does_not_recommend_force_on_a_release( + repo_with_occupancy: Path, tmp_path: Path +) -> None: + """`-Force` is recommended for exactly one state, and this is not it. + + The negative control is the vanished-holder case above, which DOES recommend it -- so this + assertion is about the state, not about the word being absent everywhere. + """ + peer_holding(repo_with_occupancy, tmp_path, "k") + proc = claim(repo_with_occupancy, "-Release", "k") + assert proc.returncode != 0 + assert "A DIRECTORY, NOT A SESSION" in proc.stdout + assert "Safe to take over" not in proc.stdout + + +def test_a_vanished_holder_still_outranks_the_third_state( + repo_with_occupancy: Path, tmp_path: Path +) -> None: + """Positive control for the probe being live at all. + + With occupancy available, a DELETED worktree must still read GONE and still recommend -Force. + If this ever reported the third state instead, the new branch would be swallowing the one + verdict that is safe to act on unasked. + """ + peer = peer_holding(repo_with_occupancy, tmp_path, "k") + orphan(repo_with_occupancy, peer) + out = claim(repo_with_occupancy, "-List").stdout + assert "HOLDER GONE" in out + assert "DIRECTORY ONLY" not in out + + +def test_without_the_probe_it_falls_back_to_REFUSING_not_to_the_new_state( + repo: Path, tmp_path: Path +) -> None: + """THE POLARITY RULE, and it is the one that must never regress. + + The base fixture has no occupancy.ps1, so the probe cannot load. A missing answer must cost a + REFUSAL, never a licence: recognition may only ever suppress. If an unloadable probe ever + produced "DIRECTORY ONLY", every checkout with a partial scripts/coord/ would start reporting + live lanes as abandoned -- the same false record, arrived at from the other side. + """ + peer_holding(repo, tmp_path, "k") + out = claim(repo, "-List").stdout + assert "DIRECTORY ONLY" not in out, "an unavailable probe must not produce the new state" + assert "last committed" in out + proc = claim(repo, "-Take", "k", "-Note", "mine now") + assert proc.returncode != 0, "the take must still be refused when the probe cannot load" + + +def test_an_unavailable_probe_never_claims_a_live_session(repo: Path, tmp_path: Path) -> None: + """THE REGRESSION TEST FOR THE BUG THAT REACHED CI. One state cannot mean two things. + + The first version of BACKLOG #1348 returned `present` BOTH when the probe looked and found an + occupant AND when the probe could not look at all -- then labelled `present` "LIVE SESSION in + the holder". On any machine with no Claude session registry, which is every CI runner, the + fallback fired and the tool ASSERTED A LIVE SESSION IT HAD NEVER OBSERVED. + + That is not a cosmetic wording defect. The whole point of the item is telling a DIRECTORY apart + from a PERSON, and a state that reports "person" when it means "I could not look" is the same + conflation the item exists to remove, reintroduced in the deny text. + + The base `repo` fixture copies only claim.ps1, so occupancy.ps1 cannot be dot-sourced and the + probe is genuinely unavailable -- the same condition as a runner, reached a different way. + """ + peer_holding(repo, tmp_path, "k") + out = claim(repo, "-List").stdout + assert "LIVE SESSION in the holder" not in out, ( + "the tool claimed a live session while the occupancy probe could not run. It cannot know " + f"that, and saying it is what shipped to CI.\n{out}" + ) + assert "OCCUPANCY UNKNOWN" in out, ( + "an unavailable probe must SAY it could not look, not fall silent into a state that reads " + f"as a measurement.\n{out}" + ) + # And it must still refuse -- naming the unknown is not a licence. + proc = claim(repo, "-Take", "k", "-Note", "mine now") + assert proc.returncode != 0 + assert "quiet is not dead" in proc.stdout, ( + "the unknown-occupancy branch dropped the safety guidance the occupied branch carries. " + "Only the liveness CLAIM should differ between them; the advice must not." + )