diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 987a97699..cd507e5ec 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -21997,3 +21997,71 @@ So `test_the_script_prefers_its_own_repo_over_an_earlier_path_entry` supplies th **Verification:** 8 passed in `tests/test_webconsole_seam_snapshot.py`. Mutation check run rather than argued -- with the `sys.path.insert` line deleted, the decoy test reds naming the decoy import, and the by-path digest test **stays green**, which is the luck described above measured rather than predicted. Anchor restored, 8 passed again. **Adjacent and NOT fixed here, named rather than numbered.** `docs/WEBCONSOLE-PACKAGE.md`'s seam-refresh procedure is stale in three steps left behind by #1220: it says to bump `ENGINE_UI_SEAM` by hand (`1` to `2`) when the value is a derived digest, it says to update curated lists in this script that #1220 retired, and its step 5 prescribes `python scripts/webconsole_seam_snapshot.py > tests/golden/...`, the shell redirect this script's own docstring forbids because PowerShell's `>` writes UTF-16LE with a BOM into a file the test reads as UTF-8. That is doc drift with its own cause and it wants its own item; folding a documentation rewrite into a `sys.path` fix would make both harder to review. + +## 1434. the tooling-partition gate names the file it rejects and not the remedy, so the Builder who trips it has already exited + +> 🚧 **Filed 2026-09-03. The failure message is fixed on this branch. Five open pull requests still carry unregistered test files, and only their own branches can fix them -- they are named below.** `tests/test_tooling_partition.py::test_every_non_engine_test_is_classified` fails when a file matching `tests/test_*.py` sits in neither `tests/tooling_manifest.txt` nor `_STAYS_WITHOUT_IMPORTING`. It is not deselected by `-m 'not tooling'`, so it reds **all three required `test` legs at once**. PR 774 hit exactly that, and its Builder's process had exited before any leg reported. + +**Cluster:** CI gates / development harness. **Priority:** P2. **Verdict:** build. +**Severity:** no deployment axis (sec. 0). This is a repository gate over `tests/`. No engine behaviour, no shipped artifact, and no configuration a deploying site would meet. + +### The defect: the message stops one sentence before the remedy + +The predecessor read, in full: + +> these tests import no engine module, so they are unclassified: add them to tests/tooling_manifest.txt, or to _STAYS_WITHOUT_IMPORTING here if they read engine source: ['test_coord_fleet_fetch_clock.py'] + +It names both registries, and that is where it stops. Three things a reader needs are absent: + +1. **The entry shape.** A manifest line must be the repo-relative path `tests/.py`. Both readers of the manifest (`_manifest_names` here, `_tooling_basenames` in `tests/conftest.py`) `rsplit` on `/`, so a bare `.py` still marks the test and still takes it off the engine legs -- but `ci.yml`'s `changes` job summons the `tooling` job with `grep -qxFf` over **whole changed paths**, so a later pull request editing only that test would match no arm and summon no job. Deselected everywhere, green. +2. **Which list to pick, and the tie-break.** The choice is by the test's **subject**, not by its location or its name. This file's own header already carries the rule -- *"when ambiguous, LEAVE IT OFF"* -- and the message never said so. A hurried reader appends to the manifest because it is a one-line edit, which is the direction that loses coverage silently. +3. **That an engine-importing test needs no entry at all.** 720 files match `tests/test_*.py` and 163 are registered. The other 557 import the engine and are classified by that alone. + +### Why the message is the whole remedy here + +A Builder gets one turn and its process exits when its pull request opens (CLAUDE.md section 5). Both Builders who tripped this gate on 2026-09-03 had exited before any leg reported, so neither could read its own failure. The person who eventually reads it is someone else, in a CI log, with no session to ask. **The assertion string is the only channel that reaches them.** + +### The sweep, 2026-09-03, over all 51 open pull requests + +Instrument: each pull request's own head ref evaluated against **its own** copy of the manifest and stay list, so a pull request that registers its file passes. Verified against the control pair the Console measured, which is what tells this apart from "is the file in the manifest": + +| control | file | in manifest | legs | why | +|---|---|---|---|---| +| PR 774 (negative) | `tests/test_coord_fleet_fetch_clock.py` | no | 3 red | subject is `scripts/coord/fleet.ps1`, harness, so it must be listed | +| PR 769 (positive) | `tests/test_container_health_probe.py` | no | 3 green | imports `messagefoundry.config.settings`, so absence is correct | + +**Absence is not the defect.** Eleven open pull requests add a file under `tests/`. Five will red three required legs; every one of the five has a harness subject and belongs in the manifest: + +| PR | branch | file | subject | +|---|---|---|---| +| 774 | `worktree-agent-a1ba06ee275587442` | `test_coord_fleet_fetch_clock.py` | `scripts/coord/fleet.ps1` | +| 789 | `claude/1385-ci-red-reader-and-timeout-flake` | `test_ci_red_reader.py` | `scripts/ci/report_ci_red.py`, `.github/workflows/failure-signal.yml` | +| 795 | `b-1375-install-gate-allowlist-merge` | `test_install_gate_allowlist_merge.py` | `scripts/worktree/install-gate.ps1` | +| 798 | `worktree-agent-a9051692500267199` | `test_claim_shared_registry.py` | `scripts/coord/claim.ps1`, `scripts/hooks/claim_check.py` | +| 799 | `worktree-agent-a2baeeba559ec2796` | `test_steer_inject.py` | `scripts/hooks/steer-inject.ps1` | + +The remaining six are already correct: PRs 764, 769, 773 and 796 import the engine; PRs 792 and 806 each appended their `tests/.py` line to the manifest. + +### The silent direction was checked too, and it is clean + +An engine test wrongly **in** the manifest leaves no trace: it is deselected from the engine legs, and the `tooling` job's `scripts/**` path gate is not tripped by an engine diff. `test_no_listed_test_imports_the_engine` catches the importing case; nothing catches a listed test that **reads** engine source off disk, which is the class the header records four files being caught in by review rather than by a guard. + +All 137 listed entries were swept for a repo-rooted read of `messagefoundry/**`. **One hit, and it is correctly listed.** `tests/test_serverdb_ci_coverage.py:276` runs `(REPO_ROOT / "messagefoundry").rglob("*.py")`, but only to prove `ci.yml`'s `serverdb` regex still **refuses** something. Its assertion is invariant to the content of every file it reads, so its subject is the workflow. + +**The discriminator, written down so the next sweep does not re-derive it: can an engine-only diff change this test's result?** If yes, it is engine-subject whatever it imports. That is what separates this file from `test_install_instruction_provenance.py`, which globs the same tree and asserts on what it finds, and is stay-listed for that reason. + +### What this change ships + +- `_unclassified_remedy` renders the failure. Line one keeps both registry paths and the file list, because that is the line a truncated summary keeps. The body gives the exact line to paste, states the criterion as **subject**, and gives the tie-break with its cost. Both registry paths are rendered from `_MANIFEST` and `__file__`, so a rename moves the message rather than leaving it pointing at a path that is gone. +- `test_every_manifest_entry_resolves_as_written` replaces a basename check. It resolves each entry **as written** from the repository root and requires it to equal the path `git diff --name-only` reports, which is what `ci.yml`'s `grep -qxFf` gate matches. The predecessor rsplit the directory away and was blind to a bare name, a `./` prefix or a wrong directory. +- `test_the_gate_offers_a_line_the_manifest_parser_accepts` takes the example off the **raised** message and feeds it through the manifest parser, so the advice cannot drift from either the assertion that issues it or the reader that consumes it. +- `test_the_gate_raises_the_remedy_and_not_a_bare_list` pulls every `tests/...` path out of the raised message and requires each to name a real file, then checks `_STAYS_WITHOUT_IMPORTING` is defined in the file the message points at. Only the three prose claims are matched as literals. + +**Asserting on `_unclassified_remedy`'s return value was tried first and measured vacuous.** Reverting the assertion's call site to the predecessor string left every content arm green, because none of them touched the call site. The arms now read the message the gate actually raises. That is SDS-3.8: the instrument was answering *does the helper return good text*, and the question is *does the gate fail with good text*. + +### What remains + +**Two things, and neither is in this change's scope.** + +1. **The five pull requests above.** A Builder must not push to another seat's branch, so each is fixed by whoever next touches it: append `tests/.py` to `tests/tooling_manifest.txt`, keeping the list alphabetical. **The gate is passable and two pull requests that night passed it correctly -- nothing here asks for it to be weakened.** +2. **Three copies of the manifest parser, pinned against nothing.** This file, `_tooling_basenames` in `tests/conftest.py` (the copy that actually applies the marker), and `_manifest_paths` in `tests/test_ci_tooling_gate.py` all implement the same rule. Extracting one `tests/_tooling_manifest.py` is the real fix. Related: `test_every_manifest_entry_trips_its_own_gate` in `tests/test_ci_tooling_gate.py` feeds each manifest entry back as its own changed path, so on the manifest arm every entry matches itself unconditionally. diff --git a/tests/test_tooling_partition.py b/tests/test_tooling_partition.py index cc5bb01bf..db7d5c050 100644 --- a/tests/test_tooling_partition.py +++ b/tests/test_tooling_partition.py @@ -137,18 +137,105 @@ ) -def _manifest_names() -> list[str]: +def _manifest_lines(text: str) -> list[str]: return [ - line.strip().rsplit("/", 1)[-1] - for line in _MANIFEST.read_text(encoding="utf-8").splitlines() + line.strip() + for line in text.splitlines() if line.strip() and not line.lstrip().startswith("#") ] -def test_every_manifest_entry_exists() -> None: - """A stale entry silently un-marks a test back onto the engine legs.""" - missing = sorted(n for n in _manifest_names() if not (_TESTS / n).is_file()) - assert not missing, f"tooling_manifest.txt names files that do not exist: {missing}" +def _names_from(text: str) -> list[str]: + """THIS FILE's manifest parser, over arbitrary text rather than over the file. + + Split out so ``test_the_gate_offers_a_line_the_manifest_parser_accepts`` can feed the example + line the failure message hands out straight back through it. A message that recommends a shape + nothing checks is the compensating-control-on-a-false-premise defect (CLAUDE.md 11). + + IT IS NOT THE ONLY PARSER, AND SAYING SO WOULD BE THE SAME DEFECT ONE LEVEL UP. The copy that + actually applies the ``tooling`` marker is ``_tooling_basenames`` in tests/conftest.py, and a + third lives in ``_manifest_paths`` in tests/test_ci_tooling_gate.py. All three implement the + same rule -- strip, drop blanks and ``#`` comments, rsplit on ``/`` -- and NOTHING pins them + against each other. Extracting one shared ``tests/_tooling_manifest.py`` is the real fix and it + is not this change's job; until then, what the arm below proves is that the recommended line + survives THIS parser. + """ + return [line.rsplit("/", 1)[-1] for line in _manifest_lines(text)] + + +def _manifest_entries() -> list[str]: + """The manifest's full lines -- ``tests/.py``, which is what ci.yml's path gate matches.""" + return _manifest_lines(_MANIFEST.read_text(encoding="utf-8")) + + +def _manifest_names() -> list[str]: + return [line.rsplit("/", 1)[-1] for line in _manifest_entries()] + + +def _unclassified_remedy(names: list[str]) -> str: + """The text ``test_every_non_engine_test_is_classified`` fails with. + + WRITTEN TO BE ACTIONABLE FROM A CI LOG ALONE, because its reader usually cannot ask. Measured + 2026-09-03: this assertion reds all three required ``test`` legs, and the two Builders who + tripped it that night had each exited before any leg reported. The message is therefore the + whole remedy, and its predecessor named the offending file and stopped -- so both of them left + a red branch nobody was on. + + Line one carries the two registry paths and the file list, because that is the line a truncated + summary keeps. NOTHING HERE IS A HAND-WRITTEN PATH: the example is built from a real offender, + and both registries are rendered from ``_MANIFEST`` and ``__file__``, so a rename moves the + message with the file instead of leaving it pointing at a path that no longer exists. + """ + example = names[0] if names else "test_example.py" + manifest = _MANIFEST.relative_to(_ROOT).as_posix() + here = Path(__file__).resolve().relative_to(_ROOT).as_posix() + return ( + f"UNCLASSIFIED TEST FILE(S) -- add each to {manifest} or to " + f"_STAYS_WITHOUT_IMPORTING in {here}: {names}\n" + "They import no engine module, so nothing decides which CI legs run them. Choose one, per " + "file, in the SAME pull request:\n" + f" (a) HARNESS subject (scripts/**, .github/**, the ledger) -> append `tests/{example}` " + f"to {manifest}, keeping the `tests/` prefix (ci.yml matches whole changed paths against " + "these lines).\n" + f' (b) ENGINE subject it READS off disk without importing -> add `"{example}"` to ' + f"_STAYS_WITHOUT_IMPORTING in {here}, with a comment naming the messagefoundry/** file it " + "reads.\n" + " A test that IMPORTS the engine needs neither entry -- which is why most files here sit " + "in no list at all.\n" + "WHEN AMBIGUOUS CHOOSE (b). (a) is the answer that loses coverage silently: a wrongly " + "listed engine test leaves the engine legs, and the tooling job's scripts/** path gate is " + "not tripped by an engine diff, so it runs on NO leg for the change that would break it." + ) + + +def test_every_manifest_entry_resolves_as_written() -> None: + """A stale entry silently un-marks a test back onto the engine legs. + + RESOLVED AS WRITTEN, not by basename, and that is the strengthening. The predecessor read this + file through ``_manifest_names``, which rsplits the directory away -- so it proved a file of + that NAME exists somewhere in tests/ and was blind to the path actually written down. That + blindness matters because ci.yml's ``changes`` job summons the tooling job by matching WHOLE + changed paths against these lines (``grep -qxFf`` over ``git diff --name-only``). A bare + ``test_x.py``, a ``./tests/`` prefix or a backslash separator all still MARK the test -- this + file's parser and tests/conftest.py both rsplit on ``/`` -- so the test leaves the engine legs + by ``-m 'not tooling'``, and then a later pull request editing only that test matches no arm of + the gate and summons no tooling job. Deselected everywhere, green. + + Two clauses, because one does not cover it: the entry must name a real file, AND it must be + spelled the way git spells it. ``./tests/test_x.py`` satisfies the first and fails the second. + """ + bad: list[str] = [] + for line in _manifest_entries(): + target = _ROOT / line + if not target.is_file(): + bad.append(f"{line} (names no file)") + elif line != target.resolve().relative_to(_ROOT).as_posix(): + bad.append(f"{line} (not the repo-relative path git reports for it)") + assert not bad, ( + "tooling_manifest.txt entries must be repo-relative paths naming a real file, exactly as " + "`git diff --name-only` spells them, or ci.yml's path gate cannot match the changed file " + f"and the tier stops being summoned by an edit to it: {bad}" + ) def test_manifest_has_no_duplicates() -> None: @@ -170,23 +257,118 @@ def test_no_listed_test_imports_the_engine() -> None: ) -def test_every_non_engine_test_is_classified() -> None: - """The drift guard: a NEW harness test must land in the manifest or be named as staying. +def _assert_every_test_is_classified( + tests_dir: Path, listed: set[str], stays: frozenset[str] +) -> None: + """The gate itself, over a NAMED directory rather than the real one. - Without this, a new worktree-gate test quietly joins the engine legs and the tier grows back. + Parameterised for one reason: so ``test_the_gate_raises_the_remedy_and_not_a_bare_list`` can + drive this exact assertion into failure against a tmp_path and read the message it actually + raises. Asserting on ``_unclassified_remedy``'s return value instead was tried and MEASURED + VACUOUS -- reverting the call site below to the predecessor string left all the content arms + green, because they never touched the call site. The instrument was answering "does the helper + return good text" while the question is "does the gate FAIL with good text" (CLAUDE.md 11, + SDS-3.8). """ - listed = set(_manifest_names()) unclassified = sorted( p.name - for p in _TESTS.glob("test_*.py") + for p in tests_dir.glob("test_*.py") if p.name not in listed - and p.name not in _STAYS_WITHOUT_IMPORTING + and p.name not in stays and not _ENGINE_IMPORT.search(p.read_text(encoding="utf-8", errors="replace")) ) - assert not unclassified, ( - "these tests import no engine module, so they are unclassified: add them to " - "tests/tooling_manifest.txt, or to _STAYS_WITHOUT_IMPORTING here if they read engine " - f"source: {unclassified}" + assert not unclassified, _unclassified_remedy(unclassified) + + +def test_every_non_engine_test_is_classified() -> None: + """The drift guard: a NEW harness test must land in the manifest or be named as staying. + + Without this, a new worktree-gate test quietly joins the engine legs and the tier grows back. + """ + _assert_every_test_is_classified(_TESTS, set(_manifest_names()), _STAYS_WITHOUT_IMPORTING) + + +#: The offender the two arms below fabricate. One spelling, so renaming it cannot leave an +#: assertion comparing against a stale literal. +_SENTINEL = "test_made_up_harness_thing.py" + + +def _raised_remedy(tmp_path: Path) -> str: + """Drive the real gate into failure over one unclassified file and hand back its message. + + Doubles as the mechanism's positive control: a gate that cannot be made to fire here would + satisfy every arm below while reporting nothing on the real tree. + + The empty lists are deliberate rather than incidental -- they keep this arm hermetic against + someone later adding ``_SENTINEL`` to the real manifest or stay list. + """ + (tmp_path / _SENTINEL).write_text("def test_x() -> None:\n assert True\n", encoding="utf-8") + with pytest.raises(AssertionError) as caught: + _assert_every_test_is_classified(tmp_path, set(), frozenset()) + return str(caught.value) + + +def test_the_gate_offers_a_line_the_manifest_parser_accepts(tmp_path: Path) -> None: + """The message hands out a manifest line; this file's parser must read it back. + + Taken off the RAISED message, so it cannot drift from the assertion that issues it, and fed + through ``_names_from``, so it cannot drift from the reader that consumes it. + """ + rendered = _raised_remedy(tmp_path) + recommended = f"tests/{_SENTINEL}" + assert f"`{recommended}`" in rendered, rendered + assert _names_from(recommended) == [_SENTINEL], ( + "the manifest parser does not read back the line the failure message recommends" + ) + + +def test_the_gate_raises_the_remedy_and_not_a_bare_list(tmp_path: Path) -> None: + """A reader who only ever sees this string must be able to act on it. + + Both Builders who tripped this gate on 2026-09-03 had exited before any leg reported, so there + was nobody to ask what the remedy was. + + THE PATHS ARE RESOLVED, NOT MATCHED. A literal needle for ``tests/tooling_manifest.txt`` here + would be a literal checked against a literal: rename or move either registry and the message and + the test go stale together, green. So every ``tests/...`` path the message offers is pulled out + of the rendered string and required to name a real file, and the stay list is required to be + defined in the file the message sends the reader to. + + Read off the RAISED message, so reverting the call site to the predecessor (which named the + offending file and stopped) turns this red. Reading ``_unclassified_remedy`` directly did NOT -- + measured, and it is the whole reason this arm is shaped this way. + """ + rendered = _raised_remedy(tmp_path) + + offered = set(re.findall(r"tests/[A-Za-z0-9_./-]+\.(?:py|txt)", rendered)) + named_files = {p for p in offered if not p.endswith(f"/{_SENTINEL}")} + assert len(named_files) >= 2, f"the message offers fewer than two registry paths: {offered}" + unresolvable = sorted(p for p in named_files if not (_ROOT / p).is_file()) + assert not unresolvable, ( + f"the failure message sends its reader to paths that do not exist: {unresolvable}" + ) + assert _MANIFEST.relative_to(_ROOT).as_posix() in named_files, ( + f"the message never names the manifest itself: {sorted(named_files)}" + ) + + stay_list_home = next(p for p in named_files if p.endswith(".py")) + assert "_STAYS_WITHOUT_IMPORTING" in (_ROOT / stay_list_home).read_text(encoding="utf-8"), ( + f"the message sends the reader to {stay_list_home} for _STAYS_WITHOUT_IMPORTING, and it is " + "not defined there" + ) + + # The prose half. These carry no path to resolve, so they are needles -- and they are the three + # claims that make the message a remedy rather than a report. + for needle, why in ( + ("HARNESS subject", "the criterion is the test's SUBJECT, not its location"), + ("ENGINE subject", "the other half of that criterion"), + ("WHEN AMBIGUOUS", "the tie-break, without which a hurried reader picks (a)"), + ): + assert needle in rendered, f"the failure message no longer states {why}: {needle!r}" + + assert rendered.splitlines()[0].endswith(f"['{_SENTINEL}']"), ( + "line one must still end with the offending files -- it is the line a truncated CI summary " + "keeps, and it must name both the registries and the files at once" )