feat: skip kit-repo-only tests instead of failing a sized-down adopter - #232
Conversation
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.
📝 WalkthroughWalkthroughThe test harness now resolves repository layouts and skips kit-specific tests when required paths are absent. Kit-only markers cover initialization, templates, dependency tooling, and portability tests. Doctrine loading is deferred until use, and subprocess tests validate marker behavior. ChangesKit test portability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… claim 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/tests/test_kit_doctor.py`:
- Line 610: Move the module-level kit_doctor imports in test_kit_doctor.py into
the relevant test bodies, fixtures, or loader so importing the test module does
not require scripts/kit_doctor.py. Preserve the existing
kit_repo_only("scripts/kit_doctor.py") markers, allowing pytest_runtest_setup to
skip the affected tests before any kit_doctor dependency is loaded.
In `@scripts/tests/test_kit_repo_only.py`:
- Around line 50-59: Update the subprocess.run call in _run to include an
explicit timeout value, ensuring a hung pytest invocation raises a failure
instead of blocking indefinitely while preserving the existing command and
output-capture behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a060c5c1-98e1-4a59-b888-2d1af125fb0b
📒 Files selected for processing (7)
scripts/tests/conftest.pyscripts/tests/test_init_sh.pyscripts/tests/test_kit_doctor.pyscripts/tests/test_kit_repo_only.pyscripts/tests/test_kitconfig.pyscripts/tests/test_panel_prompt.pyscripts/tests/test_portability.py
| return tmp_path | ||
|
|
||
|
|
||
| @pytest.mark.kit_repo_only("scripts/kit_doctor.py") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'test_kit_doctor.py' scripts/tests
sed -n '1,40p' scripts/tests/test_kit_doctor.py
rg -n 'import kit_doctor|from kit_doctor|kit_doctor\.' scripts/tests/test_kit_doctor.py | head -50Repository: topij/agentic-dev-kit
Length of output: 5324
🏁 Script executed:
#!/bin/bash
set -e
echo "== conftest relevant sections =="
rg -n "pytest_runtest_setup|kit_repo_only|Delete|collection abort|ModuleNotFoundError|ImportError" scripts/tests/scripts/tests scripts/tests -g 'conftest.py' -g 'test_kit_doctor.py' | head -120
echo
echo "== all kit_repo_only occurrences =="
rg -n "`@pytest.mark`\.kit_repo_only|def pytest_runtest_setup|kit_repo_only" scripts/tests | head -200
echo
echo "== outline test_kit_doctor around marker lines =="
ast-grep outline scripts/tests/test_kit_doctor.py --match 'def.*' --view compact | sed -n '90,140p'
echo
echo "== lines 590-725 =="
sed -n '590,725p' scripts/tests/test_kit_doctor.pyRepository: topij/agentic-dev-kit
Length of output: 11130
🏁 Script executed:
#!/bin/bash
set -e
echo "== scripts/tests/conftest.py relevant implementation =="
sed -n '80,145p' scripts/tests/conftest.py
echo
echo "== pytest availability and runtest_setup ordering docs probe =="
python3 - <<'PY'
import inspect
try:
import pytest
print("pytest_version:", pytest.__version__)
from _pytest import hooks
print("has hookspec:", hasattr(hooks, "PytestRuntestSetupHookspec"))
src = inspect.getsource(hooks.PytestRuntestSetupHookspec.pytest_runtest_setup)
print(src)
except Exception as e:
print("probe_error:", type(e).__name__, str(e))
PY
echo
echo "== test_kit_doctor import usage list =="
rg -n '^\s*import kit_doctor|^\s*from kit_doctor|kit_doctor\.' scripts/tests/test_kit_doctor.pyRepository: topij/agentic-dev-kit
Length of output: 12104
🏁 Script executed:
#!/bin/bash
set -e
echo "== repo root setup =="
rg -n "def find_repo_root|def engine_dir|sys\.path|_repo_layout" scripts/tests scripts -g'*.py' | head -80
echo
echo "== locate _repo_layout =="
fd -a '_repo_layout|repo_layout' .
echo
echo "== _repo_layout contents if present =="
while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f"
done < <(fd -a '_repo_layout\.py$|repo_layout\.py$' .)
echo
echo "== package metadata names =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*'):
if p.is_file() and p.name in {'pyproject.toml', 'setup.py', 'setup.cfg'}:
try:
txt=p.read_text(errors='ignore')
except Exception as e:
txt=str(type(e).__name__)+':'+str(e)
print(f'--- {p}')
for line in txt.splitlines()[:80]:
if any(s in line for s in ('agentic', 'dev-kit', 'package', 'script', 'paths', 'tool', 'test')):
print(line)
PYRepository: topij/agentic-dev-kit
Length of output: 5603
🏁 Script executed:
#!/bin/bash
set -e
echo "--- scripts/tests/_repo_layout.py ---"
sed -n '1,140p' scripts/tests/_repo_layout.py
echo "--- scripts/tests/conftest.py ---"
sed -n '1,90p' scripts/tests/conftest.pyRepository: topij/agentic-dev-kit
Length of output: 8263
🌐 Web query:
Pytest collection imports module pytest_runtest_setup hook item setup phase docs
💡 Result:
In pytest, the collection process and the test setup phase are distinct stages within the test execution protocol. Collection and Imports During the collection phase, pytest recursively traverses the filesystem to discover test modules, classes, and functions [1]. Depending on the configured --import-mode (e.g., prepend, append, or importlib), pytest imports these modules to inspect their contents [2][3]. In the default prepend mode, pytest modifies sys.path to ensure test modules can be imported as top-level modules or packages [1]. This process involves identifying test files and loading them as Python modules using standard import mechanisms or importlib [2][4]. Test Setup Phase Once collection is complete, pytest initiates the test execution protocol for each discovered item [5][6]. The setup phase is managed primarily by the pytest_runtest_setup hook [5][7]. 1. The Hook: pytest_runtest_setup(item) is called to prepare a specific test item for execution [5][6]. 2. Responsibility: Its default implementation is responsible for running the setup methods of the item and all its parent nodes (e.g., classes, modules, or the session) that have not yet been set up [5][6]. 3. Fixtures: A key part of this setup is resolving and obtaining the values of any fixtures required by the test item [5][6]. 4. State Management: Pytest uses a SetupState object to track the stack of active setup/teardown operations, ensuring that fixtures and setup logic are executed in the correct dependency order [8][9]. For developers needing to share data across these phases, the recommended approach is to use the item.stash attribute, which provides a safe way to store and retrieve data on a test item throughout its lifecycle [10][11]. To inspect the setup plan and verify fixture execution order, you can use the --setup-plan command-line flag [12].
Citations:
- 1: https://pytest.org/en/8.1.x/explanation/goodpractices.html
- 2: https://pytest.org/en/latest/explanation/pythonpath.html
- 3: https://pytest.org/latest/pythonpath.html
- 4: https://docs.pytest.org/en/stable/explanation/pythonpath.html
- 5: https://docs.pytest.org/en/stable/_modules/_pytest/hookspec.html
- 6: https://github.com/pytest-dev/pytest/blob/90465694/src/_pytest/hookspec.py
- 7: https://docs.pytest.org/en/8.3.x/_modules/_pytest/hookspec.html
- 8: https://pytest.org/en/8.0.x/_modules/_pytest/runner.html
- 9: https://docs.pytest.org/en/latest/%5Fmodules/%5Fpytest/runner.html
- 10: https://docs.pytest.org/en/8.3.x/how-to/writing_hook_functions.html
- 11: https://docs.pytest.org/en/8.2.x/how-to/writing_hook_functions.html
- 12: https://pytest.org/en/stable/reference/fixtures.html
Move kit_doctor imports into test-scoped paths.
scripts/tests/test_kit_doctor.py imports kit_doctor at module scope, so an adopted tree without scripts/kit_doctor.py aborts collection before pytest_runtest_setup can evaluate function-level kit_repo_only("scripts/kit_doctor.py") markers. These three functions already target that missing-path case; defer the kit_doctor dependency into the test body, fixture, or loader so the skip can take effect for the whole module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/test_kit_doctor.py` at line 610, Move the module-level
kit_doctor imports in test_kit_doctor.py into the relevant test bodies,
fixtures, or loader so importing the test module does not require
scripts/kit_doctor.py. Preserve the existing
kit_repo_only("scripts/kit_doctor.py") markers, allowing pytest_runtest_setup to
skip the affected tests before any kit_doctor dependency is loaded.
| 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, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a timeout to the subprocess call.
_run() calls subprocess.run with no timeout. If the subprocess pytest run hangs for any reason, this test blocks indefinitely and can stall the CI job. Set an explicit timeout so a hang surfaces as a test failure instead of a stuck job.
🔧 Proposed fix
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,
+ timeout=60,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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, | |
| ) | |
| 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, | |
| timeout=60, | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 50-58: Command coming from incoming request
Context: 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,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/test_kit_repo_only.py` around lines 50 - 59, Update the
subprocess.run call in _run to include an explicit timeout value, ensuring a
hung pytest invocation raises a failure instead of blocking indefinitely while
preserving the existing command and output-capture behavior.
…borting collection 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.
… lazy
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 <dir>; 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.
|
Round 4 (terminal): no HIGH from either lens. Merging, with six findings Declared before this round: no HIGH in the round-3 delta means the PR merges. Why filed and not fixed. Every one is sub-HIGH, and logging produces no @coderabbitai — both of your findings are real and are on #234, not
One correction to my own round-3 commit message, caught by the adversarial And the pattern worth naming, since it is the third instance in this PR: Final state, three trees, each with its vendored subset stated in the commits:
Out of scope and stated: a tree with no |
|
Tip For best results, initiate chat on the files or code changes.
The two findings remain valid and are tracked in The three measured repository layouts show that the new marker behavior works for the stated adoption cases. The no- ✏️ Learnings added
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
The two findings remain valid and are tracked in The reported results confirm the stated behavior in the kit repository and both adopter trees. The no- You are interacting with an AI system. |
1 participant
The problem
The kit tells adopters to run its test suite as post-install verification. Two
real trees, both measured at this branch's base:
/adopttree ran zero tests — pytest aborted duringcollection.
A first run that is red teaches an adopter to ignore the suite, which hides the
failures that would be real.
#226 first, because nothing under it was measurable
test_panel_prompt.pyreaddocs/agentic-dev-kit/fallback-review-panel.mdatmodule scope, so any tree without that file died during collection.
/adoptStep 3 names three files under
docs/agentic-dev-kit/and this is not one ofthem — so that was not the extreme floor, it was the documented adoption. The
read is now a function called at use time.
#134 cause 2: a
kit_repo_onlymarkerRegistered in the
conftest.pythat travels with the tests. It takes therepo-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.pyasserts oninit.sh's behaviour;an adopter who vendored engines and config has no
init.shto 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.mdrecords as having opened a hole three times. Italso 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_portability.pygets 7 marks ratherthan a module mark, because its other ~370 tests are portable and still run.
Measured, three trees, this head
711 passed in 43.65s/adopt4 failed, 647 passed, 60 skipped in 33.64s4 failed, 555 passed, 152 skipped in 8.44sEach tree is stated with its vendored subset in the commit message, because
#134's own thread establishes that a count identifies no tree without one.
Coverage here is unchanged — every marked path exists in this repo, so
nothing skips.
How the mechanism is pinned
test_kit_repo_only.pyruns pytest in a subprocess against synthetic vendoredtrees at
scripts/devkit/. Asserting from inside this repo, where every pathexists, 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, each killed by the test written for it: skip never fires, skip
always fires,
exists()→is_file(), only the first path checked, registrationdropped.
Scope, stated rather than implied
A tree with no
.gitstill aborts collection, for a reason predating thisPR:
test_portability.py,test_mutation_gate.pyandtest_pr_followup_hook.pycarry private walk-up helpers that raise rather than fall back —
_repo_layout.py's own docstring names this and calls it "not the form tocopy".
#203owns consolidating them. This PR removes one contributor andclaims nothing about that case; every tree measured here has
.git, which everyreal adopter repository has by definition. Measured and recorded on
#233.Stated limits
red.
test_kit_repo_only.pycatches a marker naming a path that neverexisted; it cannot catch a deletion, because the marker would then be telling
the truth.
kit-manifest.jsoncovers every KIT_OWNED path, butinit.shandthe root
Makefileare tracked by neither.ruff format:ruff.tomland the CI step both record the kitas deliberately lint-only and not format-clean. Running it churned 700+ lines
across four untouched modules and was reverted.
Review cost
Declared before round 1: both configured lenses over the full diff,
--carry-forwardempty. Second class — act on HIGH and on any lens-markedregression; sub-HIGH imprecision in record prose is logged, not fixed.
Withdrawal threshold: if a fix round draws a HIGH in the skip mechanism
itself, the mechanism comes out and is filed. Set at a fix round rather than at
round 1, deliberately — PR #230's hatch fired before any fix existed, which was
stricter than the rule it protected and cost that PR for nothing (#231).
Closes #226. Closes #134 — cause 1 was repaired by PR #202, this is cause 2.
#228, #227, #229, #231 stay open.