From 37ba4fcd3e76d3e82a948a37741ea00af6200015 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 13:59:54 +0300 Subject: [PATCH 1/4] feat: skip kit-repo-only tests instead of failing a sized-down adopter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kit tells adopters to run its test suite as post-install verification. A by-the-book `/adopt` tree ran ZERO tests; the engines-and-config floor ran 90 red. Both are now clean. **#226 first, because nothing under it was measurable.** `test_panel_prompt.py` read `docs/agentic-dev-kit/fallback-review-panel.md` at MODULE scope, so pytest aborted during collection in any tree without it — not "some tests fail", zero tests run. `/adopt` Step 3 names three files under `docs/agentic-dev-kit/` and this is not one of them, so that was a by-the-book adoption, not the extreme floor. The read is now a function called at use time. **#134 cause 2: a `kit_repo_only` marker, registered in the conftest that travels with the tests.** It takes the repo-relative paths a test needs and skips when any is absent. Why a skip and not a fix: these tests are inapplicable, not path-portable-with-effort. `test_init_sh.py` asserts on `init.sh`'s behaviour and an adopter who vendored engines and config has no `init.sh` to assert about. #134 says exactly that, and the repair of cause 1 in PR #202 demonstrated it — it converted a collection abort into legible failures, not into a clean run. Why paths rather than probing for "the kit's own repo": a test declares what it needs, so the answer is a fact about the tree rather than a judgement about which repo this is. Any such judgement would be a bound the author sets — the shape fallback-review-panel.md records as having opened a hole three times. It also gets the full-vendor case right for free: an adopter who keeps `scripts/` has the files, so these tests run there and pass. Marked, by what each actually needs: - `test_init_sh.py` — module, on `init.sh` - `test_portability.py` — 7 migration tests, on `init.sh` (not the module; the other ~370 tests there are portable and still run) - `test_kitconfig.py` — `test_narrative_templates_ship`, on `docs/templates` - `test_kit_doctor.py` — the 3 tests calling `derive_dependencies(REPO_ROOT)`, on `scripts/kit_doctor.py`, which is present exactly when engines sit at the kit's own layout. These are the ones self-reported on #225 as arriving with that PR. - `test_panel_prompt.py` — module, on the doctrine. The whole module and not just the readers: panel_prompt.py quotes its contract from that file at run time and exits 2 rather than guessing, so an engine installed without it is non-functional by design. Measured, three trees, all at this head, each stated with its vendored subset because #134's own thread establishes that a count identifies no tree without one: | tree | before | after | |-----------------------------------|-------------------------|--------------------------| | kit's own repo | 699 passed | 711 passed, 0 skipped | | by-the-book /adopt (no doctrine) | 0 run, collection abort | 640 passed, 60 skipped | | engines+config floor | 90 failed, 1 error | 548 passed, 152 skipped | Coverage here is unchanged: every marked path exists in this repo, so nothing skips and the kit still runs its own suite in full. `test_kit_repo_only.py` pins the mechanism by running pytest in a SUBPROCESS against synthetic vendored trees at `scripts/devkit/` — asserting from inside this repo, where every path exists, would only ever exercise the not-skipped branch, and "fires when a path is absent" is the whole behaviour. Its marked-path check is DERIVED by scanning the modules rather than restated, with a non-vacuity control so a regex that stopped matching cannot pass over an empty set. Five mutations run, each killed by the test written for it: skip never fires, skip always fires, exists()->is_file(), only the first path checked, registration dropped. The limit is stated in the conftest rather than papered over: in this repo a deleted kit file would make its tests go quiet rather than red. `test_kit_repo_only.py` catches a marker naming a path that never existed; it cannot catch a deletion, because the marker would then be telling the truth. kit-manifest.json covers every KIT_OWNED path, but `init.sh` and the root Makefile are tracked by neither. Not formatted with `ruff format`: ruff.toml and the CI step both record that the kit is deliberately lint-only and not format-clean. Running it here churned 700+ lines across four untouched modules and was reverted. make test -> 711 passed. kit_doctor -> 32 unchanged, 0 differ, 0 missing, 0 unknown, exit 0. ruff check clean. Closes #226. Closes #134 — cause 1 of it was already repaired by PR #202, and this change is cause 2, the remainder. --- scripts/tests/conftest.py | 58 +++++++++- scripts/tests/test_init_sh.py | 7 ++ scripts/tests/test_kit_doctor.py | 3 + scripts/tests/test_kit_repo_only.py | 170 ++++++++++++++++++++++++++++ scripts/tests/test_kitconfig.py | 1 + scripts/tests/test_panel_prompt.py | 39 ++++++- scripts/tests/test_portability.py | 7 ++ 7 files changed, 281 insertions(+), 4 deletions(-) create mode 100644 scripts/tests/test_kit_repo_only.py diff --git a/scripts/tests/conftest.py b/scripts/tests/conftest.py index 5b50020..0fcc2ca 100644 --- a/scripts/tests/conftest.py +++ b/scripts/tests/conftest.py @@ -25,9 +25,22 @@ from __future__ import annotations +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +from _repo_layout import engine_dir, find_repo_root # noqa: E402 + +# Resolved once, the same way every test module resolves it — walk up for `.git` +# from the engines directory, itself derived from this file's own location +# rather than counted in `parents[N]` (#134 cause 1). +REPO_ROOT = find_repo_root(engine_dir(Path(__file__))) + def pytest_configure(config) -> None: - """Register `driftcheck` so `-m 'not driftcheck'` is a supported invocation. + """Register the marks, so `-m` expressions naming them are supported. Unregistered marks still *match* under `-m`, so the exclusion would work without this — it would just warn (`PytestUnknownMarkWarning`) on every run. @@ -41,3 +54,46 @@ def pytest_configure(config) -> None: "(`-m 'not driftcheck'`) because ANY mutation fails it, which reads " "as a kill while nothing behavioural caught anything (#33/#112).", ) + config.addinivalue_line( + "markers", + "kit_repo_only(*paths): asserts against files the kit ships but an " + "adopter need not vendor. Skipped when any named path is absent, so a " + "sized-down adoption gets a clean run instead of inapplicable failures " + "(#134 cause 2).", + ) + + +def pytest_runtest_setup(item) -> None: + """Skip a `kit_repo_only` test whose required paths this tree does not have. + + **Why a skip and not a fix.** These tests are not path-portable-with-effort; + they are *inapplicable*. `test_init_sh.py` asserts on `init.sh`'s behaviour, + and an adopter who vendored engines and config has no `init.sh` to assert + about. Repairing the paths would only convert a `FileNotFoundError` into a + differently-worded failure — #134 says exactly that, and the repair of + cause 1 in PR #202 demonstrated it: it turned a collection abort into 90 + legible failures rather than into a clean run. + + **Why the marker takes paths rather than probing for "the kit's own repo".** + A test declares what it needs, and the answer is then a fact about the tree + rather than a judgement about which repo this is. Any such judgement would + be a bound the author sets — the shape `fallback-review-panel.md` records as + having opened a hole three times — and no marker file distinguishes the + kit's checkout from a full-vendor adoption anyway. A full vendor that keeps + `scripts/` runs these tests, correctly, because it genuinely has the files. + + **The limit this leaves, stated because it is real.** In the kit's own repo + every path is present, so nothing skips and coverage is unchanged — but if a + kit file were deleted here, its tests would go quiet rather than red. + `test_kit_repo_only.py` catches a marker naming a path that never existed; + it cannot catch a deletion, because the marker would then be telling the + truth. `kit-manifest.json` covers every KIT_OWNED path; `init.sh` and the + root `Makefile` are tracked by neither, so for those two the trade is a loud + failure for a counted skip. + """ + marker = item.get_closest_marker("kit_repo_only") + if marker is None: + return + missing = [rel for rel in marker.args if not (REPO_ROOT / rel).exists()] + if missing: + pytest.skip(f"not vendored in this tree: {', '.join(missing)}") diff --git a/scripts/tests/test_init_sh.py b/scripts/tests/test_init_sh.py index 5da4106..847f5f7 100644 --- a/scripts/tests/test_init_sh.py +++ b/scripts/tests/test_init_sh.py @@ -35,6 +35,13 @@ import yaml from _repo_layout import engine_dir, find_repo_root +# Every test here asserts on `init.sh`'s behaviour, and an adopter who +# vendored engines and config has no `init.sh` to assert about. Repairing +# the paths would only turn FileNotFoundError into a differently-worded +# failure — #134 cause 2 says the honest handling is a skip, not a fix. +pytestmark = pytest.mark.kit_repo_only("init.sh") + + ENGINE_DIR = engine_dir(Path(__file__)) REPO_ROOT = find_repo_root(ENGINE_DIR) sys.path.insert(0, str(ENGINE_DIR / "lib")) diff --git a/scripts/tests/test_kit_doctor.py b/scripts/tests/test_kit_doctor.py index d3c9a65..471051f 100644 --- a/scripts/tests/test_kit_doctor.py +++ b/scripts/tests/test_kit_doctor.py @@ -607,6 +607,7 @@ def _tree(tmp_path: Path, files: dict[str, str]) -> Path: return tmp_path +@pytest.mark.kit_repo_only("scripts/kit_doctor.py") def test_dependency_graph_of_the_real_kit_names_kitconfigs_importers(): """Measured against the kit's own tree, not a fixture. @@ -668,6 +669,7 @@ def test_importing_a_package_by_name_resolves_to_its_init(tmp_path): } +@pytest.mark.kit_repo_only("scripts/kit_doctor.py") def test_the_shell_source_dependency_is_a_KNOWN_GAP_not_an_oversight(): """`dev_session.sh` and `reconcile_sessions.sh` both `source "$SCRIPT_DIR/lib/repo_root.sh"`, and that edge is deliberately NOT @@ -702,6 +704,7 @@ def test_the_shell_source_dependency_is_a_KNOWN_GAP_not_an_oversight(): } +@pytest.mark.kit_repo_only("scripts/kit_doctor.py") def test_shipped_manifest_required_by_matches_a_fresh_derivation(): """A stale `required_by` is a silent downgrade: the file stops being called required and the report goes back to inviting an operator to decline it. diff --git a/scripts/tests/test_kit_repo_only.py b/scripts/tests/test_kit_repo_only.py new file mode 100644 index 0000000..ab23810 --- /dev/null +++ b/scripts/tests/test_kit_repo_only.py @@ -0,0 +1,170 @@ +"""What pins the `kit_repo_only` skip mechanism (`conftest.py`). + +The mechanism exists because several test modules assert against files the kit +ships but an adopter need not vendor — `init.sh`, `docs/templates/*.tmpl`, the +panel doctrine. Before it, a sized-down adopter's first run of the suite the kit +tells them to run as post-install verification was 90 red, which trains an +adopter to ignore the suite and hides the failures that would be real (#134 +cause 2). + +These tests run pytest in a **subprocess against a synthetic tree**, not against +this repo. Asserting the mechanism from inside the repo where every path happens +to exist would only ever exercise the not-skipped branch — and "the marker fires +when a path is absent" is the whole behaviour. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest +from _repo_layout import engine_dir + +TESTS_DIR = Path(__file__).resolve().parent +ENGINE_DIR = engine_dir(Path(__file__)) + + +def _tree(tmp_path: Path, body: str) -> Path: + """A minimal vendored tree: a `.git` marker, the real conftest, one module. + + The engines land at `scripts/devkit/` deliberately — the layout `/adopt` + defaults to and the one that broke `parents[2]` — so this also covers the + conftest resolving its own root by walk-up rather than by counting. + """ + root = tmp_path / "adopter" + vendored = root / "scripts" / "devkit" / "tests" + vendored.mkdir(parents=True) + (root / ".git").mkdir() + for name in ("conftest.py", "_repo_layout.py"): + shutil.copy(TESTS_DIR / name, vendored / name) + (vendored / "test_probe.py").write_text(body, encoding="utf-8") + return root + + +def _run(root: Path) -> subprocess.CompletedProcess: + return subprocess.run( + # `-rs` renders skip REASONS. Without it the reason is computed and + # discarded, so an assertion on it would pass vacuously against output + # that never contained one. + [sys.executable, "-m", "pytest", "scripts/devkit/tests", "-q", "--no-header", "-rs"], + cwd=root, + capture_output=True, + text=True, + ) + + +PROBE = """ +import pytest + + +@pytest.mark.kit_repo_only({paths}) +def test_probe(): + assert True +""" + + +def test_a_marker_naming_an_absent_path_skips(tmp_path): + root = _tree(tmp_path, PROBE.format(paths='"init.sh"')) + out = _run(root) + assert out.returncode == 0, out.stdout + out.stderr + assert "1 skipped" in out.stdout, out.stdout + # The reason names the path, so a reader of a skipped run can tell an + # intentional omission from a broken install without reading this file. + assert "init.sh" in out.stdout, out.stdout + + +def test_a_marker_naming_a_present_path_runs(tmp_path): + """The other branch, and the one that matters for the kit's own repo: if the + marker skipped unconditionally it would silently delete coverage here while + every suite still reported green.""" + root = _tree(tmp_path, PROBE.format(paths='"init.sh"')) + (root / "init.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + out = _run(root) + assert out.returncode == 0, out.stdout + out.stderr + assert "1 passed" in out.stdout, out.stdout + assert "skipped" not in out.stdout, out.stdout + + +def test_one_absent_path_among_several_is_enough_to_skip(tmp_path): + root = _tree(tmp_path, PROBE.format(paths='"init.sh", "Makefile"')) + (root / "init.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + out = _run(root) + assert "1 skipped" in out.stdout, out.stdout + reason = next(ln for ln in out.stdout.splitlines() if "not vendored" in ln) + assert "Makefile" in reason, reason + # Only the genuinely missing one is named — a reason listing a file the tree + # has would send an adopter looking for a problem they do not have. + assert "init.sh" not in reason, reason + + +def test_a_directory_counts_as_present(tmp_path): + """`docs/templates` is named as a directory by `test_kitconfig.py`, so the + check has to be `exists()` rather than `is_file()`.""" + root = _tree(tmp_path, PROBE.format(paths='"docs/templates"')) + (root / "docs" / "templates").mkdir(parents=True) + out = _run(root) + assert "1 passed" in out.stdout, out.stdout + + +def test_an_unmarked_test_is_untouched(tmp_path): + root = _tree(tmp_path, "def test_probe():\n assert True\n") + out = _run(root) + assert "1 passed" in out.stdout, out.stdout + assert "skipped" not in out.stdout, out.stdout + + +def test_the_marker_is_registered_so_m_expressions_do_not_warn(tmp_path): + """An unregistered mark still *matches* under `-m`, so the selection would + work — it would just warn on every run. A warning attached to a command a + reviewer is told to trust is what gets the command dropped.""" + root = _tree(tmp_path, PROBE.format(paths='"init.sh"')) + out = _run(root) + assert "PytestUnknownMarkWarning" not in out.stdout + out.stderr + + +def _marked_paths() -> set[str]: + """Every path named by a `kit_repo_only` marker anywhere in this suite. + + DERIVED by scanning the modules rather than restated as a list, for the same + reason `kit_doctor._derive_engine_names` is derived: a hand-kept copy goes + stale exactly when someone adds a marker, which is the moment the check + below needed to know about it. + """ + found: set[str] = set() + for module in sorted(TESTS_DIR.glob("test_*.py")): + for call in re.finditer(r"kit_repo_only\(([^)]*)\)", module.read_text(encoding="utf-8")): + found.update(re.findall(r'"([^"]+)"', call.group(1))) + return found + + +def test_the_marker_scan_finds_something(): + """A non-vacuity control on `_marked_paths`. If the regex stopped matching — + a marker written with single quotes, a rename — the check below would pass + over an empty set and assert nothing, silently.""" + found = _marked_paths() + assert "init.sh" in found, found + assert len(found) >= 4, found + + +@pytest.mark.parametrize("path", sorted(_marked_paths())) +def test_every_path_this_repo_marks_actually_exists_here(path): + """The kit's own repo must have every path its markers name, or those tests + go quiet rather than red. + + This is a positive control on the marker set, not on the mechanism: it fails + if someone marks a test with a path that never existed (a typo, a renamed + file), which would skip that test in EVERY tree including this one — the + failure mode the mechanism makes possible and nothing else would catch. + + It does not, and cannot, catch a kit file being deleted: the marker would + then correctly report it absent, and this test would fail for the same + reason the deletion caused. That is the stated limit in `conftest.py`. + """ + assert (ENGINE_DIR.parent / path).exists(), ( + f"a kit_repo_only marker names {path!r}, which does not exist in the " + "kit's own repo — every test carrying it will skip everywhere" + ) diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 0367d8e..54e96b9 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -296,6 +296,7 @@ def test_load_config_reports_a_missing_file_clearly(): "AGENTS.md.tmpl", ], ) +@pytest.mark.kit_repo_only("docs/templates") def test_narrative_templates_ship(name): """init.sh renders these; a missing one silently degrades adoption to an unrendered skeleton, which is the bug the templates were added to fix.""" diff --git a/scripts/tests/test_panel_prompt.py b/scripts/tests/test_panel_prompt.py index 475b3da..b830c39 100644 --- a/scripts/tests/test_panel_prompt.py +++ b/scripts/tests/test_panel_prompt.py @@ -36,7 +36,40 @@ REPO_ROOT = find_repo_root(Path(__file__).resolve()) ENGINE = engine_dir(Path(__file__).resolve()) / "panel_prompt.py" DOCTRINE = Path("docs") / "agentic-dev-kit" / "fallback-review-panel.md" -DOCTRINE_TEXT = (REPO_ROOT / DOCTRINE).read_text() + +# The whole module is conditioned on the shipped doctrine, not just the tests +# that read it: `panel_prompt.py` QUOTES its contract out of this file at run +# time and exits 2 rather than guessing when it cannot, so an engine installed +# without the doctrine is non-functional by design and there is nothing here +# worth asserting about it. That an adopter can reach that state at all is +# #226's second half — `/adopt` Step 3 installs three files from +# `docs/agentic-dev-kit/` and this is not one of them — and it stays open there. +# +# A string LITERAL, not `str(DOCTRINE)`, so `test_kit_repo_only.py` can find it +# by scanning the source; the test below pins the two spellings together. +pytestmark = pytest.mark.kit_repo_only("docs/agentic-dev-kit/fallback-review-panel.md") + + +def test_the_marker_path_matches_the_doctrine_path(): + """Without this, a doctrine rename would leave the marker naming a file that + no longer exists, and skip this whole module everywhere — silently.""" + assert str(DOCTRINE) == "docs/agentic-dev-kit/fallback-review-panel.md" + + +def doctrine_text() -> str: + """The shipped doctrine, read at CALL time rather than import time. + + This was a module-level `read_text()`, which raised during **collection** in + any tree without the doctrine — so pytest aborted and ran **zero** tests, + rather than failing the handful that need the file. `/adopt` Step 3 does not + name `fallback-review-panel.md` among the docs it installs, so that was not + the extreme floor: it was a by-the-book adoption (#226). + + A function and not a fixture, deliberately: one caller wants it inside a + test body and one inside another fixture, and a fixture would thread a + parameter through call sites that need nothing else. + """ + return (REPO_ROOT / DOCTRINE).read_text(encoding="utf-8") def _load(): @@ -67,7 +100,7 @@ def repo(tmp_path: Path) -> Path: (root / "docs" / "agentic-dev-kit").mkdir(parents=True) # The real doctrine, so the contract these tests assert on is the shipped one. - (root / DOCTRINE).write_text(DOCTRINE_TEXT) + (root / DOCTRINE).write_text(doctrine_text()) (root / "config" / "dev-model.yaml").write_text( "vcs:\n protected_branch: main\n" "review:\n" @@ -139,7 +172,7 @@ def test_the_contract_is_read_from_the_doctrine_not_embedded_in_the_script(tmp_p """ pp = _load() doctored = tmp_path / "doctrine.md" - text = DOCTRINE_TEXT.replace( + text = doctrine_text().replace( "1. **Fresh context.**", "1. **Wholly invented item.**", 1 ) doctored.write_text(text) diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index a646568..ec737e8 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -1090,6 +1090,7 @@ def test_keep_alone_is_unchanged_by_the_target_lines_addition( assert updated_history.index("### Second") < updated_history.index("### First") +@pytest.mark.kit_repo_only("init.sh") def test_init_migrates_the_previous_runtime_schema(tmp_path: Path) -> None: repo = tmp_path / "project" (repo / "config").mkdir(parents=True) @@ -1273,6 +1274,7 @@ def _run_init(tmp_path: Path, name: str, config_text: str): @pytest.mark.parametrize("shape", sorted(_MIGRATION_SHAPES)) +@pytest.mark.kit_repo_only("init.sh") def test_migration_never_corrupts_or_silently_drops_adopter_config( tmp_path: Path, shape: str ) -> None: @@ -1318,6 +1320,7 @@ def test_migration_never_corrupts_or_silently_drops_adopter_config( @pytest.mark.parametrize("shape", sorted(_MIGRATION_SHAPES)) +@pytest.mark.kit_repo_only("init.sh") def test_migration_is_idempotent(tmp_path: Path, shape: str) -> None: """Re-running `./init.sh` is the documented upgrade path, so a second run must be a no-op — not a second copy of every key it added.""" @@ -1331,6 +1334,7 @@ def test_migration_is_idempotent(tmp_path: Path, shape: str) -> None: assert path.read_text(encoding="utf-8") == once +@pytest.mark.kit_repo_only("init.sh") def test_migration_adds_every_review_key_exactly_once(tmp_path: Path) -> None: """Per-key guards, not one guard over a block of five. @@ -1402,6 +1406,7 @@ def test_migration_adds_every_review_key_exactly_once(tmp_path: Path) -> None: ), ], ) +@pytest.mark.kit_repo_only("init.sh") def test_the_instruction_matches_the_list_style( tmp_path: Path, style: str, config: str, wanted: str, unwanted: str ) -> None: @@ -1438,6 +1443,7 @@ def test_the_instruction_matches_the_list_style( ("absent", "review:\n bots: [bugbot]\n"), ], ) +@pytest.mark.kit_repo_only("init.sh") def test_the_instruction_stays_quiet_when_there_is_nothing_to_add( tmp_path: Path, case: str, config: str ) -> None: @@ -1446,6 +1452,7 @@ def test_the_instruction_stays_quiet_when_there_is_nothing_to_add( assert "ACTION NEEDED" not in proc.stderr +@pytest.mark.kit_repo_only("init.sh") def test_a_marker_named_only_in_a_comment_does_not_count(tmp_path: Path) -> None: """The kit's own shipped config carries a trailing comment on that very line, so a raw-line grep is satisfied by a config whose LIST lacks it.""" From d7e1f81ee6e361cc360cb3c50ef69caf74492c70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 14:37:44 +0300 Subject: [PATCH 2/4] fix: scope the positive control, compose markers, and correct a false claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round 1, both lenses. Four findings, all real, and the first one invalidated this PR's own headline measurement. **The positive control had no scope guard (adversarial, HIGH).** `test_every_path_this_repo_marks_actually_exists_here` asserted that every marked path exists, with nothing restricting it to the kit's own tree — so it ran in every vendored tree and failed wherever a marked path was legitimately absent, which is the designed state of a sized-down adoption. It turned the by-the-book /adopt tree this PR exists to make clean into 4 failures. **And my measurement missed it, which is the more useful finding.** The three tree figures in the previous commit were taken while `test_kit_repo_only.py` was still UNTRACKED, and the tree builder selects paths with `git ls-files` — so every measured tree omitted the file the PR was adding. Rebuilt from the committed state, the /adopt tree was `4 failed`, not `0 failed`. A measurement that cannot see the change it is measuring is worse than no measurement, and the previous commit message's table should be read as withdrawn. The guard is now `_is_complete_kit_tree()`: does this tree hold every file `kit-manifest.json` lists. DERIVED rather than a judgement about "is this the kit's own repo", which would be a bound the author sets. True in the kit's checkout and in a full vendor that kept `scripts/`, false in any sized-down tree. **A function marker replaced its module marker instead of adding to it (correctness, HIGH).** `get_closest_marker` returns only the nearest, so a function-level `kit_repo_only` silently dropped the module's requirement. Now `iter_markers`, unioned. This was not hypothetical: 6 tests in `test_init_sh.py` transitively need `docs/templates` — `_fixture(templates=True)` globs the real directory, and a missing directory globs to nothing, so `init.sh` seeds nothing and the test's own read throws. In a tree with `init.sh` and no `docs/templates` they failed rather than skipped. Marked, and they now need both. **A wrong repo root turned a loud failure into a confident wrong claim (correctness, MED-HIGH).** `find_repo_root` falls back to `start.parent` when no `.git` is found, which is one level short in the `scripts/devkit/` layout /adopt defaults to (#60, pinned elsewhere). Before this PR that surfaced as a `FileNotFoundError` from a test body; the skip converted it into `not vendored in this tree` about a file that was present. The hook now does not skip at all when no `.git` marker was found, preserving the loud failure. **The module marker on `test_panel_prompt.py` was over-broad (both lenses, MED).** 15 test cases there never read the shipped doctrine — they parse synthetic doctrines they write themselves, or exercise `_repo_slug()`, a pure string function with three prior lens-found bugs. Marking the module skipped all of them in exactly the tree #226 says /adopt produces. The dependency now lives in `doctrine_text()` via `require_kit_paths()`, the fixture-time counterpart of the marker — declared where it arises, so a new test using the `repo` fixture inherits it rather than needing a 39th decorator. **And a claim of mine was simply false.** The conftest said a deleted kit file would "go quiet rather than red". It does not: deleting each marked path in turn shows the positive control fires every time, and deleting `scripts/kit_doctor.py` aborts collection outright. The docstring now says what is actually true — the control cannot tell a deletion from a typo, and `init.sh` is the one marked path the manifest does not track. Also renamed `test_the_marker_is_registered_so_m_expressions_do_not_warn`, whose body passes no `-m` flag; registration warns on any application, which is what it checks. Measured from the COMMITTED state this time, each tree with its vendored subset: | tree | before this PR | now | |-----------------------------------|-------------------------|--------------------------| | kit's own repo | 699 passed | 711 passed, 0 skipped | | by-the-book /adopt (no doctrine) | 0 run, collection abort | 662 passed, 49 skipped | | engines+config floor | 90 failed, 1 error | 570 passed, 141 skipped | Both vendored trees are 0 failed, and both run MORE tests than the previous commit measured (662 vs 640, 570 vs 548) because the module marker no longer over-skips. make test -> 711 passed. ruff check clean. --- scripts/tests/conftest.py | 73 ++++++++++++++++++++++------- scripts/tests/test_init_sh.py | 6 +++ scripts/tests/test_kit_repo_only.py | 64 ++++++++++++++++++------- scripts/tests/test_panel_prompt.py | 32 ++++++++----- 4 files changed, 127 insertions(+), 48 deletions(-) diff --git a/scripts/tests/conftest.py b/scripts/tests/conftest.py index 0fcc2ca..626f6ca 100644 --- a/scripts/tests/conftest.py +++ b/scripts/tests/conftest.py @@ -33,10 +33,42 @@ sys.path.insert(0, str(Path(__file__).resolve().parent)) from _repo_layout import engine_dir, find_repo_root # noqa: E402 -# Resolved once, the same way every test module resolves it — walk up for `.git` -# from the engines directory, itself derived from this file's own location -# rather than counted in `parents[N]` (#134 cause 1). -REPO_ROOT = find_repo_root(engine_dir(Path(__file__))) +ENGINE_DIR = engine_dir(Path(__file__)) +REPO_ROOT = find_repo_root(ENGINE_DIR) + +# Whether a `.git` marker was actually FOUND, as opposed to `find_repo_root` +# falling back to `start.parent`. The fallback is right only when the engines +# sit directly under the root, and is one level short in the `scripts/devkit/` +# layout `/adopt` defaults to (#60, pinned in `test_repo_layout.py`). +# +# It matters here and nowhere else in the suite: a wrong root used to surface as +# a loud `FileNotFoundError` from a test body, and the skip below would convert +# that into `not vendored in this tree` — a confident, wrong claim about a file +# that is present. So when the marker is absent the skip does not run at all, +# and the pre-existing loud failure is preserved. Adversarial and correctness +# lenses, PR #232 round 1. +ROOT_IS_RESOLVED = any((c / ".git").exists() for c in (ENGINE_DIR, *ENGINE_DIR.parents)) + + +def require_kit_paths(*paths: str) -> None: + """Skip the current test unless every path is present, from inside a fixture. + + The fixture-time counterpart of the `kit_repo_only` marker, and the same + predicate. It exists because a dependency introduced by a FIXTURE cannot be + declared on the tests that use it without repeating a marker on every one of + them — and a marker repeated 38 times goes stale the first time someone adds + a 39th test. Expressed at the fixture, a new user of that fixture inherits + it. `test_kit_repo_only.py` scans for both spellings. + """ + _skip_if_missing(paths, "a fixture this test uses needs") + + +def _skip_if_missing(paths, prefix: str) -> None: + if not paths or not ROOT_IS_RESOLVED: + return + missing = [rel for rel in paths if not (REPO_ROOT / rel).exists()] + if missing: + pytest.skip(f"{prefix}: not vendored in this tree: {', '.join(missing)}") def pytest_configure(config) -> None: @@ -82,18 +114,23 @@ def pytest_runtest_setup(item) -> None: kit's checkout from a full-vendor adoption anyway. A full vendor that keeps `scripts/` runs these tests, correctly, because it genuinely has the files. - **The limit this leaves, stated because it is real.** In the kit's own repo - every path is present, so nothing skips and coverage is unchanged — but if a - kit file were deleted here, its tests would go quiet rather than red. - `test_kit_repo_only.py` catches a marker naming a path that never existed; - it cannot catch a deletion, because the marker would then be telling the - truth. `kit-manifest.json` covers every KIT_OWNED path; `init.sh` and the - root `Makefile` are tracked by neither, so for those two the trade is a loud - failure for a counted skip. + **The limit this leaves, corrected from a claim that was wrong.** An earlier + version of this docstring said a deleted kit file would "go quiet rather + than red". It does not: `test_kit_repo_only.py`'s positive control asserts + every marked path exists in a complete kit tree, and it fires on a deletion + exactly as readily as on a typo — measured by deleting each marked path in + turn. Deleting `scripts/kit_doctor.py` is louder still, since + `test_kit_doctor.py` imports it at module scope and collection aborts. + + What the control genuinely cannot do is tell a deletion from a typo, and it + is scoped to trees that hold every file `kit-manifest.json` lists — so a + marker naming `init.sh`, which the manifest does not track, is the one path + whose typo could go unnoticed in a tree that is otherwise incomplete. """ - marker = item.get_closest_marker("kit_repo_only") - if marker is None: - return - missing = [rel for rel in marker.args if not (REPO_ROOT / rel).exists()] - if missing: - pytest.skip(f"not vendored in this tree: {', '.join(missing)}") + # `iter_markers`, not `get_closest_marker`: a function-level marker must ADD + # to a module-level one rather than replace it. `test_portability.py`'s + # init.sh tests that also need `docs/templates` are exactly that case, and + # with `get_closest_marker` the function marker silently dropped the + # module's `init.sh` requirement. Correctness lens, PR #232 round 1. + paths = sorted({rel for m in item.iter_markers("kit_repo_only") for rel in m.args}) + _skip_if_missing(paths, "needs") diff --git a/scripts/tests/test_init_sh.py b/scripts/tests/test_init_sh.py index 847f5f7..58d0f9b 100644 --- a/scripts/tests/test_init_sh.py +++ b/scripts/tests/test_init_sh.py @@ -609,6 +609,7 @@ def test_set_field_writes_backslashes_literally(tmp_path: Path) -> None: # --------------------------------------------------------------------------- # +@pytest.mark.kit_repo_only("docs/templates") def test_seeds_narrative_docs_with_tokens_rendered(tmp_path: Path) -> None: repo = _fixture(tmp_path, config=SHIPPED_CONFIG, templates=True) @@ -663,6 +664,7 @@ def test_seeds_narrative_docs_with_tokens_rendered(tmp_path: Path) -> None: assert "docs/kit-handoff.md" in seeded["AGENTS.md"] # {{HANDOFF_PATH}} +@pytest.mark.kit_repo_only("docs/templates") def test_blank_tracker_url_renders_the_set_it_instruction(tmp_path: Path) -> None: """The {{TRACKER_URL}} fallback branch, pinned independently of what the shipped config holds. @@ -680,6 +682,7 @@ def test_blank_tracker_url_renders_the_set_it_instruction(tmp_path: Path) -> Non assert "{{" not in friction +@pytest.mark.kit_repo_only("docs/templates") def test_render_preserves_backslashes_in_values(tmp_path: Path) -> None: """_render passes values to awk via ENVIRON: with `-v`, a backslash-n in a project name became a real newline in every seeded doc — this was the one @@ -692,6 +695,7 @@ def test_render_preserves_backslashes_in_values(tmp_path: Path) -> None: assert r"Acme\nCo" in handoff +@pytest.mark.kit_repo_only("docs/templates") def test_seeding_respects_in_use_docs_and_reclaims_marked_ones(tmp_path: Path) -> None: repo = _fixture(tmp_path, config=SHIPPED_CONFIG, templates=True) (repo / "docs").mkdir(parents=True, exist_ok=True) @@ -710,6 +714,7 @@ def test_seeding_respects_in_use_docs_and_reclaims_marked_ones(tmp_path: Path) - assert "{{" not in reseeded +@pytest.mark.kit_repo_only("docs/templates") def test_agents_md_renders_the_configured_protected_branch(tmp_path: Path) -> None: """{{PROTECTED_BRANCH}} pinned against a DISTINCTIVE value, because the token has a FALLBACK: `render_protected_branch` defaults to "main" when the config @@ -728,6 +733,7 @@ def test_agents_md_renders_the_configured_protected_branch(tmp_path: Path) -> No assert "trunk-9f2a" in (repo / "AGENTS.md").read_text(encoding="utf-8") +@pytest.mark.kit_repo_only("docs/templates") @pytest.mark.parametrize("marker_line", [2, 3]) def test_seeding_leaves_a_doc_that_merely_quotes_the_marker_untouched( tmp_path: Path, marker_line: int diff --git a/scripts/tests/test_kit_repo_only.py b/scripts/tests/test_kit_repo_only.py index ab23810..906f2f6 100644 --- a/scripts/tests/test_kit_repo_only.py +++ b/scripts/tests/test_kit_repo_only.py @@ -15,6 +15,7 @@ from __future__ import annotations +import json import re import shutil import subprocess @@ -22,10 +23,11 @@ from pathlib import Path import pytest -from _repo_layout import engine_dir +from _repo_layout import engine_dir, find_repo_root TESTS_DIR = Path(__file__).resolve().parent ENGINE_DIR = engine_dir(Path(__file__)) +REPO_ROOT = find_repo_root(ENGINE_DIR) def _tree(tmp_path: Path, body: str) -> Path: @@ -135,12 +137,32 @@ def _marked_paths() -> set[str]: below needed to know about it. """ found: set[str] = set() + pattern = re.compile(r"(?:kit_repo_only|require_kit_paths)\(([^)]*)\)") for module in sorted(TESTS_DIR.glob("test_*.py")): - for call in re.finditer(r"kit_repo_only\(([^)]*)\)", module.read_text(encoding="utf-8")): + for call in pattern.finditer(module.read_text(encoding="utf-8")): found.update(re.findall(r'"([^"]+)"', call.group(1))) return found +def _is_complete_kit_tree() -> bool: + """Whether this tree holds every file `kit-manifest.json` says the kit ships. + + The scope guard the check below needs, and DERIVED rather than a judgement + about "is this the kit's own repo" — which would be a bound the author sets. + True in the kit's checkout and in a full vendor that kept `scripts/`; false + in any sized-down tree, where a marked path being absent is the mechanism + working rather than a defect. + """ + manifest = REPO_ROOT / "kit-manifest.json" + if not manifest.is_file(): + return False + try: + files = json.loads(manifest.read_text(encoding="utf-8")).get("files", {}) + except (json.JSONDecodeError, OSError): + return False + return bool(files) and all((REPO_ROOT / rel).exists() for rel in files) + + def test_the_marker_scan_finds_something(): """A non-vacuity control on `_marked_paths`. If the regex stopped matching — a marker written with single quotes, a rename — the check below would pass @@ -150,21 +172,29 @@ def test_the_marker_scan_finds_something(): assert len(found) >= 4, found +@pytest.mark.skipif( + not _is_complete_kit_tree(), + reason="not a complete kit tree — a marked path being absent here is the " + "mechanism working, not a defect", +) @pytest.mark.parametrize("path", sorted(_marked_paths())) -def test_every_path_this_repo_marks_actually_exists_here(path): - """The kit's own repo must have every path its markers name, or those tests - go quiet rather than red. - - This is a positive control on the marker set, not on the mechanism: it fails - if someone marks a test with a path that never existed (a typo, a renamed - file), which would skip that test in EVERY tree including this one — the - failure mode the mechanism makes possible and nothing else would catch. - - It does not, and cannot, catch a kit file being deleted: the marker would - then correctly report it absent, and this test would fail for the same - reason the deletion caused. That is the stated limit in `conftest.py`. +def test_every_path_a_marker_names_exists_in_a_complete_kit_tree(path): + """A positive control on the marker SET, not on the mechanism. + + It fails if a marker names a path that is not there — a typo, a renamed + file — which would otherwise skip that test in every tree, silently. + + **The scope guard is the point, and its absence was a HIGH finding.** With + no guard this ran in every vendored tree and failed wherever a marked path + was legitimately absent — which is the normal, designed state of a + sized-down adoption. It turned the `/adopt` tree this PR exists to make + clean into 4 failures. Adversarial lens, PR #232 round 1. + + It catches a DELETION too, loudly, contradicting an earlier version of this + docstring that claimed it could not: the assertion does not know why a path + is missing and fires either way. What it cannot do is tell the two apart. """ - assert (ENGINE_DIR.parent / path).exists(), ( - f"a kit_repo_only marker names {path!r}, which does not exist in the " - "kit's own repo — every test carrying it will skip everywhere" + assert (REPO_ROOT / path).exists(), ( + f"a kit_repo_only marker names {path!r}, which is absent from a tree " + "that is otherwise a complete kit — every test carrying it will skip" ) diff --git a/scripts/tests/test_panel_prompt.py b/scripts/tests/test_panel_prompt.py index b830c39..46fe4bd 100644 --- a/scripts/tests/test_panel_prompt.py +++ b/scripts/tests/test_panel_prompt.py @@ -29,6 +29,7 @@ from pathlib import Path import pytest +from conftest import require_kit_paths sys.path.insert(0, str(Path(__file__).resolve().parent)) from _repo_layout import engine_dir, find_repo_root # noqa: E402 @@ -37,22 +38,26 @@ ENGINE = engine_dir(Path(__file__).resolve()) / "panel_prompt.py" DOCTRINE = Path("docs") / "agentic-dev-kit" / "fallback-review-panel.md" -# The whole module is conditioned on the shipped doctrine, not just the tests -# that read it: `panel_prompt.py` QUOTES its contract out of this file at run -# time and exits 2 rather than guessing when it cannot, so an engine installed -# without the doctrine is non-functional by design and there is nothing here -# worth asserting about it. That an adopter can reach that state at all is -# #226's second half — `/adopt` Step 3 installs three files from -# `docs/agentic-dev-kit/` and this is not one of them — and it stays open there. +# NOT a module-level marker. An earlier version marked the whole module on the +# doctrine, reasoning that `panel_prompt.py` is non-functional without it. That +# over-reached: 15 test cases here never read the shipped file — they parse +# synthetic doctrines they write themselves, or exercise `_repo_slug()`, a pure +# string function with three prior lens-found bugs. Marking the module skipped +# all of them in exactly the tree #226 says `/adopt` produces, losing coverage +# of shipped parsing logic that such a tree still has. Both lenses, PR #232 +# round 1. # -# A string LITERAL, not `str(DOCTRINE)`, so `test_kit_repo_only.py` can find it -# by scanning the source; the test below pins the two spellings together. -pytestmark = pytest.mark.kit_repo_only("docs/agentic-dev-kit/fallback-review-panel.md") +# The dependency is instead declared where it actually arises — in +# `doctrine_text()`, which the `repo` fixture and one test call — so a new test +# inherits it by using the fixture rather than by remembering a decorator. -def test_the_marker_path_matches_the_doctrine_path(): - """Without this, a doctrine rename would leave the marker naming a file that - no longer exists, and skip this whole module everywhere — silently.""" +def test_the_declared_path_matches_the_doctrine_path(): + """`doctrine_text()` names the path as a string LITERAL so + `test_kit_repo_only.py` can find it by scanning the source; `DOCTRINE` is a + `Path` built separately. Two spellings of one path drift, so this pins them + — without it a doctrine rename would leave the requirement naming a file + that no longer exists, and skip every test using it, silently.""" assert str(DOCTRINE) == "docs/agentic-dev-kit/fallback-review-panel.md" @@ -69,6 +74,7 @@ def doctrine_text() -> str: test body and one inside another fixture, and a fixture would thread a parameter through call sites that need nothing else. """ + require_kit_paths("docs/agentic-dev-kit/fallback-review-panel.md") return (REPO_ROOT / DOCTRINE).read_text(encoding="utf-8") From 3a826ba05f914e8861b704da45f14313de826570 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 15:07:04 +0300 Subject: [PATCH 3/4] fix: withdraw the no-.git skip guard, and stop a malformed manifest aborting collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round 2. Both lenses independently found the same HIGH, and it was a regression introduced by round 1's own fix. **`ROOT_IS_RESOLVED` is withdrawn.** Round 1 disabled skipping outright whenever no `.git` marker was found, reasoning that an unresolved root makes the skip's "not vendored in this tree" a confident false claim. True for the NESTED layout, where the fallback root is one level short. But the fallback is CORRECT for the flat layout, so the guard broke a case that worked: a tarball export of a genuinely sized-down tree went from an accurate skip to a raw FileNotFoundError. That is #134's own harm class, for a different population, and both lenses built the tree and measured it. Withdrawn rather than patched, and the reasoning is worth stating because this PR declared a threshold for it: `ROOT_IS_RESOLVED` was itself a round-1 addition prompted by a lens finding — the "new mechanism, however squarely a finding prompted it" the doctrine says to file rather than build. So the thing that came out is that guard, not the marker mechanism. What replaces it is a narrowing of the existing existence check rather than another guard: when the root is a guess, BOTH candidate roots are searched, and a path found under either counts as present. It can never claim "not vendored" about a file that exists at either plausible root, and it skips correctly when the file is absent from both. Verified in both directions — flat/absent now skips, nested/present now runs. The underlying root ambiguity is #233. **A malformed manifest could abort collection (correctness, MED).** `_is_complete_kit_tree()` did `.get("files")` on whatever the manifest parsed to. `[1, 2, 3]` is valid JSON, so that raised AttributeError — at MODULE scope, since the value feeds a `skipif` — and the whole session ran zero tests. That is #226's exact failure class reproduced inside the fix for it. Now shape-checked, with every malformed form pinned. **The completeness guard could still fire on a legitimate tree (adversarial, MED).** `kit-manifest.json` tracks neither `init.sh` nor the root `Makefile`, so a manifest-complete tree missing one was called complete, and the positive control then failed over a legitimately absent file — round 1's HIGH narrowed rather than closed. Both are now part of the conjunction. **Three round-1 fixes had no regression coverage (both lenses, MED).** Reverting the `iter_markers` union, dropping the second candidate root, and dropping the `init.sh`/`Makefile` conjunction each left the whole suite green, because the kit's own repo has every path present so the branches are indistinguishable here. All three now have synthetic-tree tests, and all four mutations are killed. **A comment named the wrong file (both lenses, LOW).** The `iter_markers` rationale cited `test_portability.py`, which has no module-level marker and no `docs/templates` reference. The case is `test_init_sh.py`'s six seeding tests. Corrected — and it sat in the same docstring block round 2 was already fixing a false claim in. Re-measured, each tree from the committed state: | tree | before this PR | now | |-----------------------------------|-------------------------|--------------------------| | kit's own repo | 699 passed | 720 passed, 0 skipped | | by-the-book /adopt (no doctrine) | 0 run, collection abort | 671 passed, 49 skipped | | engines+config floor | 90 failed, 1 error | 579 passed, 141 skipped | Both vendored trees 0 failed. make test -> 720 passed. ruff check clean. --- scripts/tests/conftest.py | 43 ++++++---- scripts/tests/test_kit_repo_only.py | 120 +++++++++++++++++++++++++++- 2 files changed, 144 insertions(+), 19 deletions(-) diff --git a/scripts/tests/conftest.py b/scripts/tests/conftest.py index 626f6ca..56c105b 100644 --- a/scripts/tests/conftest.py +++ b/scripts/tests/conftest.py @@ -36,18 +36,28 @@ ENGINE_DIR = engine_dir(Path(__file__)) REPO_ROOT = find_repo_root(ENGINE_DIR) -# Whether a `.git` marker was actually FOUND, as opposed to `find_repo_root` -# falling back to `start.parent`. The fallback is right only when the engines -# sit directly under the root, and is one level short in the `scripts/devkit/` -# layout `/adopt` defaults to (#60, pinned in `test_repo_layout.py`). +# The roots a path may be looked up under. Normally one: the resolved root. # -# It matters here and nowhere else in the suite: a wrong root used to surface as -# a loud `FileNotFoundError` from a test body, and the skip below would convert -# that into `not vendored in this tree` — a confident, wrong claim about a file -# that is present. So when the marker is absent the skip does not run at all, -# and the pre-existing loud failure is preserved. Adversarial and correctness -# lenses, PR #232 round 1. -ROOT_IS_RESOLVED = any((c / ".git").exists() for c in (ENGINE_DIR, *ENGINE_DIR.parents)) +# With no `.git` anywhere, `find_repo_root` falls back to `start.parent`, which +# is right when the engines sit directly under the root and one level short in +# the `scripts/devkit/` layout `/adopt` defaults to (#60, pinned in +# `test_repo_layout.py`) — and nothing here can tell those apart. So when the +# root is a guess, BOTH candidates count, and a path found under either is +# treated as present. +# +# The direction is chosen. A missing skip leaves the pre-existing loud +# `FileNotFoundError` from the test body, which is merely unhelpful; a wrongly +# fired skip claims `not vendored in this tree` about a file that is right +# there, which is a confident false statement to an adopter. +# +# Round 1 addressed the same finding by disabling the skip outright whenever no +# `.git` was found. That was withdrawn in round 2: the fallback root is CORRECT +# for the flat layout, so disabling the skip broke a case that worked — a +# tarball export of a genuinely sized-down tree went from an accurate skip to a +# failure, which is #134's own harm class. Adversarial lens, PR #232 rounds 1 +# and 2. #233 holds the underlying root ambiguity. +_ROOT_FOUND = any((c / ".git").exists() for c in (ENGINE_DIR, *ENGINE_DIR.parents)) +SEARCH_ROOTS = (REPO_ROOT,) if _ROOT_FOUND else (REPO_ROOT, REPO_ROOT.parent) def require_kit_paths(*paths: str) -> None: @@ -64,9 +74,11 @@ def require_kit_paths(*paths: str) -> None: def _skip_if_missing(paths, prefix: str) -> None: - if not paths or not ROOT_IS_RESOLVED: + if not paths: return - missing = [rel for rel in paths if not (REPO_ROOT / rel).exists()] + missing = [ + rel for rel in paths if not any((root / rel).exists() for root in SEARCH_ROOTS) + ] if missing: pytest.skip(f"{prefix}: not vendored in this tree: {', '.join(missing)}") @@ -128,8 +140,9 @@ def pytest_runtest_setup(item) -> None: whose typo could go unnoticed in a tree that is otherwise incomplete. """ # `iter_markers`, not `get_closest_marker`: a function-level marker must ADD - # to a module-level one rather than replace it. `test_portability.py`'s - # init.sh tests that also need `docs/templates` are exactly that case, and + # to a module-level one rather than replace it. `test_init_sh.py`'s six + # seeding tests, which need `docs/templates` on top of the module's + # `init.sh`, are exactly that case, and # with `get_closest_marker` the function marker silently dropped the # module's `init.sh` requirement. Correctness lens, PR #232 round 1. paths = sorted({rel for m in item.iter_markers("kit_repo_only") for rel in m.args}) diff --git a/scripts/tests/test_kit_repo_only.py b/scripts/tests/test_kit_repo_only.py index 906f2f6..82fd56d 100644 --- a/scripts/tests/test_kit_repo_only.py +++ b/scripts/tests/test_kit_repo_only.py @@ -103,6 +103,33 @@ def test_one_absent_path_among_several_is_enough_to_skip(tmp_path): assert "init.sh" not in reason, reason +def test_a_function_marker_adds_to_a_module_marker_rather_than_replacing_it(tmp_path): + """`get_closest_marker` returns only the nearest, so a function-level marker + silently dropped the module's requirement — `test_init_sh.py`'s six seeding + tests need `docs/templates` on top of the module's `init.sh`, and with the + closest-marker form they ran in a tree with no `init.sh`. + + Fixed in round 1 by unioning `iter_markers`, and pinned here in round 2: + reverting to `get_closest_marker` left the whole suite green, because the + only real instance has both paths present in the kit's own repo either way. + Adversarial lens, PR #232 round 2. + """ + body = ( + "import pytest\n\n" + 'pytestmark = pytest.mark.kit_repo_only("init.sh")\n\n\n' + '@pytest.mark.kit_repo_only("docs/templates")\n' + "def test_probe():\n assert True\n" + ) + root = _tree(tmp_path, body) + # The FUNCTION's path is present and the MODULE's is not: under + # `get_closest_marker` this runs, under the union it skips. + (root / "docs" / "templates").mkdir(parents=True) + out = _run(root) + assert "1 skipped" in out.stdout, out.stdout + reason = next(ln for ln in out.stdout.splitlines() if "not vendored" in ln) + assert "init.sh" in reason, reason + + def test_a_directory_counts_as_present(tmp_path): """`docs/templates` is named as a directory by `test_kitconfig.py`, so the check has to be `exists()` rather than `is_file()`.""" @@ -112,6 +139,75 @@ def test_a_directory_counts_as_present(tmp_path): assert "1 passed" in out.stdout, out.stdout +def test_a_path_at_the_parent_of_a_guessed_root_counts_as_present(tmp_path): + """With no `.git` anywhere, the resolved root is a guess — right for a flat + layout, one level short for the nested one `/adopt` defaults to (#233). Both + candidates are searched, so a present file is never called `not vendored`. + + Round 1 handled this by disabling the skip whenever `.git` was absent, which + broke the flat sized-down case (see the test below). Adversarial lens, + PR #232 rounds 1 and 2. + """ + root = tmp_path / "adopter" + vendored = root / "scripts" / "devkit" / "tests" + vendored.mkdir(parents=True) + for name in ("conftest.py", "_repo_layout.py"): + shutil.copy(TESTS_DIR / name, vendored / name) + (vendored / "test_probe.py").write_text(PROBE.format(paths='"init.sh"'), encoding="utf-8") + # No `.git`, and `init.sh` at the TRUE root — one above the guessed one. + (root / "init.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + out = _run(root) + assert "1 passed" in out.stdout, out.stdout + + +def test_a_flat_tree_with_no_git_still_skips_a_genuinely_absent_path(tmp_path): + """The other direction, and the one round 1 broke: a flat layout's guessed + root is CORRECT, so a genuinely missing file must still skip rather than + fail. Disabling the skip on `.git` absence turned an accurate skip into a + failure for a tarball export of a sized-down tree — #134's own harm class.""" + root = tmp_path / "adopter" + vendored = root / "scripts" / "tests" + vendored.mkdir(parents=True) + for name in ("conftest.py", "_repo_layout.py"): + shutil.copy(TESTS_DIR / name, vendored / name) + (vendored / "test_probe.py").write_text(PROBE.format(paths='"init.sh"'), encoding="utf-8") + out = subprocess.run( + [sys.executable, "-m", "pytest", "scripts/tests", "-q", "--no-header", "-rs"], + cwd=root, capture_output=True, text=True, + ) + assert "1 skipped" in out.stdout, out.stdout + + +def test_the_completeness_guard_rejects_a_tree_missing_an_untracked_kit_file(tmp_path): + """`kit-manifest.json` tracks neither `init.sh` nor the root `Makefile`, so + manifest-completeness alone said True for a tree missing one — and the + positive control then ran and FAILED over a legitimately absent file, which + is round 1's HIGH narrowed rather than closed. Adversarial lens, round 2.""" + root = tmp_path / "kitish" + (root / "scripts").mkdir(parents=True) + (root / "scripts" / "engine.py").write_text("x = 1\n", encoding="utf-8") + (root / "kit-manifest.json").write_text( + json.dumps({"files": {"scripts/engine.py": {"sha256": "0" * 64}}}), encoding="utf-8" + ) + (root / "Makefile").write_text("test:\n", encoding="utf-8") + assert _is_complete_kit_tree(root) is False, "manifest-complete but no init.sh" + (root / "init.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + assert _is_complete_kit_tree(root) is True + + +@pytest.mark.parametrize( + "body", ["[1, 2, 3]", '"a string"', "null", "{garbage not json", '{"files": []}'] +) +def test_a_manifest_of_any_shape_degrades_rather_than_aborting(tmp_path, body): + """This predicate feeds a `skipif`, so it runs at MODULE scope — anything it + raises aborts collection and runs zero tests, which is exactly #226. Every + malformed shape must return False, never raise.""" + root = tmp_path / "tree" + root.mkdir() + (root / "kit-manifest.json").write_text(body, encoding="utf-8") + assert _is_complete_kit_tree(root) is False + + def test_an_unmarked_test_is_untouched(tmp_path): root = _tree(tmp_path, "def test_probe():\n assert True\n") out = _run(root) @@ -144,7 +240,7 @@ def _marked_paths() -> set[str]: return found -def _is_complete_kit_tree() -> bool: +def _is_complete_kit_tree(root: Path = None) -> bool: """Whether this tree holds every file `kit-manifest.json` says the kit ships. The scope guard the check below needs, and DERIVED rather than a judgement @@ -153,14 +249,30 @@ def _is_complete_kit_tree() -> bool: in any sized-down tree, where a marked path being absent is the mechanism working rather than a defect. """ - manifest = REPO_ROOT / "kit-manifest.json" + root = REPO_ROOT if root is None else root + manifest = root / "kit-manifest.json" if not manifest.is_file(): return False try: - files = json.loads(manifest.read_text(encoding="utf-8")).get("files", {}) + parsed = json.loads(manifest.read_text(encoding="utf-8")) except (json.JSONDecodeError, OSError): return False - return bool(files) and all((REPO_ROOT / rel).exists() for rel in files) + # `[1, 2, 3]` is valid JSON. Calling `.get` on it raised `AttributeError` + # here — at MODULE scope, since this feeds a `skipif` — so collection + # aborted and the whole session ran zero tests. That is #226's failure class + # reproduced inside the fix for it. Correctness lens, PR #232 round 2. + if not isinstance(parsed, dict): + return False + files = parsed.get("files") + if not isinstance(files, dict): + return False + if not files or not all((root / rel).exists() for rel in files): + return False + # The manifest tracks neither of these, so a manifest-complete tree could + # still be missing one — and then the control below would run and FAIL over + # a legitimately absent file, which is the round-1 defect narrowed rather + # than closed. Adversarial lens, PR #232 round 2. + return all((root / rel).exists() for rel in ("init.sh", "Makefile")) def test_the_marker_scan_finds_something(): From c6fd3a1988f47b725502e4570f2debe480ce55d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 15:33:47 +0300 Subject: [PATCH 4/4] fix: stop guessing the repo root, and make a second module-scope read lazy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round 3. The headline is a pattern rather than any single defect. **Root resolution without `.git` has now drawn a finding in three consecutive rounds, each in the previous round's fix:** round 1 — disable skipping when no `.git` is found. Broke the FLAT sized-down tarball case, which had been skipping correctly. round 2 — search `REPO_ROOT` and its parent. Still wrong at nesting depth > 1 (`tools/internal/devkit/` gave a confident `not vendored` about a file present at the true root — worse than round 1 on that tree); and in the flat layout the second candidate sits OUTSIDE the repo, so a same-named file one directory up suppressed a skip that should have fired. Both reproduced by both lenses. round 3 — this. No guess at all. That is this PR's declared withdrawal threshold, and the #225 pattern: each patch was a fresh guess at a value that is not derivable from the information available, and each opened a hole in a different tree shape. What ships searches the one resolved root and, when no `.git` was found, SAYS the root was a guess: not vendored in this tree: init.sh (repo root unresolved — no .git above ; see #233) That is not a false claim — it states exactly what was checked — and it keeps the clean run a sized-down adopter is owed. #233 records all three attempts so a fourth is not made here. **Invalid UTF-8 in the manifest aborted collection (adversarial, HIGH).** `_is_complete_kit_tree` caught `(json.JSONDecodeError, OSError)`, but a stray byte raises `UnicodeDecodeError` — a `ValueError`, caught by neither. At module scope, feeding a `skipif`, so the whole session ran zero tests: #226's failure class inside the fix for #226, for the second round running. Now `ValueError`. Round 2's claim that "every malformed form" was pinned was false, and structurally so: `write_text` on a `str` can only emit valid UTF-8, so no parametrized case could ever reach that branch. **`isinstance(files, dict)` was a surviving mutant (both lenses).** The one case aimed at it, `{"files": []}`, is caught earlier by the truthiness check. Added `{"files": ["init.sh", "Makefile"]}` — truthy and not a dict, the only shape that reaches it. Without the guard that input raises `TypeError` on `PosixPath / int`, at module scope again. **A second module-scope read could abort collection (correctness, HIGH).** `test_init_sh.py` read `config/dev-model.yaml` at import. Wherever `REPO_ROOT` resolves wrong this raised during collection and took unrelated modules down with it — the marker cannot help, because the exception precedes it. Now lazy, the same shape as #226's fix in `test_panel_prompt.py`. **Scope stated rather than implied.** A no-`.git` tree STILL aborts collection, for a reason that predates this PR: `test_portability.py`, `test_mutation_gate.py` and `test_pr_followup_hook.py` carry private walk-up helpers that raise instead of falling back — `_repo_layout.py`'s own docstring names this and says it is "not the form to copy". #203 owns consolidating them, and that is a prerequisite for a no-`.git` tree collecting. This PR removes one contributor and claims nothing about that case; every tree it measures has `.git`, which every real adopter repository has by definition. Measured and recorded on #233. | tree | before this PR | now | |-----------------------------------|-------------------------|--------------------------| | kit's own repo | 699 passed | 721 passed, 0 skipped | | by-the-book /adopt (no doctrine) | 0 run, collection abort | 672 passed, 49 skipped | | engines+config floor | 90 failed, 1 error | 580 passed, 141 skipped | Both vendored trees 0 failed. make test -> 721 passed. ruff check clean. --- scripts/tests/conftest.py | 46 ++++++++++--------- scripts/tests/test_init_sh.py | 59 ++++++++++++++---------- scripts/tests/test_kit_repo_only.py | 70 ++++++++++++++++++----------- 3 files changed, 102 insertions(+), 73 deletions(-) diff --git a/scripts/tests/conftest.py b/scripts/tests/conftest.py index 56c105b..2de8900 100644 --- a/scripts/tests/conftest.py +++ b/scripts/tests/conftest.py @@ -36,28 +36,32 @@ ENGINE_DIR = engine_dir(Path(__file__)) REPO_ROOT = find_repo_root(ENGINE_DIR) -# The roots a path may be looked up under. Normally one: the resolved root. +# Whether a `.git` marker was actually found. With none, `find_repo_root` falls +# back to `start.parent`, which is right for a flat layout and short by one or +# more levels for a nested one — and nothing here can tell those apart, because +# the only signal that would is exactly what is missing. # -# With no `.git` anywhere, `find_repo_root` falls back to `start.parent`, which -# is right when the engines sit directly under the root and one level short in -# the `scripts/devkit/` layout `/adopt` defaults to (#60, pinned in -# `test_repo_layout.py`) — and nothing here can tell those apart. So when the -# root is a guess, BOTH candidates count, and a path found under either is -# treated as present. +# THREE ATTEMPTS TO BE CLEVER ABOUT THIS WERE WITHDRAWN, one per review round, +# and the next one should not be made here: # -# The direction is chosen. A missing skip leaves the pre-existing loud -# `FileNotFoundError` from the test body, which is merely unhelpful; a wrongly -# fired skip claims `not vendored in this tree` about a file that is right -# there, which is a confident false statement to an adopter. +# round 1 — disable skipping entirely when no `.git` was found. Broke the FLAT +# sized-down tarball case, which had been skipping correctly: an +# accurate skip became a raw FileNotFoundError, #134's own harm class. +# round 2 — search both `REPO_ROOT` and its parent. Still wrong at nesting +# depth > 1 (`tools/internal/devkit/`), where it emitted a confident +# `not vendored` about a file present at the true root; and in the +# flat case the second candidate sits OUTSIDE the tree, so a +# same-named file above it suppressed a skip that should have fired. +# round 3 — this. No guess at all. # -# Round 1 addressed the same finding by disabling the skip outright whenever no -# `.git` was found. That was withdrawn in round 2: the fallback root is CORRECT -# for the flat layout, so disabling the skip broke a case that worked — a -# tarball export of a genuinely sized-down tree went from an accurate skip to a -# failure, which is #134's own harm class. Adversarial lens, PR #232 rounds 1 -# and 2. #233 holds the underlying root ambiguity. +# The root is unknowable without `.git`, so the skip no longer pretends +# otherwise: it searches the one resolved root and SAYS the root was a guess. +# That is not a false claim — it states exactly what was checked — and it keeps +# the clean run a sized-down adopter is owed. #233 holds the resolution. _ROOT_FOUND = any((c / ".git").exists() for c in (ENGINE_DIR, *ENGINE_DIR.parents)) -SEARCH_ROOTS = (REPO_ROOT,) if _ROOT_FOUND else (REPO_ROOT, REPO_ROOT.parent) +_UNRESOLVED = ( + "" if _ROOT_FOUND else f" (repo root unresolved — no .git above {ENGINE_DIR}; see #233)" +) def require_kit_paths(*paths: str) -> None: @@ -76,11 +80,9 @@ def require_kit_paths(*paths: str) -> None: def _skip_if_missing(paths, prefix: str) -> None: if not paths: return - missing = [ - rel for rel in paths if not any((root / rel).exists() for root in SEARCH_ROOTS) - ] + missing = [rel for rel in paths if not (REPO_ROOT / rel).exists()] if missing: - pytest.skip(f"{prefix}: not vendored in this tree: {', '.join(missing)}") + pytest.skip(f"{prefix}: not vendored in this tree: {', '.join(missing)}{_UNRESOLVED}") def pytest_configure(config) -> None: diff --git a/scripts/tests/test_init_sh.py b/scripts/tests/test_init_sh.py index 58d0f9b..8fdbd0e 100644 --- a/scripts/tests/test_init_sh.py +++ b/scripts/tests/test_init_sh.py @@ -46,7 +46,18 @@ REPO_ROOT = find_repo_root(ENGINE_DIR) sys.path.insert(0, str(ENGINE_DIR / "lib")) -SHIPPED_CONFIG = (REPO_ROOT / "config" / "dev-model.yaml").read_text(encoding="utf-8") +def shipped_config() -> str: + """The kit's own `config/dev-model.yaml`, read at CALL time. + + This was a module-scope `read_text()`. Wherever `REPO_ROOT` resolves wrong — + a nested engines directory in a tree with no `.git`, which #233 records as + not resolvable — it raised during COLLECTION, so pytest aborted the whole + session and ran zero tests, taking unrelated modules down with it. That is + #226's failure class in a second module, and the reason the `kit_repo_only` + marker cannot help: the exception fires at import, long before any marker is + consulted. Correctness lens, PR #232 round 3. + """ + return (REPO_ROOT / "config" / "dev-model.yaml").read_text(encoding="utf-8") # A v1-schema config with no `paths.engines`, so a run must call # detect_engines_dir() to stamp it — the same shape test_portability.py migrates. @@ -270,7 +281,7 @@ def _shipped_with_name(name_value: str) -> str: pattern = re.compile(r"^ name: .*$", re.M) # No `count=` cap: capping at 1 would make the assertion below unable to fire # on a duplicate, which is exactly what it claims to guard (panel, correctness). - replaced, count = pattern.subn(lambda _m: f" name: {name_value}", SHIPPED_CONFIG) + replaced, count = pattern.subn(lambda _m: f" name: {name_value}", shipped_config()) assert count == 1, ( f"expected exactly one ` name:` line under project: in the shipped config, found {count}" ) @@ -283,7 +294,7 @@ def _shipped_with_tracker_url(url_value: str) -> str: pattern = re.compile(r"^ url: .*$", re.M) # No `count=` cap: capping at 1 would make the assertion below unable to fire # on a duplicate, which is exactly what it claims to guard (panel, correctness). - replaced, count = pattern.subn(lambda _m: f" url: {url_value}", SHIPPED_CONFIG) + replaced, count = pattern.subn(lambda _m: f" url: {url_value}", shipped_config()) assert count == 1, ( f"expected exactly one ` url:` line under tracker: in the shipped config, found {count}" ) @@ -295,11 +306,11 @@ def test_rerun_on_shipped_config_preserves_every_value_and_is_stable(tmp_path: P and a further re-run must be byte-identical (the documented upgrade path). The bots byte-assertion pins the quoted-item list serialization — value equality alone let a revert to unquoted items survive (panel, #87).""" - repo = _fixture(tmp_path, config=SHIPPED_CONFIG) + repo = _fixture(tmp_path, config=shipped_config()) _run_init(repo) once = _config(repo) - assert yaml.safe_load(once) == yaml.safe_load(SHIPPED_CONFIG) + assert yaml.safe_load(once) == yaml.safe_load(shipped_config()) assert 'bots: ["coderabbit"]' in once _run_init(repo) @@ -463,7 +474,7 @@ def test_rerun_normalizes_single_quoted_bots_item(tmp_path: Path) -> None: """A hand-written `bots: ['coderabbit']` is valid YAML naming the reviewer `coderabbit` — the double-quote-only strip re-serialized it as the literal name `'coderabbit'`, which pr_watch silently fails to match (panel, #87).""" - config = SHIPPED_CONFIG.replace(" bots: [coderabbit]", " bots: ['coderabbit']") + config = shipped_config().replace(" bots: [coderabbit]", " bots: ['coderabbit']") assert " bots: ['coderabbit']" in config repo = _fixture(tmp_path, config=config) @@ -491,7 +502,7 @@ def test_rerun_drops_yaml_significant_chars_from_bots_items(tmp_path: Path) -> N """A quote or backslash inside a bots item would corrupt the whole flow list when re-wrapped; such characters cannot appear in a real bot handle and are dropped so the config stays loadable (CodeRabbit on #87).""" - config = SHIPPED_CONFIG.replace(' bots: [coderabbit]', ' bots: ["a\\"b"]') + config = shipped_config().replace(' bots: [coderabbit]', ' bots: ["a\\"b"]') assert ' bots: ["a\\"b"]' in config repo = _fixture(tmp_path, config=config) @@ -611,7 +622,7 @@ def test_set_field_writes_backslashes_literally(tmp_path: Path) -> None: @pytest.mark.kit_repo_only("docs/templates") def test_seeds_narrative_docs_with_tokens_rendered(tmp_path: Path) -> None: - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, templates=True) + repo = _fixture(tmp_path, config=shipped_config(), templates=True) _run_init(repo) @@ -637,7 +648,7 @@ def test_seeds_narrative_docs_with_tokens_rendered(tmp_path: Path) -> None: # under test — not to a literal. Asserting `my-project` coupled this to the # config being unstamped, so it failed the moment this repo (or any adopter) # set its own project name, which is not what this test is about. - configured_name = yaml.safe_load(SHIPPED_CONFIG)["project"]["name"] + configured_name = yaml.safe_load(shipped_config())["project"]["name"] assert configured_name, "shipped config has no project.name to render" assert configured_name in seeded["docs/kit-handoff.md"] # {{PROJECT_NAME}} # Read the engines dir from the config under test for the same reason @@ -645,7 +656,7 @@ def test_seeds_narrative_docs_with_tokens_rendered(tmp_path: Path) -> None: # bare "scripts/" pins this to one layout, and it failed under the # `scripts/devkit` layout `/adopt` defaults to — where the token renders # correctly and the assertion was simply wrong about what correct is. - configured_engines = yaml.safe_load(SHIPPED_CONFIG)["paths"]["engines"] + configured_engines = yaml.safe_load(shipped_config())["paths"]["engines"] assert ( f"{configured_engines}/check_doc_budget.py" in seeded["docs/kit-handoff.md"] ) # {{ENGINE_DIR}} @@ -657,7 +668,7 @@ def test_seeds_narrative_docs_with_tokens_rendered(tmp_path: Path) -> None: # branch the config under test selects. The blank branch keeps its own test # below, so stamping a real URL here cannot silently delete that coverage — # which is exactly what happened when this assertion was a bare literal. - configured_url = yaml.safe_load(SHIPPED_CONFIG)["tracker"]["url"] + configured_url = yaml.safe_load(shipped_config())["tracker"]["url"] assert (configured_url or "tracker.url") in seeded["docs/kit-friction-log.md"] # AGENTS.md renders at the repo ROOT, so its handoff link is the repo-relative # configured path, not the sibling-relative form the narrative docs use. @@ -697,7 +708,7 @@ def test_render_preserves_backslashes_in_values(tmp_path: Path) -> None: @pytest.mark.kit_repo_only("docs/templates") def test_seeding_respects_in_use_docs_and_reclaims_marked_ones(tmp_path: Path) -> None: - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, templates=True) + repo = _fixture(tmp_path, config=shipped_config(), templates=True) (repo / "docs").mkdir(parents=True, exist_ok=True) in_use = repo / "docs" / "kit-handoff.md" in_use.write_text("# mine — hands off\n", encoding="utf-8") @@ -725,7 +736,7 @@ def test_agents_md_renders_the_configured_protected_branch(tmp_path: Path) -> No would pass with the substitution deleted: the template contains no literal `main`, so that assertion would have failed. The test is right; the reason given for it was not.)""" - config = SHIPPED_CONFIG.replace("protected_branch: main", "protected_branch: trunk-9f2a") + config = shipped_config().replace("protected_branch: main", "protected_branch: trunk-9f2a") repo = _fixture(tmp_path, config=config, templates=True) _run_init(repo) @@ -748,7 +759,7 @@ def test_seeding_leaves_a_doc_that_merely_quotes_the_marker_untouched( green while init.sh overwrote a doc whose line 2 quotes the marker. The read-only reporter got this case first; the file-destroying one had it open two rounds longer (panel round 5).""" - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, templates=True) + repo = _fixture(tmp_path, config=shipped_config(), templates=True) mine = repo / "AGENTS.md" lines = ["# AGENTS.md — hand written", "", "still hand written", ""] lines[marker_line - 1] = "The kit marks skeletons `devkit-template: unrendered` on line 1." @@ -784,7 +795,7 @@ def test_kit_ships_no_root_agents_md(tmp_path: Path) -> None: def test_gitignore_entries_added_exactly_once_across_reruns(tmp_path: Path) -> None: - repo = _fixture(tmp_path, config=SHIPPED_CONFIG) + repo = _fixture(tmp_path, config=shipped_config()) _run_init(repo) _run_init(repo) @@ -811,12 +822,12 @@ def test_gitignore_gains_mcp_json_only_for_literal_credentials(tmp_path: Path) - ignored, a ${ENV} reference leaves it tracked. The sniff itself misses the kit's own documented hyphenated shape (CF-Access-Client-Id) — #86 tracks that; this green pins the guard that exists, not sufficiency.""" - literal = _fixture(tmp_path / "literal", config=SHIPPED_CONFIG) + literal = _fixture(tmp_path / "literal", config=shipped_config()) (literal / ".mcp.json").write_text('{"CF_TOKEN": "abc123"}', encoding="utf-8") _run_init(literal) assert ".mcp.json" in (literal / ".gitignore").read_text(encoding="utf-8").splitlines() - envref = _fixture(tmp_path / "envref", config=SHIPPED_CONFIG) + envref = _fixture(tmp_path / "envref", config=shipped_config()) (envref / ".mcp.json").write_text('{"CF_TOKEN": "${CF_TOKEN}"}', encoding="utf-8") _run_init(envref) assert ".mcp.json" not in (envref / ".gitignore").read_text(encoding="utf-8").splitlines() @@ -828,7 +839,7 @@ def test_gitignore_gains_mcp_json_only_for_literal_credentials(tmp_path: Path) - def test_installs_pre_push_shim_into_git_hooks(tmp_path: Path) -> None: - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, git=True, hooks=True) + repo = _fixture(tmp_path, config=shipped_config(), git=True, hooks=True) _run_init(repo) @@ -841,12 +852,12 @@ def test_installs_pre_push_shim_into_git_hooks(tmp_path: Path) -> None: # a literal `scripts/` (#134). Under `paths.engines: scripts/devkit` the # generated shim correctly reads `scripts/devkit/hooks/pre-push`, and the # literal form of this assertion failed on a shim that was right. - configured_engines = yaml.safe_load(SHIPPED_CONFIG)["paths"]["engines"] + configured_engines = yaml.safe_load(shipped_config())["paths"]["engines"] assert f"{configured_engines}/hooks/pre-push" in body def test_hook_shim_honors_repo_local_hookspath(tmp_path: Path) -> None: - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, git=True, hooks=True) + repo = _fixture(tmp_path, config=shipped_config(), git=True, hooks=True) subprocess.run( ["git", "config", "core.hooksPath", ".githooks"], cwd=repo, @@ -862,7 +873,7 @@ def test_hook_shim_honors_repo_local_hookspath(tmp_path: Path) -> None: def test_existing_non_shim_hook_left_untouched(tmp_path: Path) -> None: - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, git=True, hooks=True) + repo = _fixture(tmp_path, config=shipped_config(), git=True, hooks=True) hookdir = repo / ".git" / "hooks" hookdir.mkdir(parents=True, exist_ok=True) own = "#!/bin/sh\n# the adopter's own hook\n" @@ -881,7 +892,7 @@ def test_gitignore_append_preserves_a_file_with_no_trailing_newline(tmp_path: Pa The fix shipped without a test and its mutant survived the whole suite (panel, adversarial lens) — this is that test.""" - repo = _fixture(tmp_path, config=SHIPPED_CONFIG) + repo = _fixture(tmp_path, config=shipped_config()) (repo / ".gitignore").write_text("node_modules/\n.env", encoding="utf-8") _run_init(repo) @@ -899,7 +910,7 @@ def test_non_interactive_run_refuses_to_inherit_a_foreign_tracker(tmp_path: Path Fires only when an origin remote exists and disagrees, so the kit's own repo and every fixture here (no remote) are unaffected.""" - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, git=True) + repo = _fixture(tmp_path, config=shipped_config(), git=True) subprocess.run( ["git", "remote", "add", "origin", "https://github.com/acme/widgets.git"], cwd=repo, check=True, capture_output=True, env=_env(tmp_path), @@ -918,7 +929,7 @@ def test_non_interactive_run_is_unaffected_without_an_origin_remote(tmp_path: Pa """The guard must not fire for the kit's own repo or a fresh copy-in — both reach init.sh before any remote exists. Pins the guard's narrowness, which is what keeps it from wedging the documented install path.""" - repo = _fixture(tmp_path, config=SHIPPED_CONFIG, git=True) + repo = _fixture(tmp_path, config=shipped_config(), git=True) _run_init(repo) # check=True — a non-zero exit fails here diff --git a/scripts/tests/test_kit_repo_only.py b/scripts/tests/test_kit_repo_only.py index 82fd56d..375a614 100644 --- a/scripts/tests/test_kit_repo_only.py +++ b/scripts/tests/test_kit_repo_only.py @@ -139,33 +139,16 @@ def test_a_directory_counts_as_present(tmp_path): assert "1 passed" in out.stdout, out.stdout -def test_a_path_at_the_parent_of_a_guessed_root_counts_as_present(tmp_path): - """With no `.git` anywhere, the resolved root is a guess — right for a flat - layout, one level short for the nested one `/adopt` defaults to (#233). Both - candidates are searched, so a present file is never called `not vendored`. - - Round 1 handled this by disabling the skip whenever `.git` was absent, which - broke the flat sized-down case (see the test below). Adversarial lens, - PR #232 rounds 1 and 2. +def test_an_unresolved_root_says_so_rather_than_claiming_certainty(tmp_path): + """With no `.git` the root is a guess, and the skip must not pretend + otherwise. Three rounds of guessing at it were withdrawn (see `conftest.py`); + what ships states what was searched and points at #233. + + The reason text is the whole behaviour here — a skip that reads identically + to a resolved-root one is the confident false claim the withdrawals were + about. """ root = tmp_path / "adopter" - vendored = root / "scripts" / "devkit" / "tests" - vendored.mkdir(parents=True) - for name in ("conftest.py", "_repo_layout.py"): - shutil.copy(TESTS_DIR / name, vendored / name) - (vendored / "test_probe.py").write_text(PROBE.format(paths='"init.sh"'), encoding="utf-8") - # No `.git`, and `init.sh` at the TRUE root — one above the guessed one. - (root / "init.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") - out = _run(root) - assert "1 passed" in out.stdout, out.stdout - - -def test_a_flat_tree_with_no_git_still_skips_a_genuinely_absent_path(tmp_path): - """The other direction, and the one round 1 broke: a flat layout's guessed - root is CORRECT, so a genuinely missing file must still skip rather than - fail. Disabling the skip on `.git` absence turned an accurate skip into a - failure for a tarball export of a sized-down tree — #134's own harm class.""" - root = tmp_path / "adopter" vendored = root / "scripts" / "tests" vendored.mkdir(parents=True) for name in ("conftest.py", "_repo_layout.py"): @@ -176,6 +159,18 @@ def test_a_flat_tree_with_no_git_still_skips_a_genuinely_absent_path(tmp_path): cwd=root, capture_output=True, text=True, ) assert "1 skipped" in out.stdout, out.stdout + reason = next(ln for ln in out.stdout.splitlines() if "not vendored" in ln) + assert "repo root unresolved" in reason, reason + assert "#233" in reason, reason + + +def test_a_resolved_root_does_not_carry_the_unresolved_note(tmp_path): + """The other side: with `.git` present the root is known, so the caveat must + be absent — a caveat on every skip would be noise that stops being read.""" + root = _tree(tmp_path, PROBE.format(paths='"init.sh"')) + out = _run(root) + reason = next(ln for ln in out.stdout.splitlines() if "not vendored" in ln) + assert "repo root unresolved" not in reason, reason def test_the_completeness_guard_rejects_a_tree_missing_an_untracked_kit_file(tmp_path): @@ -196,7 +191,20 @@ def test_the_completeness_guard_rejects_a_tree_missing_an_untracked_kit_file(tmp @pytest.mark.parametrize( - "body", ["[1, 2, 3]", '"a string"', "null", "{garbage not json", '{"files": []}'] + "body", + [ + "[1, 2, 3]", + '"a string"', + "null", + "{garbage not json", + '{"files": []}', + # Truthy but not a dict — the only shape that reaches, and therefore + # pins, the `isinstance(files, dict)` guard. Without it `for rel in + # files` iterates the LIST's elements as manifest keys, and real path + # strings there would yield a confident wrong verdict rather than a + # rejection. Adversarial lens, PR #232 round 3. + '{"files": ["init.sh", "Makefile"]}', + ], ) def test_a_manifest_of_any_shape_degrades_rather_than_aborting(tmp_path, body): """This predicate feeds a `skipif`, so it runs at MODULE scope — anything it @@ -255,7 +263,15 @@ def _is_complete_kit_tree(root: Path = None) -> bool: return False try: parsed = json.loads(manifest.read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError): + except (ValueError, OSError): + # ValueError, not JSONDecodeError: invalid UTF-8 bytes raise + # UnicodeDecodeError, which is a ValueError and was NOT caught — so a + # manifest with one stray byte aborted collection at module scope, since + # this feeds a `skipif`. #226's failure class inside the fix for #226, + # and the round-2 claim that every malformed form was pinned was false: + # `write_text` on a `str` can only ever emit valid UTF-8, so the + # parametrized cases structurally could not reach it. Adversarial lens, + # PR #232 round 3. return False # `[1, 2, 3]` is valid JSON. Calling `.get` on it raised `AttributeError` # here — at MODULE scope, since this feeds a `skipif` — so collection