diff --git a/docs/LEDGER-GATE.md b/docs/LEDGER-GATE.md index ab1385ab..680054de 100644 --- a/docs/LEDGER-GATE.md +++ b/docs/LEDGER-GATE.md @@ -100,7 +100,22 @@ worse than no gate. `git commit --no-verify` bypasses the hook, and a branch cut from a **stale main** cannot see a collision at all — each branch is internally consistent, and the duplicate only exists once *both* have merged. So -CI re-runs the same rules with `--ci` against a freshly fetched `origin/main`. +CI re-runs the rules with `--ci` against a freshly fetched `origin/main`. + +**It re-runs all of them but one, and the exception is the ownership rule** — `ledger_check.py:196` and +`:241` both read `not self.ci and not self.owns(...)`, so *"was this number allocated to you"* is +**enforced locally and never in CI**. It has to be: `owns()` reads the allocation store from +`/mefor-coord/alloc`, a CI runner clones fresh and has none, so the check would return +False for every ADR and no ADR could ever merge. The consequence is worth stating plainly rather than +leaving as an inference — **a green CI on an ADR or BACKLOG PR is not evidence that the number was +allocated to anybody.** What CI *does* still catch, and what actually prevents the ledger corruption +this gate exists for, is collision-with-base, the missing index row, and duplicate rows. + +So the residual after `--no-verify` is narrower than "unprotected" and wider than "backstopped": a +number belonging to another session's *unmerged* branch can be taken and committed with nothing +objecting. The corruption surfaces when the second of the two merges — the collision rule blocks it — +which is late, loud, and recoverable, rather than silent. That is the property the gate was built for; +allocation discipline itself is on the honour system once the local hook is skipped. That step is **deliberately ungated** in `.github/workflows/ci.yml`. Every other step in the `test` job is conditioned on `code == 'true'`, which is **false for a docs-only PR** — and an ADR-only PR *is* docs-only. @@ -119,7 +134,8 @@ a pair. ## Limits - **`--no-verify` bypasses the pre-commit hook.** It is a guardrail, not a security boundary. The `--ci` - leg is the backstop, and it cannot be bypassed from a branch. + leg is the backstop for *collision*, and it cannot be bypassed from a branch — but it does **not** + re-check ownership (§3), so that one rule has no backstop at all. - **It does not stop two sessions building the same thing** under two different numbers. Duplicated work has no file conflict and no number conflict; nothing here sees it. - **Numbers leak.** An abandoned branch's number is never reclaimed. Accepted, deliberately. diff --git a/docs/SESSION-DRIFT-CONTROLS.md b/docs/SESSION-DRIFT-CONTROLS.md index c1253941..0dc56f92 100644 --- a/docs/SESSION-DRIFT-CONTROLS.md +++ b/docs/SESSION-DRIFT-CONTROLS.md @@ -85,7 +85,12 @@ and the open work claims. Frequently forgotten in discussions of "the gate", but it is the same problem class: - **[`scripts/coord/claim.ps1`](../scripts/coord/claim.ps1)** — atomic exclusive-create of - `/mefor-coord/claims/.json`. Claims work, not numbers. + `/mefor-coord/claims/.json`. Claims work, not numbers. `-Take` on a key you + already hold **refreshes the note and branch in place**, stamping `refreshed`; until 2026-08-02 it + accepted a new `-Note`, reported success and discarded it, so the only way to correct a note was + `-Release` then `-Take` — which drops the claim in between and re-opens the race it exists to close. + The note is what `announce-session.ps1` broadcasts to every joining session *in preference to the + worktree name*, so a note that cannot be corrected is announced as current intent indefinitely. - **[`scripts/hooks/claim_check.py`](../scripts/hooks/claim_check.py)** — `commit-msg` gate: a commit whose *subject* declares `BACKLOG #N` with a code-touching diff must hold a claim on N **for this worktree**. Motivated by a recorded incident: three sessions independently fixed one npm advisory; two PRs were @@ -114,6 +119,25 @@ writes when measured. > strictly downstream of the failure it existed to detect. Looking was not neglected; it was > impossible. When adding a control, ask which surface still reports when the control itself fails to > load. (Formulation owed to the session that hit four instances of the same class in one day.) +> +> **A fifth instance, found 2026-08-02 in the collision gate itself — fixed.** Every fail-open path +> (`overlap.ps1` missing, throwing, or printing garbage) exited 0 with **empty stdout**, which on this +> hook's stdout is byte-for-byte identical to *"checked, nobody else is in this file"*. A gate that had +> consulted nothing reported an all-clear, so its own failure reached the session as reassurance. The +> sharpest case needed no breakage at all, and the first attempt at the fix walked straight into it: +> **`overlap.ps1` had no representation for "nobody else"**. `@() | ConvertTo-Json -AsArray` sends zero +> objects down the pipeline, so ConvertTo-Json never runs and the script printed *nothing* — `-AsArray` +> only shapes output that already exists. A gate rule of "empty output means the script never answered" +> therefore fired on an ordinary edit to an untouched file, which is most edits. Caught by running the +> real pair rather than the test stubs, which used a shape the real script never produced. +> +> So the fix is in two places, and the second is the load-bearing one: `overlap.ps1` now always emits a +> JSON array (`[]` for no hits), and only then can the gate treat silence as a fault. It emits a +> `hookSpecificOutput.additionalContext` notice naming the reason, rate-limited per reason so a +> persistent fault cannot narrate itself into every edit, and **fails toward noise** when it cannot +> write that rate-limit stamp. The posture is unchanged: it still allows. Note the shape of the fix — +> it is not "check harder", it is *give the two states different bytes*, and you cannot detect a +> difference the producer never encoded. ### Recovery and lifecycle @@ -142,9 +166,12 @@ reading the emitted decision — not by reading source alone. | Selfheal — primary auto-repair | user (4 of 5 dirs) | LIVE | | Selfheal — hijack warning | user (4 of 5 dirs) | **LIVE and currently mis-firing** (§3, G4) | | `session-context.ps1` banner | project | LIVE where the branch carries the file | -| Announce-on-join (`announce-session.ps1`) | user | **NEW** — the only **push** control; asks, cannot send, and every decision leaves a receipt | +| Announce-on-join (`announce-session.ps1`) | user | **MERGED, NOT INSTALLED — inert by accident, 2026-08-02.** The only **push** control; asks, cannot send, and every decision leaves a receipt. It has never run. See below | | Announce wiring reaches a real script | test | **NEW** — `tests/test_announce_wiring.py`; nothing asserted this for *any* hook before, which is how a wired-but-inert shim survived weeks | | Announce missing-script notice | user | **NEW** — the one surface that still reports when the script itself fails to resolve | +| Collision gate — unresolved notice | user | **NEW** — every fail-open path used to be indistinguishable from an all-clear; it now says which reason, once per 30 min, and still allows | +| Claim note refresh (`-Take -Note` on a held key) | manual | **NEW** — was a silent discard; refresh is now in place, so correcting a note never drops the claim | +| Overlap session attribution | manual + gate | **FIXED** — longest-prefix, not first-hit. Linked worktrees are nested under the primary, so the primary's row used to absorb an arbitrary peer's session and report itself LIVE on `main` | | Claim / alloc / ledger gates | git hooks | LIVE | | `new.ps1` / `remove.ps1` / `prune-merged.ps1` | manual | LIVE, **sibling-layout only** | | `tests/test_worktree_gate*.py`, `test_install_gate_wiring.py` | CI + local | Was **85 green, and blind** — every one bound the repo copy; nothing read the installed copy or any live `settings.json`. Now 91 across six files, plus the local-only parity check below | @@ -153,6 +180,33 @@ Rule 4 being inert is **deliberate and announced** — the commit that landed it nothing changes until `install-gate.ps1` is re-run." It is listed as INERT here because a control that has never been installed is a source artefact, not an enforcement. +**Announce-on-join is inert too, and that one is an accident.** Measured 2026-08-02, hours after +[#133](https://github.com/MEFORORG/MessageFoundry/pull/133) merged it to `main`: + +| Check | Result | +|---|---| +| `mefor-announce` UserPromptSubmit entry in any of the 5 config roots | **absent** | +| The one UserPromptSubmit entry that *is* installed | `# mefor-web-announce`, resolving `scripts/hooks/announce.ps1` — **a different script in a different repo** | +| `/mefor-coord/announce/` | **does not exist**, so there is not one receipt: it has never executed | + +`install-coordination.ps1` was last run before the announce row existed, and merging a hook does not +install one. The two `mefor-coord` entries it wired then — the SessionStart banner and the collision +gate — are present, which is exactly why nothing looked wrong. + +Two things this costs, both observed rather than predicted. A peer session announced itself **by hand** +on 2026-08-02 and reported the hook as unavailable because it was "on an unmerged branch"; it had +merged, so the correct diagnosis was never reached. And the missing-script notice listed below — the +surface built precisely so this class cannot hide — **cannot fire when the hook is not wired at all**, +because it lives inside the shim. That is this section's own lesson recurring one level up: the +detector was still downstream of the failure. Re-arm from a plain terminal, then confirm by receipt +rather than by reading the settings file: + +```powershell +pwsh -NoProfile -File scripts\coord\install-coordination.ps1 +# then, after one prompt in any session in this repo: +ls (Join-Path (git rev-parse --path-format=absolute --git-common-dir) 'mefor-coord/announce/receipts') +``` + --- ## 2. What it demonstrably buys diff --git a/docs/WORKTREES.md b/docs/WORKTREES.md index 1d85314f..e0f8d872 100644 --- a/docs/WORKTREES.md +++ b/docs/WORKTREES.md @@ -321,6 +321,35 @@ hooks are wired at **user** level by [../scripts/coord/install-coordination.ps1](../scripts/coord/install-coordination.ps1): git cannot deliver a project-level hook to a worktree. +**A change to these scripts reaches a session by one of TWO rules, and they are not the same rule.** +Getting this wrong produces a confident, wrong answer to "can I use it yet", so state which one applies: + +| How the script is reached | When your change reaches a session | +|---|---| +| **Run by a hook** (`collision_gate.ps1`, and `overlap.ps1` as its callee) | The installed shim resolves the **primary checkout** first, falling back to the calling worktree. So: **when the primary advances** — regardless of what any session's own branch contains | +| **Run by hand from a worktree** (`claim.ps1`, `overlap.ps1`, `presence.ps1`) | From that session's **own tree**. So: **when that session's branch contains it** — the primary is irrelevant | + +Reported 2026-08-02 by a peer session that was told a merged `claim.ps1` improvement was available to +it, tried it, and got the old behaviour: its branch predated the change. Both halves of the answer were +individually true and the combination was wrong. Test the property, not the provenance, and test it +where the script will actually run from: + +```powershell +grep -c /scripts/coord/overlap.ps1 # hook-run scripts +grep -c ./scripts/coord/claim.ps1 # hand-run scripts +``` + +**Asking who holds a file, right now, without changing anything.** The collision gate answers this +directly, and it is the only command that does — `-PathOverride` makes it skip stdin and print its +decision: + +```powershell +pwsh -NoProfile -File scripts\hooks\collision_gate.ps1 -PathOverride docs\BACKLOG.md +``` + +Empty output means no live session holds it. Documented in-script as a test affordance; surfaced here +because a session that needed the answer found it by reading the source. + ## Announcing yourself (UserPromptSubmit hook) **What it fixes.** Everything above is **pull**-based: a new session discovers its peers and the peers @@ -341,7 +370,11 @@ the id rule; the model does the sending. banners is the **registry** id. `ccd_session_mgmt` uses a *different* id for the same session. **The cwd is the only join key, and it must be matched exactly, never by prefix** — every worktree cwd is an extension of the primary's, so a prefix match resolves a peer in the primary to an arbitrary worktree -session. Branch is not a join key either: measured 2026-08-01, the two rosters reported different +session. (The same trap, same nesting, was live in `overlap.ps1` until 2026-08-02: it *needs* a prefix +match, since a session may sit in any subdirectory, and it took the **first** hit — so the primary's row +absorbed whichever nested-worktree session the hash table enumerated first and reported the primary LIVE +on `main`. Where a prefix match is genuinely required, the rule has to be **longest prefix wins**.) +Branch is not a join key either: measured 2026-08-01, the two rosters reported different branches for the same checkout in 2 of 6 cases. A usable id starts with `local_`. **A registry id passed to `send_message` fails silently**, which reads as the peer ignoring you. @@ -395,9 +428,37 @@ it by hand. Three constraints came out of that, recorded here so the next attemp them: - **A broadcast needs an expiry or a predicate the *recipient* can evaluate — never a promise from the - sender.** A merge freeze went out with "lift when #119 merges". #119 never merged (it died on an - unrelated CI timeout), so five sessions held on a condition that could not arrive, and it took a - second round to retract. + sender.** A merge freeze went out with "lift when #119 merges", and five sessions held. **#119 + merged** — 2026-08-02 01:45:00Z, merge commit `002be182`, **12h15m** after its auto-merge was armed + (`auto_squash_enabled` 2026-08-01 13:29:37Z, from the issue timeline — note the event is + `auto_squash_enabled`, so a filter on `auto_merge_enabled` finds nothing and the wait looks + unmeasurable). The failure was never that the condition could not arrive; it was that **the world + moved while everyone waited**. `main` advanced four times before it did: + + | | | + |---|---| + | #74 | 2026-08-01 20:27:03Z | + | #120 | 2026-08-01 23:59:43Z | + | #131 | 2026-08-02 00:35:29Z | + | #130 | 2026-08-02 01:01:35Z | + + The first of those landed **8m26s** after the claim declaring the freeze was taken (that claim is + stamped 2026-08-01 23:51:17Z and is *still on the board*; #120 merged 23:59:43Z). Stated as the claim + timestamp rather than "when the freeze was written" deliberately — `claimed` records when the key was + taken, and only a `refreshed` stamp would evidence when the note itself was last edited. That field + is absent here, which on the code of the day is consistent: there was no way to edit a note in place, + so the two coincide unless someone hand-edited the JSON. + + So the freeze did not hold `main` still even while it was nominally in force — it held only the + sessions honouring it, which is the worst of both. And it outlived its own condition in the other + direction too: on 2026-08-02 that same claim note was still announcing the freeze to every joining + session hours after #119 had merged. A predicate the recipient cannot evaluate does not expire. + + This bullet said "#119 never merged" for a day, which made it a compensating control resting on a + false premise — the failure [`CLAUDE.md`](../CLAUDE.md) §11 names, inside the document arguing for + it. Corrected against the API, and the same framing was independently corrected in `ci.yml` + (`07b6e55a`) and in BACKLOG #340 by two other sessions; timestamps above are theirs, re-verified + here rather than restated. - **"Don't do X" is the wrong primitive when automation already has X armed.** The freeze asked sessions not to merge, while six PRs had auto-merge *armed* and would have landed with nobody clicking anything. The correct ask was an action — "disarm auto-merge" — not restraint. diff --git a/scripts/coord/claim.ps1 b/scripts/coord/claim.ps1 index 40390ee9..958aa235 100644 --- a/scripts/coord/claim.ps1 +++ b/scripts/coord/claim.ps1 @@ -36,7 +36,8 @@ #> [CmdletBinding()] param( - # Claim this key for THIS worktree. Idempotent: re-taking your own claim just refreshes the note. + # Claim this key for THIS worktree. Idempotent: re-taking a key you already hold refreshes its note + # and branch in place (never dropping the claim), rather than failing. [string]$Take, # Release a claim this worktree holds. [string]$Release, @@ -64,6 +65,16 @@ function ConvertTo-KeyFile([string]$Key) { $safe } +# ConvertFrom-Json SILENTLY COERCES an ISO-8601 string into a [datetime], so `[string]$c.claimed` does +# not give you back what was written -- it gives the local short form ("08/01/2026 23:17:46"), losing +# the sub-second precision and the UTC offset. Writing that back would quietly downgrade the stamp on +# every refresh, and it would still parse, so nothing would ever complain. Round-trip it instead. +function ConvertTo-Stamp($Value) { + if ($Value -is [datetime]) { return $Value.ToString("o") } + if ($Value -is [datetimeoffset]) { return $Value.ToString("o") } + return [string]$Value +} + function Get-Mine([string]$Path) { $c = Get-Content $Path -Raw | ConvertFrom-Json $held = ($c.worktree -replace '\\', '/').TrimEnd('/') @@ -154,8 +165,84 @@ try { } catch [System.IO.IOException] { $info = Get-Mine $file if ($info.IsMine) { - # Re-taking your own claim is a no-op, not an error: a session should be able to re-assert freely. + # Re-taking your own claim is not an error: a session should be able to re-assert freely. It was + # also, until now, not a REFRESH -- despite the -Take parameter documenting one. A new -Note was + # accepted, reported as success, and silently discarded. + # + # That is worse than an outright failure, because the note is the one field written deliberately + # to say what a session is doing, and announce-session.ps1 broadcasts it to every session that + # joins the repo, telling them to prefer it over the worktree name. Measured 2026-08-02: the + # 'announce-hook' claim still read "NO PR OPENED -- honouring the #119 merge freeze" to every + # joining session hours after both #133 and #119 had merged. A stale note broadcast as current + # intent is a coordination fault, not a cosmetic one -- and the documented workaround (-Release + # then -Take) drops the claim in between, re-opening the race the claim exists to close. + if ($Note) { + $updated = [ordered]@{ + # $Take, not the stored key: ConvertFrom-Json date-coerces any ISO-8601-SHAPED string, + # and a key is free text, so a key like "2026-08-01T00:00:00" would come back through + # [string] as "08/01/2026 00:00:00" -- a record naming a key nobody typed and -Release + # cannot be spelled to match. $Take is the caller's current spelling of the same key, + # and it folds to the same filename, which is the real identity. + key = $Take + note = $Note + # Refresh the branch too: a worktree can have switched branches since the claim was made, + # and a claim naming a branch nobody is on is another confidently-wrong coordination fact. + branch = $branch + worktree = [string]$info.Claim.worktree + # `claimed` is the identity of the claim and never moves; `refreshed` is what tells a + # reader how old the NOTE is, which is the question a stale note makes urgent. + claimed = ConvertTo-Stamp $info.Claim.claimed + refreshed = (Get-Date).ToString("o") + } | ConvertTo-Json -Compress + # Write-then-replace, not a truncating in-place write. A torn claim file does not fail loudly: + # scripts/hooks/claim_check.py swallows a parse error into "not claimed", which would disable + # the enforced gate for that key -- so a crash mid-write must leave the OLD file intact. + $tmp = "$file.$PID.tmp" + # UTF8 WITHOUT a BOM, for that same reader: a BOM makes json.loads raise. + [System.IO.File]::WriteAllBytes($tmp, [System.Text.Encoding]::UTF8.GetBytes($updated)) + + # [IO.File]::Move(.., overwrite) AND NOT `Move-Item -Force`. THE CLAIM FILE'S EXISTENCE *IS* + # THE LOCK -- the take path above is an exclusive CreateNew, so any instant in which the name + # does not exist is an instant another worktree can claim a key we hold. `Move-Item -Force` + # is delete-then-rename and opens exactly that window: measured on this box, 400 moves left + # the destination absent on 2,559 of 154,506 polls. The same harness over [IO.File]::Move + # with overwrite -- which is MoveFileEx(MOVEFILE_REPLACE_EXISTING), atomic on NTFS -- polled + # 134,581 times and never once saw the name missing. + # + # It can fail transiently instead (a scanner or an editor holding the destination without + # FILE_SHARE_DELETE); the same harness saw 13.5% under back-to-back churn, which is nothing + # like one refresh per invocation but is cheap to absorb. Failing is the SAFE direction: the + # old note survives and the claim stays ours. Losing the lock is not. + # + # The catch is deliberately UNTYPED. PowerShell wraps an exception thrown by a .NET METHOD in + # a MethodInvocationException, so `catch [System.IO.IOException]` around this call never + # matches -- the failure escapes to $ErrorActionPreference = "Stop", the cleanup below never + # runs, and the temp file is orphaned in the claim registry. (Written typed first; the + # orphaned-temp assertion is what caught it.) Every failure here has the same right answer + # anyway: leave the old note, keep the claim, say so. + $moved = $false + foreach ($attempt in 1..5) { + try { [System.IO.File]::Move($tmp, $file, $true); $moved = $true; break } + catch { Start-Sleep -Milliseconds (20 * $attempt) } + } + if (-not $moved) { + # Never orphan the temp: this directory is the claim registry, and a k.json..tmp + # nothing ever removes accumulates in it for the life of the repo. + Remove-Item -LiteralPath $tmp -Force -ErrorAction SilentlyContinue + Write-Host "" + Write-Host "Could NOT refresh the note on '$Take' -- the file was locked by another process." -ForegroundColor Yellow + Write-Host " Your claim is UNCHANGED and still yours; only the note was not updated." + Write-Host " Retry in a moment. Do NOT -Release: that would drop a claim you still hold." + exit 1 + } + Write-Host "" + Write-Host "REFRESHED '$Take' (held since $($info.Claim.claimed))." -ForegroundColor Green + Write-Host " note : $Note" + exit 0 + } Write-Host "You already hold '$Take' (claimed $($info.Claim.claimed))." -ForegroundColor Green + Write-Host " note : $($info.Claim.note)" + Write-Host " (pass -Note to update it -- it is what other sessions are shown.)" exit 0 } Write-Host "" diff --git a/scripts/coord/overlap.ps1 b/scripts/coord/overlap.ps1 index feed3ba0..74252b06 100644 --- a/scripts/coord/overlap.ps1 +++ b/scripts/coord/overlap.ps1 @@ -68,6 +68,19 @@ function ConvertTo-Norm([string]$p) { return ($p -replace '\\', '/').TrimEnd('/').ToLowerInvariant() } +# One emitter for every -Json exit, so no path can print nothing. See the note at the -File query below. +# +# The null filter is load-bearing, not defensive tidying. `Build-Map` returning no rows yields +# AutomationNull, which PARAMETER BINDING converts to a real $null on the way in -- and `@($null).Count` +# is 1, so the zero-rows guard was dead in exactly the case it was written for and the whole-map query +# emitted `[null]`: a phantom row, strictly worse than the nothing it replaced. Caught by adversarial +# review after the -File path had been verified by hand; the two call sites do not fail alike. +function Write-JsonArray($Rows) { + $r = @($Rows | Where-Object { $null -ne $_ }) + if ($r.Count -eq 0) { Write-Output "[]"; return } + Write-Output ($r | ConvertTo-Json -Depth 6 -AsArray) +} + $gitArgs = @(); if ($Repo) { $gitArgs = @("-C", $Repo) } $common = (& git @gitArgs rev-parse --path-format=absolute --git-common-dir 2>$null) if ($LASTEXITCODE -ne 0 -or -not $common) { @@ -124,11 +137,47 @@ function Build-Map { # we report a worktree as dormant when its owner is actually around, and dormant still shows. if ($l.State -ne "LIVE" -and $l.State -ne "UNVERIFIED") { continue } $k = ConvertTo-Norm $e.Record.cwd - $sessionsByCwd[$k] = $e.Record + # A FENCED session outranks an unfenceable one for the same directory. UNVERIFIED is the shape a + # crashed session's record takes once its pid is recycled -- alive, but nothing proves it is the + # same run -- so letting one displace a genuinely LIVE record would report a ghost's id and branch + # for a worktree somebody is really sitting in. Last-write-wins had no opinion about which it kept. + $rank = if ($l.State -eq "LIVE") { 0 } else { 1 } + if ($sessionsByCwd.ContainsKey($k) -and $sessionsByCwd[$k].Rank -le $rank) { continue } + $sessionsByCwd[$k] = [pscustomobject]@{ Record = $e.Record; Rank = $rank } + } + + $worktrees = @(Get-WorktreeList) + + # ATTRIBUTE A SESSION TO THE MOST SPECIFIC WORKTREE CONTAINING IT, not to the first one that matches. + # + # Linked worktrees live UNDER the primary checkout (.../MessageFoundry/.claude/worktrees/), so + # every linked worktree's path is ALSO a prefix match for the primary's row. The old rule walked the + # session table and broke on the first hit, so the primary row was handed whichever linked-worktree + # session the hashtable happened to enumerate first -- reporting the primary checkout as LIVE, on a + # branch nobody was on, "building" a peer's task list. Hashtable order is not stable, so the wrong + # answer was also a different wrong answer each run, which is why it read as noise rather than a bug. + # + # Longest prefix wins is the only rule that survives nesting: a session in .../worktrees/foo matches + # both the primary and foo, and foo is the one it is actually sitting in. + $ownerByWorktree = @{} + foreach ($k in @($sessionsByCwd.Keys | Sort-Object)) { + $best = $null + foreach ($w in $worktrees) { + $wn = ConvertTo-Norm $w.Path + if ($k -eq $wn -or $k.StartsWith("$wn/")) { + if ($null -eq $best -or $wn.Length -gt $best.Length) { $best = $wn } + } + } + # Two sessions inside one worktree: the fenced one wins, then first by sorted cwd -- so the + # answer is the same every run, and a ghost never displaces a session that is really there. + if ($best) { + $cur = $ownerByWorktree[$best] + if ($null -eq $cur -or $sessionsByCwd[$k].Rank -lt $cur.Rank) { $ownerByWorktree[$best] = $sessionsByCwd[$k] } + } } $rows = @() - foreach ($w in (Get-WorktreeList)) { + foreach ($w in $worktrees) { $norm = ConvertTo-Norm $w.Path if ($norm -eq $myRootNorm) { continue } # not a collision with yourself if (-not (Test-Path -LiteralPath $w.Path)) { continue } @@ -169,11 +218,9 @@ function Build-Map { $files += $dirty $files = @($files | Where-Object { $_ } | Sort-Object -Unique) - # A session sitting anywhere INSIDE the worktree owns it, not just one whose cwd is the root. - $sess = $null - foreach ($k in $sessionsByCwd.Keys) { - if ($k -eq $norm -or $k.StartsWith("$norm/")) { $sess = $sessionsByCwd[$k]; break } - } + # A session sitting anywhere INSIDE the worktree owns it, not just one whose cwd is the root -- + # resolved above, against every worktree at once, because "inside" is ambiguous when they nest. + $sess = if ($ownerByWorktree.ContainsKey($norm)) { $ownerByWorktree[$norm].Record } else { $null } if ($files.Count -eq 0 -and -not $sess) { continue } $rows += [pscustomobject]@{ @@ -241,7 +288,13 @@ if ($File) { $hits += $r } } - if ($Json) { ($hits | ConvertTo-Json -Depth 6 -AsArray) | Write-Output; exit 0 } + # ALWAYS EMIT AN ARRAY. `@() | ConvertTo-Json -AsArray` sends ZERO objects down the pipeline, so + # ConvertTo-Json never runs and the script prints NOTHING -- despite -AsArray, which only shapes + # output that exists. "Nobody else is in this file" therefore looked identical on stdout to "this + # script died before it could answer", and a consumer had no way to tell an all-clear from a + # failure. (-InputObject is not the fix: with -AsArray it double-wraps to [[]].) Measured + # 2026-08-02 against the real script. + if ($Json) { (Write-JsonArray $hits); exit 0 } foreach ($h in $hits) { $state = if ($h.Live) { "LIVE $($h.Surface) session $($h.Short)" } else { "dormant worktree" } Write-Host " $File is also changed by $state in $($h.Worktree) [$($h.Branch)]" @@ -249,7 +302,7 @@ if ($File) { exit 0 } -if ($Json) { ($map | ConvertTo-Json -Depth 6 -AsArray) | Write-Output; exit 0 } +if ($Json) { (Write-JsonArray $map); exit 0 } if (@($map).Count -eq 0) { Write-Host "No other worktree has changes."; exit 0 } Write-Host "" diff --git a/scripts/hooks/announce-session.ps1 b/scripts/hooks/announce-session.ps1 index 88ebbd79..60014ecc 100644 --- a/scripts/hooks/announce-session.ps1 +++ b/scripts/hooks/announce-session.ps1 @@ -96,6 +96,13 @@ function Get-ClaimNotes { # The claim note is the only field written DELIBERATELY to say what a session is doing, so lead # with it where one exists. Fail-open: no claims, unreadable claims, or no claim for a peer all # just mean the name is all we have. + # + # CARRY THE NOTE'S AGE, NOT JUST THE NOTE. Elevating the claim note to authoritative -- which the + # roster below explicitly does, telling readers to prefer it over the worktree name -- makes a STALE + # note strictly more dangerous than no note: it is read as current intent. Measured 2026-08-02: the + # 'announce-hook' note still told every joining session "NO PR OPENED -- honouring the #119 merge + # freeze" hours after both #133 and #119 had merged. Age is the cheap signal that lets a reader + # discount it. `refreshed` where the holder updated the note, else `claimed`. param([string]$ClaimsDir) $map = @{} try { @@ -103,7 +110,25 @@ function Get-ClaimNotes { foreach ($f in @(Get-ChildItem -LiteralPath $ClaimsDir -Filter '*.json' -ErrorAction SilentlyContinue)) { try { $c = Get-Content -LiteralPath $f.FullName -Raw | ConvertFrom-Json - if ($c.worktree -and $c.note) { $map[(Get-Norm ([string]$c.worktree))] = [string]$c.note } + if (-not ($c.worktree -and $c.note)) { continue } + $hrs = $null + # An unparseable or absent stamp leaves $hrs null, and the caller then prints the note + # with no age claim at all -- an unknown age must not be rendered as a fresh one. + try { + # ConvertFrom-Json hands these back as [datetime] already. Round-tripping through + # [string] would lose the offset and then re-parse under the host's culture, which + # is how a date silently becomes a different date. + $at = if ($c.refreshed) { $c.refreshed } else { $c.claimed } + $when = if ($at -is [datetime]) { $at } + elseif ($at -is [datetimeoffset]) { $at.LocalDateTime } + elseif ($at) { [datetime]::Parse([string]$at, [cultureinfo]::InvariantCulture) } + else { $null } + if ($when) { $hrs = [int]((Get-Date) - $when).TotalHours } + } catch { $hrs = $null } + $map[(Get-Norm ([string]$c.worktree))] = [pscustomobject]@{ + Note = [string]$c.note + Hours = $hrs + } } catch { } } } catch { } @@ -565,6 +590,8 @@ try { $lines += ' Read "claim:" where present and IGNORE the worktree name: the name is a' $lines += ' creation-time label, nothing keeps it current, and one of them is known to' $lines += ' describe work that session never did. The claim is written deliberately.' + $lines += ' Its bracketed age is how old the NOTE is, not how long the work has run --' + $lines += ' an old note may describe a PR that has since merged. Verify before relying.' $i = 0 foreach ($e in $listed) { $i++ @@ -575,8 +602,11 @@ try { $tail = if ($e.Reason) { " ($($e.Reason))" } else { '' } $lines += " [$i] $verb $(Get-Clean ([string]$p.Worktree) 40) [$(Get-Clean ([string]$p.Branch) 60)] $(Get-Clean ([string]$p.Surface) 16)/$(Get-Clean ([string]$p.Login) 24)$flag$tail" $lines += " cwd: $(Get-Clean ([string]$p.Cwd) 200)" - $note = $claims[(Get-Norm ([string]$p.Cwd))] - if ($note) { $lines += " claim: $(Get-Clean ([string]$note) 160)" } + $claim = $claims[(Get-Norm ([string]$p.Cwd))] + if ($claim) { + $stamp = if ($null -ne $claim.Hours) { " [written $($claim.Hours)h ago]" } else { ' [age unknown]' } + $lines += " claim: $(Get-Clean ([string]$claim.Note) 160)$stamp" + } } if ($more -gt 0) { $lines += " ...and $more more (run: pwsh -NoProfile -File scripts\coord\presence.ps1)" diff --git a/scripts/hooks/collision_gate.ps1 b/scripts/hooks/collision_gate.ps1 index b07b9c35..1582ddf1 100644 --- a/scripts/hooks/collision_gate.ps1 +++ b/scripts/hooks/collision_gate.ps1 @@ -22,6 +22,16 @@ session cannot work. That is the opposite of the worktree gate's posture (which protects the shared tree and should fail closed), and the difference is intentional. + FAILS OPEN, BUT NOT SILENTLY. Every one of those error paths used to `exit 0` with no output, which + on stdout is byte-for-byte what "checked, nobody else is touching this file" looks like. So a gate + that had checked NOTHING was indistinguishable from a gate reporting all-clear, and a session could + read the absence of a warning as evidence -- while the guard was inert. An unresolved run now says + so via additionalContext. It still allows: the posture is unchanged, only the silence is. + + Rate-limited per reason (`-NoticeCooldownMinutes`), because a persistently broken overlap script + would otherwise inject a notice into EVERY Edit and Write. If the throttle state cannot be read or + written the notice is emitted anyway -- silence is the defect being fixed, so the fallback is noise. + Wired on Edit|Write|MultiEdit|NotebookEdit. The overlap map is cached, so the common case is a cache read, not a git walk across every worktree. #> @@ -31,7 +41,13 @@ param( # than re-implementing its rule -- a test that asserts a copy of the rule proves nothing. [string]$OverlapScript = (Join-Path $PSScriptRoot "..\coord\overlap.ps1"), # Emit the decision and skip reading stdin (tests). - [string]$PathOverride + [string]$PathOverride, + # Where the per-reason "already told them" stamps live. Defaults to the repo's coordination dir. + # Parameterised so tests are isolated from the real repo AND from each other -- a throttle sharing + # one directory with the suite would make the first test's notice suppress the second's. + [string]$StateDir, + # How long one unresolved reason stays quiet after being reported. + [int]$NoticeCooldownMinutes = 30 ) # No $ErrorActionPreference = Stop: this gate fails OPEN, and a throw would be a deny-by-crash. @@ -51,25 +67,113 @@ function Deny([string]$Reason) { exit 0 } +function Write-Unresolved([string]$Slug, [string]$Detail) { + # THE GATE DID NOT CHECK. Say so, and allow anyway. + # + # It must be a JSON hookSpecificOutput.additionalContext payload and never a bare line: this is a + # PreToolUse hook whose stdout is PARSED AS A DECISION, so a stray line risks a misparse on every + # single Edit and Write -- turning a diagnostic into a worse fault than the one it reports. + # + # No permissionDecision key: adding one here would convert a broken guard into a blocked session, + # which is the fail-open posture inverted. + $emit = $true + try { + $dir = $StateDir + if (-not $dir) { + $common = (& git rev-parse --path-format=absolute --git-common-dir 2>$null) + if ($LASTEXITCODE -eq 0 -and $common) { $dir = Join-Path ([string]$common).Trim() "mefor-coord" } + } + if ($dir) { + # PER WORKTREE, not per repo. The stamp lives in the SHARED git-common-dir, so a single + # repo-wide stamp would mean the first session to hit a broken gate silences it for every + # other session for the whole cooldown -- and those sessions would read that silence as + # "checked, nobody is here", which is precisely the defect this notice exists to remove. + # One session's diagnostic must never become another session's false all-clear. + $who = "shared" + $top = (& git rev-parse --path-format=absolute --show-toplevel 2>$null) + if ($LASTEXITCODE -eq 0 -and $top) { + $who = (Split-Path ([string]$top).Trim() -Leaf) -replace '[^A-Za-z0-9._-]+', '-' + if (-not $who) { $who = "shared" } + } + $stamp = Join-Path $dir "gate-unresolved/$who.$Slug.stamp" + $prev = Get-Item -LiteralPath $stamp -ErrorAction SilentlyContinue + if ($prev) { + $mins = ((Get-Date) - $prev.LastWriteTime).TotalMinutes + # Bound BOTH ways. A stamp dated in the future (clock skew, a copied tree) would read as + # eternally fresh and suppress this notice forever -- the same silence, now self-inflicted. + if ($mins -ge 0 -and $mins -lt $NoticeCooldownMinutes) { $emit = $false } + } + if ($emit) { + New-Item -ItemType Directory -Force -Path (Split-Path $stamp) | Out-Null + Set-Content -LiteralPath $stamp -Value ((Get-Date).ToString("o")) -Encoding UTF8 + } + } + } catch { + # Cannot throttle -> report. Silence is the defect being fixed, so the failure mode of the + # noise-suppressor must be noise, never quiet. + $emit = $true + } + if ($emit) { + $payload = @{ + hookSpecificOutput = @{ + hookEventName = "PreToolUse" + additionalContext = "[collision] The collision gate could NOT check this edit ($Slug): $Detail. It allowed the edit without consulting any peer worktree, so an absent collision warning means UNKNOWN here, not clear. Check by hand before assuming nobody else is in this file: pwsh -NoProfile -File scripts\coord\overlap.ps1" + } + } + [Console]::Out.Write(($payload | ConvertTo-Json -Compress -Depth 6)) + } + exit 0 +} + $target = $PathOverride if (-not $target) { if (-not [Console]::IsInputRedirected) { exit 0 } - try { $hook = [Console]::In.ReadToEnd() | ConvertFrom-Json } catch { exit 0 } - if (-not $hook) { exit 0 } + try { $hook = [Console]::In.ReadToEnd() | ConvertFrom-Json -ErrorAction Stop } catch { + Write-Unresolved "payload-unreadable" ` + "the PreToolUse payload on stdin was not readable JSON, so the gate never learned which file this edit targets" + } + # Parsed, but to nothing. An empty payload or a literal `null` on stdin does not throw, so this + # used to be the one unreadable-input path that still exited silently -- the gate had learned no + # more than in the case above, and said no more than an all-clear. + if (-not $hook) { + Write-Unresolved "payload-empty" ` + "stdin carried no PreToolUse payload, so the gate never learned which file this edit targets" + } $target = [string]$hook.tool_input.file_path # NotebookEdit and some variants name the path differently; absence just means nothing to check. if (-not $target) { $target = [string]$hook.tool_input.notebook_path } } if (-not $target) { exit 0 } -if (-not (Test-Path -LiteralPath $OverlapScript)) { exit 0 } +if (-not (Test-Path -LiteralPath $OverlapScript)) { + Write-Unresolved "overlap-missing" "no overlap script at $OverlapScript" +} -$rows = @() +$raw = $null +$code = 0 try { $raw = & pwsh -NoProfile -NonInteractive -File $OverlapScript -File $target -Json 2>$null - if ($raw) { $rows = @($raw | ConvertFrom-Json) } -} catch { exit 0 } -if (-not $rows -or $rows.Count -eq 0) { exit 0 } + $code = $LASTEXITCODE +} catch { + Write-Unresolved "overlap-threw" "invoking the overlap script raised: $($_.Exception.Message)" +} +if ($code -ne 0) { + Write-Unresolved "overlap-failed" "the overlap script exited $code" +} + +# EMPTY OUTPUT IS NOT AN ANSWER. Under -Json a resolved "nobody else is in this file" is the two bytes +# `[]`; nothing at all is a script that never produced a verdict. Folding those together is precisely +# how this gate came to report all-clear while checking nothing. +$text = (@($raw) -join "`n").Trim() +if (-not $text) { + Write-Unresolved "overlap-empty" "the overlap script produced no output at all (a resolved 'nobody else' is '[]', not nothing)" +} + +$rows = @() +try { $rows = @($text | ConvertFrom-Json -ErrorAction Stop) } catch { + Write-Unresolved "overlap-unparseable" "the overlap script's output was not JSON" +} +if (-not $rows -or $rows.Count -eq 0) { exit 0 } # RESOLVED, and nobody else is touching it $live = @($rows | Where-Object { $_.Live }) if ($live.Count -eq 0) { exit 0 } # dormant only: worth knowing, not worth blocking diff --git a/tests/test_collision_gate.py b/tests/test_collision_gate.py index 240ab6bb..cc3c6b55 100644 --- a/tests/test_collision_gate.py +++ b/tests/test_collision_gate.py @@ -53,16 +53,35 @@ def make_overlap_stub(tmp_path: Path, rows: list[dict[str, Any]]) -> Path: return stub -def run_gate(overlap: Path | None, file_path: str | None = "a.py") -> dict[str, Any] | None: - """Invoke the gate exactly as Claude Code does. Returns the deny object, or None for allow.""" +def run_gate( + overlap: Path | None, + file_path: str | None = "a.py", + state_dir: Path | None = None, + raw_input: str | None = None, + cwd: Path | None = None, +) -> dict[str, Any] | None: + """Invoke the gate exactly as Claude Code does. Returns the emitted object, or None for silence. + + ``state_dir`` isolates the unresolved-notice throttle. Without it the gate would stamp the REAL + repository's coordination directory, so one test's notice would silence the next one's -- and the + suite would pass or fail on the order it happened to run in. + """ payload: dict[str, Any] = {"tool_name": "Edit", "tool_input": {}} if file_path is not None: payload["tool_input"]["file_path"] = file_path args = ["pwsh", "-NoProfile", "-NonInteractive", "-File", str(GATE)] if overlap is not None: args += ["-OverlapScript", str(overlap)] + if state_dir is not None: + args += ["-StateDir", str(state_dir)] proc = subprocess.run( - args, input=json.dumps(payload), capture_output=True, text=True, timeout=180, check=False + args, + input=json.dumps(payload) if raw_input is None else raw_input, + capture_output=True, + text=True, + timeout=180, + check=False, + cwd=None if cwd is None else str(cwd), ) # A hook must never crash the tool call: a non-zero exit is ignored by the harness, which would # leave the gate looking installed while permitting everything. @@ -158,26 +177,151 @@ def test_an_editing_peer_still_denies_when_another_peer_merely_committed(tmp_pat assert got["hookSpecificOutput"]["permissionDecision"] == "deny" -def test_fails_open_when_the_overlap_script_is_missing(tmp_path: Path) -> None: - assert run_gate(tmp_path / "does-not-exist.ps1") is None +def test_allows_a_payload_with_no_file_path(tmp_path: Path) -> None: + assert run_gate(make_overlap_stub(tmp_path, [LIVE_ROW]), file_path=None) is None + + +# ------------------------------------------------------------------- failing open, but not silently +# +# Every one of these paths used to `exit 0` with EMPTY STDOUT -- which is byte-for-byte what "checked, +# nobody else is in this file" looks like. A gate that had consulted nothing was indistinguishable from +# a gate reporting all-clear, so its own failure was reported to the session as reassurance. +# +# The posture does not change: all of them still ALLOW. Only the silence does. -def test_fails_open_when_the_overlap_script_throws(tmp_path: Path) -> None: +def unresolved(got: dict[str, Any] | None) -> str: + """Assert the shape of an unresolved notice and return its text.""" + assert got is not None, "an unresolved gate must say so, not exit silently" + out = got["hookSpecificOutput"] + # THE FAIL-OPEN POSTURE IS THE POINT. A notice that carried a permissionDecision would turn a + # broken guard into a blocked session -- strictly worse than the silence it replaces. + assert "permissionDecision" not in out, f"a diagnostic must never block: {out}" + assert out["hookEventName"] == "PreToolUse" + ctx: str = out["additionalContext"] + # It must be a JSON payload, not a bare line: this hook's stdout is parsed as a DECISION, so a + # stray line risks a misparse on every Edit and Write. json.loads in run_gate already proved that. + assert "could NOT check" in ctx + return ctx + + +def test_says_so_when_the_overlap_script_is_missing(tmp_path: Path) -> None: + got = run_gate(tmp_path / "does-not-exist.ps1", state_dir=tmp_path / "state") + assert "overlap-missing" in unresolved(got) + + +def test_says_so_when_the_overlap_script_throws(tmp_path: Path) -> None: broken = tmp_path / "broken.ps1" broken.write_text("param([string]$File,[switch]$Json)\nthrow 'boom'\n", encoding="utf-8") - assert run_gate(broken) is None + ctx = unresolved(run_gate(broken, state_dir=tmp_path / "state")) + assert "overlap-failed" in ctx or "overlap-threw" in ctx -def test_fails_open_when_the_overlap_script_emits_junk(tmp_path: Path) -> None: +def test_says_so_when_the_overlap_script_emits_junk(tmp_path: Path) -> None: junk = tmp_path / "junk.ps1" junk.write_text( "param([string]$File,[switch]$Json)\nWrite-Output 'not json'\n", encoding="utf-8" ) - assert run_gate(junk) is None + assert "overlap-unparseable" in unresolved(run_gate(junk, state_dir=tmp_path / "state")) -def test_allows_a_payload_with_no_file_path(tmp_path: Path) -> None: - assert run_gate(make_overlap_stub(tmp_path, [LIVE_ROW]), file_path=None) is None +def test_says_so_when_the_overlap_script_answers_with_nothing(tmp_path: Path) -> None: + """THE CONFLATION THIS FIX EXISTS FOR, and the only one with no other symptom. + + Under ``-Json`` a *resolved* "nobody else is in this file" is the two bytes ``[]``. A script that + exits 0 having printed nothing has produced no verdict at all. The gate treated both as "no rows, + allow", so a silently-broken overlap script was reported to the session as an all-clear forever. + """ + mute = tmp_path / "mute.ps1" + mute.write_text("param([string]$File,[switch]$Json)\nexit 0\n", encoding="utf-8") + assert "overlap-empty" in unresolved(run_gate(mute, state_dir=tmp_path / "state")) + + +def test_a_resolved_empty_answer_stays_silent(tmp_path: Path) -> None: + """The other half of that pair, and the reason it cannot simply always warn. + + ``[]`` IS an answer. This is the hot path on every single Edit and Write, so a gate that spoke up + here would put a line into the context of every edit in the repo forever. + """ + assert run_gate(make_overlap_stub(tmp_path, []), state_dir=tmp_path / "state") is None + + +def test_says_so_when_the_hook_payload_is_unreadable(tmp_path: Path) -> None: + got = run_gate( + make_overlap_stub(tmp_path, [LIVE_ROW]), state_dir=tmp_path / "state", raw_input="{not json" + ) + assert "payload-unreadable" in unresolved(got) + + +def test_the_notice_is_rate_limited_per_reason(tmp_path: Path) -> None: + """A persistently broken overlap script must not narrate itself into every edit. + + Same state dir twice: the first call reports, the second is suppressed. This is the difference + between a diagnostic and a nag, and this gate's own docstring names where nags end up. + """ + state = tmp_path / "state" + missing = tmp_path / "does-not-exist.ps1" + assert run_gate(missing, state_dir=state) is not None, "first occurrence must be reported" + assert run_gate(missing, state_dir=state) is None, "second occurrence must be suppressed" + + +def test_a_different_reason_is_not_suppressed_by_the_first(tmp_path: Path) -> None: + """Per-reason, not global -- otherwise one benign fault masks every later one.""" + state = tmp_path / "state" + assert run_gate(tmp_path / "does-not-exist.ps1", state_dir=state) is not None + junk = tmp_path / "junk.ps1" + junk.write_text( + "param([string]$File,[switch]$Json)\nWrite-Output 'not json'\n", encoding="utf-8" + ) + assert "overlap-unparseable" in unresolved(run_gate(junk, state_dir=state)) + + +def test_the_throttle_does_not_silence_a_different_worktree(tmp_path: Path) -> None: + """ONE SESSION'S DIAGNOSTIC MUST NOT BECOME ANOTHER SESSION'S FALSE ALL-CLEAR. + + The stamp lives in the SHARED git-common-dir. Keyed per repo, the first session to hit a broken + gate would silence it for every other session for the whole cooldown -- and those sessions read + silence as "checked, nobody is here", which is the exact defect the notice exists to remove. + """ + state = tmp_path / "state" + missing = tmp_path / "does-not-exist.ps1" + a, b = tmp_path / "wt-a", tmp_path / "wt-b" + for wt in (a, b): + wt.mkdir() + subprocess.run(["git", "init", "-q", str(wt)], check=True, capture_output=True) + + assert run_gate(missing, state_dir=state, cwd=a) is not None, "first worktree must be told" + assert run_gate(missing, state_dir=state, cwd=b) is not None, "so must the second" + assert run_gate(missing, state_dir=state, cwd=a) is None, "but not the same one twice" + + +def test_says_so_when_the_payload_is_empty(tmp_path: Path) -> None: + """Empty stdin and a literal `null` do not raise, so this was the one unreadable-input path that + still exited silently -- having learned no more than the case above, and said no less than an + all-clear.""" + for payload in ("", "null"): + got = run_gate( + make_overlap_stub(tmp_path, [LIVE_ROW]), + state_dir=tmp_path / f"state-{len(payload)}", + raw_input=payload, + ) + assert "payload-empty" in unresolved(got), f"silent on {payload!r}" + + +def test_an_unwritable_throttle_reports_anyway(tmp_path: Path) -> None: + """The noise-suppressor must fail toward NOISE. + + If it failed toward quiet, a coordination directory that could not be written would restore the + exact silent-allow this whole change removes -- and it would do it invisibly. + """ + blocker = tmp_path / "a-file" + blocker.write_text("not a directory", encoding="utf-8") + unwritable = blocker / "state" # cannot be created: its parent is a file + missing = tmp_path / "does-not-exist.ps1" + assert run_gate(missing, state_dir=unwritable) is not None + assert run_gate(missing, state_dir=unwritable) is not None, ( + "must not go quiet when it cannot stamp" + ) # --------------------------------------------------------------------------------- installer diff --git a/tests/test_coord_claim_refresh.py b/tests/test_coord_claim_refresh.py new file mode 100644 index 00000000..49ef8a1f --- /dev/null +++ b/tests/test_coord_claim_refresh.py @@ -0,0 +1,190 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Re-taking a claim you already hold must actually refresh its note. + +``scripts/coord/claim.ps1 -Take`` documented itself as idempotent -- "re-taking your own claim just +refreshes the note" -- and did not do it. A new ``-Note`` was accepted, reported as success, and +silently discarded. + +That is worse than a plain failure. The note is the one field written deliberately to say what a +session is doing, and ``announce-session.ps1`` broadcasts it to every session joining the repo while +telling them to prefer it over the worktree name. Measured 2026-08-02: the ``announce-hook`` claim was +still announcing "NO PR OPENED -- honouring the #119 merge freeze" to every joining session hours +after both PRs had merged. A stale note presented as current intent is a coordination fault. + +The workaround people reached for -- ``-Release`` then ``-Take`` -- drops the claim in between, which +re-opens the very race the claim exists to close. So the property under test is *refresh in place*: +the note changes and ``claimed`` does not, which is what proves the claim was never let go. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +CLAIM = ROOT / "scripts" / "coord" / "claim.ps1" +TIMEOUT = 60 + +pytestmark = pytest.mark.skipif( + shutil.which("pwsh") is None or os.name != "nt", + reason="claim.ps1 needs pwsh on Windows", +) + + +def git(repo: Path, *args: str) -> str: + proc = subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True, timeout=TIMEOUT, check=True + ) + return proc.stdout + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + r = tmp_path / "repo" + r.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main", str(r)], check=True, capture_output=True) + git(r, "config", "user.email", "t@example.invalid") + git(r, "config", "user.name", "t") + (r / "f.txt").write_text("x", encoding="utf-8") + git(r, "add", "-A") + git(r, "commit", "-qm", "base") + return r + + +def claim(cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: + """Run the real script from ``cwd`` -- it scopes itself to the cwd's repo, and has no -Repo.""" + return subprocess.run( + ["pwsh", "-NoProfile", "-NonInteractive", "-File", str(CLAIM), *args], + cwd=str(cwd), + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + + +def claim_file(repo: Path, key: str) -> Path: + common = git(repo, "rev-parse", "--path-format=absolute", "--git-common-dir").strip() + return Path(common) / "mefor-coord" / "claims" / f"{key}.json" + + +def read_claim(repo: Path, key: str) -> dict[str, Any]: + raw = claim_file(repo, key).read_bytes() + # NO BOM. scripts/hooks/claim_check.py reads this with encoding="utf-8" and a BOM makes json.loads + # raise -- which that gate swallows into "not claimed", silently disabling itself for this key. + assert not raw.startswith(b"\xef\xbb\xbf"), "claim file must be UTF-8 without a BOM" + parsed: dict[str, Any] = json.loads(raw.decode("utf-8")) + return parsed + + +def test_retaking_your_own_claim_replaces_the_note(repo: Path) -> None: + assert claim(repo, "-Take", "k", "-Note", "first").returncode == 0 + before = read_claim(repo, "k") + + proc = claim(repo, "-Take", "k", "-Note", "second -- the PR has merged") + assert proc.returncode == 0, proc.stderr + + after = read_claim(repo, "k") + assert after["note"] == "second -- the PR has merged" + assert "REFRESHED" in proc.stdout, "it must not report success without saying what it did" + # THE CLAIM WAS NEVER RELEASED. `claimed` is the claim's identity; if the refresh had gone through + # release-then-retake, this would have moved -- and there would have been a window in between with + # the key free for another session to take. + # Byte-identical, not merely "still parses". ConvertFrom-Json coerces an ISO-8601 string into a + # [datetime], so a naive rewrite emits the local short form -- lower precision, no UTC offset, and + # culture-dependent on the way back in. It would still parse, so nothing would ever complain. + assert after["claimed"] == before["claimed"] + assert "T" in str(before["claimed"]), "the stamp under test must be the ISO-8601 form" + assert after["refreshed"], "a refreshed note needs its own stamp, or readers cannot date it" + + +def test_a_fresh_claim_has_no_refreshed_stamp(repo: Path) -> None: + """Absent, not zero. Readers fall back to ``claimed``; a fabricated stamp would date it wrong.""" + assert claim(repo, "-Take", "k", "-Note", "first").returncode == 0 + assert "refreshed" not in read_claim(repo, "k") + + +def test_retaking_without_a_note_leaves_the_note_alone(repo: Path) -> None: + """Re-asserting a claim is a normal thing to do and must not blank out what it says.""" + assert claim(repo, "-Take", "k", "-Note", "the real work").returncode == 0 + proc = claim(repo, "-Take", "k") + assert proc.returncode == 0 + assert read_claim(repo, "k")["note"] == "the real work" + assert "the real work" in proc.stdout, "show the note, so a session can see it is stale" + + +def test_the_refresh_updates_the_branch_too(repo: Path) -> None: + """A claim naming a branch nobody is on is another confidently-wrong coordination fact.""" + assert claim(repo, "-Take", "k", "-Note", "n").returncode == 0 + git(repo, "checkout", "-q", "-b", "moved-on") + assert claim(repo, "-Take", "k", "-Note", "n2").returncode == 0 + assert read_claim(repo, "k")["branch"] == "moved-on" + + +def test_a_peers_claim_is_still_refused(repo: Path, tmp_path: Path) -> None: + """THE REGRESSION GUARD. The refresh path must not become a way to rewrite someone else's note. + + Mutual exclusion is the whole point of this script; a fix for a stale note that let any session + overwrite any other session's note would trade a stale fact for a forged one. + """ + peer = tmp_path / "peer-wt" + git(repo, "worktree", "add", "-q", "-b", "peer-branch", str(peer)) + assert claim(peer, "-Take", "k", "-Note", "the peer's work").returncode == 0 + + proc = claim(repo, "-Take", "k", "-Note", "mine now") + assert proc.returncode == 1, "taking a peer's key must fail" + assert "BLOCKED" in proc.stdout + assert read_claim(repo, "k")["note"] == "the peer's work", ( + "a refused take must not have written" + ) + + +def test_a_key_that_looks_like_a_date_survives_a_refresh(repo: Path) -> None: + """A key is free text, and ConvertFrom-Json date-coerces anything ISO-8601 shaped. + + Round-tripping the stored key through [string] would rewrite "2026-08-01T00:00:00" as + "08/01/2026 00:00:00" -- a record naming a key nobody typed, which -List reports and -Release + cannot be spelled to match. + """ + key = "2026-08-01T00:00:00" + assert claim(repo, "-Take", key, "-Note", "first").returncode == 0 + assert claim(repo, "-Take", key, "-Note", "second").returncode == 0 + assert read_claim(repo, "2026-08-01t00-00-00")["key"] == key + + +def test_a_refresh_that_cannot_be_written_leaves_the_old_claim_intact(repo: Path) -> None: + """Write-then-REPLACE, not truncate-in-place and not delete-then-rename. + + Two properties, and the second is the one that bites hardest: + + * A claim file caught mid-write does not fail loudly -- ``claim_check.py`` swallows a parse error + into "not claimed" and stops enforcing that key. A crash during a refresh must leave the OLD note. + * **The claim file's existence IS the lock.** The take path is an exclusive ``CreateNew``, so any + instant the name does not exist is an instant another worktree can claim a key we hold. + ``Move-Item -Force`` is delete-then-rename: measured on this box, 400 moves left the destination + absent on 2,559 of 154,506 polls. ``[IO.File]::Move(.., overwrite)`` is MoveFileEx and never + unlinked the name across 134,581 polls -- it fails transiently instead, which is the safe + direction: the old note survives and the claim stays ours. + """ + assert claim(repo, "-Take", "k", "-Note", "first").returncode == 0 + path = claim_file(repo, "k") + # Hold the file open without sharing Delete: the replace cannot land, and the old bytes must survive. + with open(path, "r+b") as held: + held.read() + proc = claim(repo, "-Take", "k", "-Note", "second") + # The NAME must never have gone away, even while the refresh was failing. + assert path.exists(), ( + "the claim file was unlinked -- a peer could take the key in that window" + ) + assert proc.returncode != 0 or read_claim(repo, "k")["note"] in {"first", "second"} + # Whatever happened, the file is still parseable -- that is the property the gate depends on. + assert read_claim(repo, "k")["key"] == "k" + # And nothing was orphaned: this directory is the claim registry, not a scratch space. + assert not list(path.parent.glob("*.tmp")), "a failed refresh left a temp file behind" diff --git a/tests/test_coord_overlap_attribution.py b/tests/test_coord_overlap_attribution.py new file mode 100644 index 00000000..397927f2 --- /dev/null +++ b/tests/test_coord_overlap_attribution.py @@ -0,0 +1,252 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Which worktree does ``scripts/coord/overlap.ps1`` say a session is sitting in? + +A session is attributed to a worktree by its cwd. The rule has to survive the layout this project +actually uses: linked worktrees live **inside** the primary checkout, at +``/.claude/worktrees/``. So every linked worktree's path is *also* a prefix match for +the primary's own row, and "the worktree containing this cwd" has two correct-looking answers. + +The old rule walked the session table and took the first prefix hit, which handed the primary row +whichever linked-worktree session the hashtable happened to enumerate first. The primary checkout was +then reported LIVE, on a branch nobody was on, "building" a peer's task list -- and because hashtable +order is not stable, it was a *different* wrong answer run to run, which is why it read as noise +rather than as a bug. + +Driven against a REAL nested worktree, because nesting is the entire mechanism. A fixture with two +sibling directories would pass under both the old rule and the new one and prove nothing. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import sys +import time +from pathlib import Path +from typing import Any + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +OVERLAP = ROOT / "scripts" / "coord" / "overlap.ps1" +TIMEOUT = 60 +NOW_MS = int(time.time() * 1000) + +pytestmark = pytest.mark.skipif( + shutil.which("pwsh") is None or os.name != "nt", + reason="overlap.ps1 needs pwsh on Windows", +) + + +def git(repo: Path, *args: str) -> str: + proc = subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True, timeout=TIMEOUT, check=True + ) + return proc.stdout + + +@pytest.fixture +def nested_layout(tmp_path: Path) -> tuple[Path, Path, Path]: + """The real shape: a primary, a linked worktree NESTED under it, and an outside vantage point. + + ``.claude/`` is gitignored and the ignore is pushed, so the primary sits exactly on origin/main + with a clean tree -- otherwise the nested worktree's own files would show up as the primary's + untracked changes and give it a row on their own merits, masking the attribution question. + """ + origin = tmp_path / "origin.git" + subprocess.run( + ["git", "init", "-q", "--bare", "-b", "main", str(origin)], check=True, capture_output=True + ) + primary = tmp_path / "primary" + primary.mkdir() + subprocess.run( + ["git", "init", "-q", "-b", "main", str(primary)], check=True, capture_output=True + ) + git(primary, "config", "user.email", "t@example.invalid") + git(primary, "config", "user.name", "t") + (primary / ".gitignore").write_text(".claude/\n", encoding="utf-8") + (primary / "alpha.txt").write_text("base\n", encoding="utf-8") + git(primary, "add", "-A") + git(primary, "commit", "-qm", "base") + git(primary, "remote", "add", "origin", str(origin)) + git(primary, "push", "-q", "origin", "main") + + nested = primary / ".claude" / "worktrees" / "peer" + git(primary, "worktree", "add", "-q", "-b", "peer-branch", str(nested)) + vantage = tmp_path / "vantage" + git(primary, "worktree", "add", "-q", "-b", "vantage-branch", str(vantage)) + return primary, nested, vantage + + +@pytest.fixture +def config_root(tmp_path: Path) -> Path: + root = tmp_path / ".claude-config" + (root / "sessions").mkdir(parents=True) + return root + + +def write_session( + config_root: Path, *, pid: int, cwd: Path, session_id: str, started_at: int = NOW_MS +) -> None: + """A live session record. A record stamped alongside its process's start fences as LIVE. + + ``started_at`` defaults to import time, which is the right stamp for *this* process; a spawned + child needs its own, because the fence rejects a record that precedes its process by too much. + """ + (config_root / "sessions" / f"{pid}.json").write_text( + json.dumps( + { + "pid": pid, + "sessionId": session_id, + "cwd": str(cwd), + "startedAt": started_at, + "version": "2.1.219", + "peerProtocol": 1, + "kind": "interactive", + "entrypoint": "claude-vscode", + "name": session_id[:8], + "nameSource": "derived", + } + ), + encoding="utf-8", + ) + + +def survey(vantage: Path, config_root: Path, tmp_path: Path) -> list[dict[str, Any]]: + """The whole map, from a worktree that is neither the primary nor the one under test.""" + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(OVERLAP), + "-Repo", + str(vantage), + "-Json", + "-Refresh", + "-ConfigRoot", + str(config_root), + "-TasksDir", + str(tmp_path / "no-such-tasks"), + ], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + assert proc.returncode == 0, f"overlap exited {proc.returncode}: {proc.stderr}" + out = proc.stdout.strip() + parsed: list[dict[str, Any]] = json.loads(out) if out else [] + return parsed + + +def row_for(rows: list[dict[str, Any]], path: Path) -> dict[str, Any] | None: + want = str(path).replace("\\", "/").rstrip("/").lower() + for r in rows: + if str(r.get("Path", "")).replace("\\", "/").rstrip("/").lower() == want: + return r + return None + + +def test_a_nested_worktrees_session_is_attributed_to_it_not_the_primary( + nested_layout: tuple[Path, Path, Path], config_root: Path, tmp_path: Path +) -> None: + """THE BUG. One session, sitting in a worktree nested under the primary checkout. + + Its cwd is a prefix match for both. Before the fix the primary could win that race and be reported + as a live session on ``main`` -- a collision signal naming the wrong worktree, wrong branch and + wrong work. + """ + primary, nested, vantage = nested_layout + (nested / "alpha.txt").write_text("base\npeer edit\n", encoding="utf-8") + write_session(config_root, pid=os.getpid(), cwd=nested, session_id="aaaaaaaa-1111") + + rows = survey(vantage, config_root, tmp_path) + + peer = row_for(rows, nested) + assert peer is not None, "the nested worktree must be reported" + assert peer["Live"] is True, "the session is sitting in it" + assert peer["Short"] == "aaaaaaaa" + assert peer["Branch"] == "peer-branch" + + got = row_for(rows, primary) + # The primary is clean and holds no session, so the correct answer is no row at all. Before the + # fix it appeared, Live, wearing this session's id. + assert got is None or got["Live"] is False, f"the primary is not where that session is: {got}" + + +def test_the_primary_still_owns_a_session_whose_cwd_is_actually_in_it( + nested_layout: tuple[Path, Path, Path], config_root: Path, tmp_path: Path +) -> None: + """Longest-prefix must not become never-the-primary. + + A session genuinely sitting in the primary checkout -- including one in a subdirectory of it that + is not a worktree -- still belongs to the primary. This is the over-correction that would swap one + silent mis-attribution for another. + """ + primary, _nested, vantage = nested_layout + subdir = primary / "messagefoundry" + subdir.mkdir() + (primary / "alpha.txt").write_text("base\nprimary edit\n", encoding="utf-8") + write_session(config_root, pid=os.getpid(), cwd=subdir, session_id="bbbbbbbb-2222") + + rows = survey(vantage, config_root, tmp_path) + + got = row_for(rows, primary) + assert got is not None, "a session in the primary's own subdirectory must attribute to it" + assert got["Live"] is True + assert got["Short"] == "bbbbbbbb" + + +def test_two_nested_sessions_each_land_in_their_own_worktree( + nested_layout: tuple[Path, Path, Path], config_root: Path, tmp_path: Path +) -> None: + """The ordering hazard, with enough sessions present for order to matter. + + First-hit-wins is only *visibly* arbitrary once more than one session is a candidate. Two sessions + in two nested worktrees: each must land in its own, the primary must claim neither, and the answer + must be the same every run -- the old rule's depended on hashtable enumeration order, so an + unstable wrong answer read as noise and nobody filed it. + """ + primary, nested, vantage = nested_layout + other = primary / ".claude" / "worktrees" / "peer2" + git(primary, "worktree", "add", "-q", "-b", "peer2-branch", str(other)) + (nested / "alpha.txt").write_text("base\npeer edit\n", encoding="utf-8") + (other / "alpha.txt").write_text("base\npeer2 edit\n", encoding="utf-8") + + write_session(config_root, pid=os.getpid(), cwd=nested, session_id="cccccccc-3333") + child = subprocess.Popen( # a second REAL live pid; the fence needs a process, not a number + [sys.executable, "-c", "import time; time.sleep(120)"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + try: + write_session( + config_root, + pid=child.pid, + cwd=other, + session_id="dddddddd-4444", + started_at=int(time.time() * 1000), + ) + answers = [ + json.dumps( + sorted((str(r["Path"]).lower(), bool(r["Live"]), str(r["Short"])) for r in rows) + ) + for rows in (survey(vantage, config_root, tmp_path) for _ in range(3)) + ] + finally: + child.kill() + child.wait(timeout=10) + + assert len(set(answers)) == 1, f"overlap gave more than one answer for one state: {answers}" + + rows = json.loads(answers[0]) + by_path = {p: (live, short) for p, live, short in rows} + assert by_path.get(str(nested).replace("\\", "/").lower()) == (True, "cccccccc") + assert by_path.get(str(other).replace("\\", "/").lower()) == (True, "dddddddd") + got = by_path.get(str(primary).replace("\\", "/").lower()) + assert got is None or got[0] is False, f"the primary owns neither session: {got}" diff --git a/tests/test_coord_overlap_signals.py b/tests/test_coord_overlap_signals.py index 98cdf298..158b7409 100644 --- a/tests/test_coord_overlap_signals.py +++ b/tests/test_coord_overlap_signals.py @@ -162,6 +162,103 @@ def test_an_untouched_file_is_not_reported( assert query(primary, tmp_path, "beta.txt") == [] +def raw_query(primary: Path, tmp_path: Path, path: str) -> str: + """The same query, but returning stdout VERBATIM -- ``query`` above maps '' to [] and would hide + the very difference under test.""" + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(OVERLAP), + "-Repo", + str(primary), + "-File", + path, + "-Json", + "-Refresh", + "-ConfigRoot", + str(tmp_path / "no-such-config"), + "-TasksDir", + str(tmp_path / "no-such-tasks"), + ], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + assert proc.returncode == 0, f"overlap exited {proc.returncode}: {proc.stderr}" + return proc.stdout.strip() + + +def raw_survey(primary: Path, tmp_path: Path) -> str: + """The WHOLE-MAP -Json query, verbatim. A different call site from ``raw_query``, and it fails + differently: this one is fed ``Build-Map``'s return value, which is AutomationNull when empty.""" + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(OVERLAP), + "-Repo", + str(primary), + "-Json", + "-Refresh", + "-ConfigRoot", + str(tmp_path / "no-such-config"), + "-TasksDir", + str(tmp_path / "no-such-tasks"), + ], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + assert proc.returncode == 0, f"overlap exited {proc.returncode}: {proc.stderr}" + return proc.stdout.strip() + + +def test_the_whole_map_json_query_emits_no_phantom_row( + peer_worktree: tuple[Path, Path], tmp_path: Path +) -> None: + """A ZERO-ROW ANSWER MUST BE ZERO ROWS, not one null one. + + ``Build-Map`` returns AutomationNull when it has no rows, and parameter binding converts that to a + real ``$null`` at the call -- where ``@($null).Count`` is 1, not 0. So a naive "if empty print []" + guard is dead in precisely the case it exists for, and the query emits ``[null]``: a phantom row, + strictly worse for any consumer that counts or indexes than the nothing it replaced. + """ + primary, _peer = ( + peer_worktree # peer worktree exists but is clean, and no session is registered + ) + out = raw_survey(primary, tmp_path) + assert out == "[]", f"a no-rows map must be an empty array, not {out!r}" + assert json.loads(out) == [] + + +def test_a_json_query_always_emits_an_array( + peer_worktree: tuple[Path, Path], tmp_path: Path +) -> None: + """AN ANSWER OF "NOBODY" MUST NOT LOOK LIKE NO ANSWER. + + ``@() | ConvertTo-Json -AsArray`` sends zero objects down the pipeline, so ConvertTo-Json never runs + and the script printed NOTHING -- ``-AsArray`` only shapes output that exists. On stdout that is + byte-for-byte what a script dying before it answers looks like, so no consumer could separate an + all-clear from a failure. The collision gate consumes this on every Edit and Write; measured + 2026-08-02, the gate reported "could not check" on an ordinary edit to an untouched file because of + exactly this. + """ + primary, peer = peer_worktree + (peer / "alpha.txt").write_text("base\nunsaved\n", encoding="utf-8") + + assert raw_query(primary, tmp_path, "beta.txt") == "[]", "a no-hit query must still answer" + hit = raw_query(primary, tmp_path, "alpha.txt") + assert hit.startswith("["), f"a hit must be an array too, not a bare object: {hit[:80]}" + assert json.loads(hit), "and it must parse with at least one row" + + def test_overlap_does_not_rewrite_a_peers_git_index( peer_worktree: tuple[Path, Path], tmp_path: Path ) -> None: