diff --git a/scripts/tests/conftest.py b/scripts/tests/conftest.py index 5b50020..2de8900 100644 --- a/scripts/tests/conftest.py +++ b/scripts/tests/conftest.py @@ -25,9 +25,68 @@ 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 + +ENGINE_DIR = engine_dir(Path(__file__)) +REPO_ROOT = find_repo_root(ENGINE_DIR) + +# 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. +# +# THREE ATTEMPTS TO BE CLEVER ABOUT THIS WERE WITHDRAWN, one per review round, +# and the next one should not be made here: +# +# 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. +# +# 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)) +_UNRESOLVED = ( + "" if _ROOT_FOUND else f" (repo root unresolved — no .git above {ENGINE_DIR}; see #233)" +) + + +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: + 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)}{_UNRESOLVED}") + 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 +100,52 @@ 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, 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. + """ + # `iter_markers`, not `get_closest_marker`: a function-level marker must ADD + # 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}) + _skip_if_missing(paths, "needs") diff --git a/scripts/tests/test_init_sh.py b/scripts/tests/test_init_sh.py index 5da4106..8fdbd0e 100644 --- a/scripts/tests/test_init_sh.py +++ b/scripts/tests/test_init_sh.py @@ -35,11 +35,29 @@ 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")) -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. @@ -263,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}" ) @@ -276,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}" ) @@ -288,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) @@ -456,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) @@ -484,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) @@ -602,8 +620,9 @@ 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) @@ -629,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 @@ -637,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}} @@ -649,13 +668,14 @@ 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. 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. @@ -673,6 +693,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 @@ -685,8 +706,9 @@ 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 = _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") @@ -703,6 +725,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 @@ -713,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) @@ -721,6 +744,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 @@ -735,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." @@ -771,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) @@ -798,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() @@ -815,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) @@ -828,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, @@ -849,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" @@ -868,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) @@ -886,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), @@ -905,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_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..375a614 --- /dev/null +++ b/scripts/tests/test_kit_repo_only.py @@ -0,0 +1,328 @@ +"""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 json +import re +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest +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: + """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_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()`.""" + 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_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" / "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 + 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): + """`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": []}', + # 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 + 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) + 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() + pattern = re.compile(r"(?:kit_repo_only|require_kit_paths)\(([^)]*)\)") + for module in sorted(TESTS_DIR.glob("test_*.py")): + 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(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 + 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. + """ + root = REPO_ROOT if root is None else root + manifest = root / "kit-manifest.json" + if not manifest.is_file(): + return False + try: + parsed = json.loads(manifest.read_text(encoding="utf-8")) + 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 + # 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(): + """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.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_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 (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_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..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 @@ -36,7 +37,45 @@ 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() + +# 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. +# +# 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_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" + + +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. + """ + require_kit_paths("docs/agentic-dev-kit/fallback-review-panel.md") + return (REPO_ROOT / DOCTRINE).read_text(encoding="utf-8") def _load(): @@ -67,7 +106,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 +178,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."""