From 14defcb0d1986a962f99f64f4fe8d07f159a3c47 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 18 Aug 2026 21:31:14 -0500 Subject: [PATCH] fix(coord): keep a zero-row overlap walk zero rows across the cache Found while auditing scripts/ for other instances of the unroll defect fixed in the previous commit. Same mechanism, different script, and it survives a round trip to disk where the other one did not. Build-Map returns an empty array, PowerShell unrolls it to AutomationNull, and in process that is harmless: assignment preserves it and the zero-rows guard still fires. It does not survive serialisation. ConvertTo-Json writes "rows": null, the next run reads it back with @($c.rows), and that is a one-element array holding $null. Count is 1, the guard does not fire, and the human report prints a phantom occupant: a blank worktree on a blank branch with 0 changed files. The hot path arms this constantly rather than rarely. collision_gate.ps1 runs overlap.ps1 with -Json on every gated edit, and the cache is written before the -File early exit, so any bare overlap.ps1 inside the cache window inherits it. An invented collision is not a safe direction to fail in. It is the answer people work around, and a report that cries wolf on a quiet repo is one they stop reading, which is how a real collision goes unnoticed. Kept out of the install-gate commit deliberately: different file, different failure mode, and it needed its own test rather than riding in on one earned elsewhere. The test drives real throwaway git repos, because the whole question is what the script writes to disk and reads back; stub rows would assert only that a value someone else computed gets carried, and the value is exactly what was wrong. --- scripts/coord/overlap.ps1 | 9 +- tests/test_coord_overlap_cache.py | 189 ++++++++++++++++++++++++++++++ tests/tooling_manifest.txt | 1 + 3 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 tests/test_coord_overlap_cache.py diff --git a/scripts/coord/overlap.ps1 b/scripts/coord/overlap.ps1 index b79cc9f9..7e9a4a81 100644 --- a/scripts/coord/overlap.ps1 +++ b/scripts/coord/overlap.ps1 @@ -260,7 +260,14 @@ if (-not $Refresh -and (Test-Path -LiteralPath $cacheFile)) { } catch { $map = $null } } if ($null -eq $map) { - $map = Build-Map + # @() for the SAME reason Write-JsonArray filters nulls, at the other end of the same round trip. + # A bare `$map = Build-Map` survives in process -- assignment preserves AutomationNull, so the + # zero-rows guard below still fires -- but it serializes into the cache as `"rows": null`, and the + # next run reads that back as `@($null)`, a ONE-element array holding $null. Count is then 1, the + # guard does not fire, and the table prints a phantom occupant. The hook arms this constantly: + # collision_gate.ps1 runs `overlap.ps1 -File ... -Json` on every gated edit and the cache is written + # before the -File early exit, so any bare `overlap.ps1` inside $CacheSeconds inherits it. + $map = @(Build-Map) try { New-Item -ItemType Directory -Force -Path (Split-Path $cacheFile) | Out-Null # Last-write-wins on purpose: a duplicate walk is the only cost of a race, and a lock on the diff --git a/tests/test_coord_overlap_cache.py b/tests/test_coord_overlap_cache.py new file mode 100644 index 00000000..7375b305 --- /dev/null +++ b/tests/test_coord_overlap_cache.py @@ -0,0 +1,189 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The overlap cache must round-trip ZERO rows as zero rows. + +``scripts/coord/overlap.ps1`` writes its walk to a per-repo cache and every later run inside +``-CacheSeconds`` reads it back instead of re-walking. The empty answer does not survive that trip by +itself. ``Build-Map`` ends in ``return $rows`` over ``$rows = @()``, and PowerShell UNROLLS an empty +array on the way out, so a zero-row walk yields AutomationNull. In process that is harmless -- the +zero-rows guard still fires -- but ``ConvertTo-Json`` serialises it as ``"rows": null``, and the next +run reads that back with ``@($c.rows)``, which is a ONE-element array holding ``$null``. ``Count`` is +then 1, the guard does not fire, and the human report prints a PHANTOM OCCUPANT: a blank worktree +name on a blank branch with "0 changed file(s)". + +This is the same class of defect as the ``Write-JsonArray`` null filter above it in the same script, +at the other end of the same round trip, and the same class as the ``Get-WiredMatchers`` unroll in +``scripts/worktree/install-gate.ps1`` -- an enumerable returned from a PowerShell function is not the +value the author wrote. + +WHY IT MATTERS RATHER THAN MERELY BEING UNTIDY. ``collision_gate.ps1`` runs ``overlap.ps1 -File ... +-Json`` on every gated edit, and the cache is written BEFORE the ``-File`` early exit. So the hot path +arms the cache constantly, and any bare ``overlap.ps1`` run inside the window inherits it: a session +asking "who else is in this repo" is answered with an occupant that does not exist. An invented +collision is not a safe direction to fail in -- it is the answer people work around, and a report that +cries wolf on a quiet repo is one they stop reading, which is how a REAL collision goes unnoticed. + +Driven against real throwaway git repos, because the question is entirely about what the script writes +to disk and reads back. A test over stub rows would assert only that a value someone else computed +gets carried, and the value is exactly what is wrong here. +""" + +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] +OVERLAP = ROOT / "scripts" / "coord" / "overlap.ps1" +TIMEOUT = 60 + +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 + + +def _repo(root: Path, name: str) -> Path: + """A repo with an ``origin/main`` to diff against, and nothing else in it.""" + origin = root / f"{name}.git" + subprocess.run( + ["git", "init", "-q", "--bare", "-b", "main", str(origin)], check=True, capture_output=True + ) + primary = root / name + 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 / "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") + return primary + + +def _cache_path(repo: Path) -> Path: + common = _git(repo, "rev-parse", "--path-format=absolute", "--git-common-dir").strip() + return Path(common) / "mefor-coord" / "overlap-cache.json" + + +def _run(repo: Path, sandbox: Path, *args: str) -> subprocess.CompletedProcess[str]: + """Drive overlap.ps1 against one throwaway repo, with every machine-global input redirected. + + ``-ConfigRoot`` and ``-TasksDir`` point at paths that do not exist on purpose: without them the + script reads this developer's REAL session registry and task lists, and the answer would depend on + who else is logged in. They cannot contribute a row here anyway (only worktrees of this repo are + walked), but a fixture whose result varies with the machine is not a fixture. + """ + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(OVERLAP), + "-Repo", + str(repo), + "-ConfigRoot", + str(sandbox / "no-such-config"), + "-TasksDir", + str(sandbox / "no-such-tasks"), + *args, + ], + capture_output=True, + text=True, + timeout=TIMEOUT, + check=False, + ) + assert proc.returncode == 0, f"overlap exited {proc.returncode}: {proc.stderr}\n{proc.stdout}" + return proc + + +def _cached_rows(repo: Path) -> Any: + """The ``rows`` member exactly as it was serialised -- deliberately NOT normalised. + + ``json.loads`` renders ``"rows": null`` as ``None`` and ``"rows": []`` as ``[]``, and the whole + defect is that PowerShell then treats those two as the same length. Coercing them here (``or []``, + ``len(rows or [])``) would reproduce the bug inside the instrument and the test would pass against + the defect it exists to pin. + """ + return json.loads(_cache_path(repo).read_text(encoding="utf-8"))["rows"] + + +def test_a_walk_that_finds_nobody_caches_an_empty_array_not_null(tmp_path: Path) -> None: + """Zero rows must reach the cache as ``[]``, and come back out as zero rows. + + The positive control runs FIRST and against the same machinery: a repo that DOES have an occupant + must cache a non-empty ``rows``. Without it, an assertion that ``rows == []`` is satisfied just as + well by a script that never writes a row under any circumstances, or by a cache path this fixture + computed wrongly -- and a null result printed by a broken instrument is indistinguishable from a + null result printed by a working one. + """ + # --- POSITIVE CONTROL: an occupied repo, so we know rows can be non-empty at all --------------- + occupied = _repo(tmp_path, "occupied") + peer = tmp_path / "occupied-peer" + _git(occupied, "worktree", "add", "-q", "-b", "peer-branch", str(peer)) + (peer / "alpha.txt").write_text("peer edit\n", encoding="utf-8") + + _run(occupied, tmp_path, "-Refresh", "-Json") + occupied_rows = _cached_rows(occupied) + # Deliberately shape-AGNOSTIC: `ConvertTo-Json` renders a single row as a bare object rather than + # a one-element array, and that shape is harmless -- `@($c.rows)` wraps it back into one row on + # the way in. Asserting a list here would make the control the first thing to fail under the + # defect below, which would report "the fixture is broken" for a fixture that is working. + assert occupied_rows not in (None, []), ( + "the control repo has a peer worktree with a dirty file, so the cache must carry a row. It " + f"does not, so this fixture cannot tell an empty cache from a broken one: {occupied_rows!r}" + ) + + # --- THE CASE: a repo with no peer worktree and nothing dirty ---------------------------------- + quiet = _repo(tmp_path, "quiet") + first = _run(quiet, tmp_path, "-Refresh", "-Json") + assert first.stdout.strip() == "[]", ( + f"the walk itself must find nobody in this repo, or the rest of this test is moot: " + f"{first.stdout!r}" + ) + + rows = _cached_rows(quiet) + assert rows == [], ( + "a walk that found nobody was serialised into the cache as something other than an empty " + "array. `null` here is the unroll: `return $rows` over an empty array yields AutomationNull, " + "and the next run reads it back as a one-element array holding $null, so the zero-rows guard " + f"does not fire and a phantom occupant is printed. Got: {rows!r}" + ) + + # --- and the read-back half: the cached answer must still be "nobody" -------------------------- + before = _cache_path(quiet).read_bytes() + second = _run(quiet, tmp_path, "-CacheSeconds", "3600") + after = _cache_path(quiet).read_bytes() + + # Guard the guard: prove this run was a cache HIT. A miss re-walks and REWRITES the cache with a + # fresh `at` stamp, so unchanged bytes are the evidence that the cached value is what was read -- + # otherwise a passing assertion below would only mean the second walk also found nobody, which + # says nothing about the round trip. + assert after == before, ( + "the second run rewrote the cache, so it re-walked instead of reading what the first run " + "stored -- this assertion would then be testing the walk twice and the cache never." + ) + assert "No other worktree has changes." in second.stdout, ( + "reading the cached empty walk back produced an occupant. Under the unroll the cache holds " + f'`"rows": null`, which `@($c.rows)` turns into one $null row.\n{second.stdout!r}' + ) + assert "changed file(s)" not in second.stdout, ( + f"a phantom row was printed from the cache:\n{second.stdout!r}" + ) diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index 9105f5cc..f0a85789 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -50,6 +50,7 @@ tests/test_coord_claim_refresh.py tests/test_coord_claim_release_history.py tests/test_coord_lock.py tests/test_coord_overlap_attribution.py +tests/test_coord_overlap_cache.py tests/test_coord_overlap_signals.py tests/test_coord_presence.py tests/test_coord_seat_prompt.py