feat: derive which kit files are required, so /upgrade stops inviting an operator to decline one - #225
Merged
Merged
Conversation
… an operator to decline one kit_doctor filed `lib/kitconfig.py` under `missing` — labelled "not installed (sized-down adoption, or incomplete)" — alongside `docs/templates/*.tmpl`, which genuinely are optional. /upgrade Step 3 then says of `missing`: "decide, don't assume … ask the operator whether each missing piece is wanted." So the documented path walked an operator into declining a file every Python engine imports, having just told them it might be a deliberate omission. Adds a `required_by` axis to kit-manifest.json, DERIVED from the import graph at --generate-manifest time rather than hand-declared. Deriving it is what keeps it true: a hand-written list of hard dependencies goes stale exactly when a new engine starts importing something, which is the moment it needed to be right. The axis is a mapping, not the boolean #41 proposed. "Required" is a property of a pair, not of a file: kitconfig.py matters to a repo that installed an engine and does not matter to one that installed none, and both are supported adoptions. So a missing file becomes the new `missing-required` state only when at least one file that imports it is itself installed — resolved through the paths.engines remap, so a scripts/devkit/ adopter is judged on its own layout. `missing-required` renders as its own ✗ section ahead of everything else, names the installed importers, and joins the exit-1 set. It is deliberately kept out of `Report.drifted`: an absent file has not drifted from anything, and this report's position is that it never claims more than it knows. The two meet only at the exit code. /upgrade Step 3 gains the ordering step — install every `missing-required` file before any other copy — and its `missing` bullet now says explicitly that nothing installed imports those, which is what makes declining one safe. Two properties a fixture could not pin, so they are measured against the kit's own tree: - scripts/hooks/pre-push is bash, and its kitconfig import lives inside a `python3 - <<'PY'` heredoc. An ast-only scan drops it, so non-Python engine and hook files fall back to a text scan. - scripts/lib/devmodel_config.py must NOT appear as its own dependent. Its module docstring opens with the literal line `from devmodel_config import get, load_config, resolve_path` as a usage example — a text scan reads prose as an import, ast sees a string constant. That is why ast is the primary path and the text scan is restricted to engine and hook roles: doctrine quotes engine imports when explaining them. An adopter comparing against a manifest older than this field gets the previous behaviour exactly — every absent file reports as plain `missing`. Degrading to that is the intended failure mode, and it is pinned. test_shipped_manifest_required_by_matches_a_fresh_derivation is the guard the KIT_OWNED comments say does not exist for the tracked/untracked pairing (#216 stays open for that one) — affordable here only because this axis is derived. Closes #41. #37 and #18 stay open; both are named on the issue as the same family — the manifest not knowing what the kit consists of — and neither is addressed here.
📝 WalkthroughWalkthrough
ChangesRequired dependency diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InstalledKit
participant kit_doctor
participant Manifest
participant Upgrade
InstalledKit->>kit_doctor: inspect installed kit files
kit_doctor->>Manifest: derive and read required_by metadata
Manifest-->>kit_doctor: return dependent file relationships
kit_doctor-->>Upgrade: report missing-required files
Upgrade->>InstalledKit: install required files before optional copies
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Owner
Author
|
Self-reported limit: this PR adds two more kit-layout-only tests. Measured while prepping #134 cause 2, not caught by anything here. Tree: this Both new tests that read the real tree fail there: The cause is not a bug in either test. Not fixed here, deliberately — no Two pre-existing neighbours — |
…entirely Panel round 1, both lenses. Two HIGH findings, both reproduced here before being acted on. **The graph failed open on the exact bug class this PR closes (adversarial).** `dev_session.sh` and `reconcile_sessions.sh` both `source "$SCRIPT_DIR/lib/repo_root.sh"` — a hard dependency expressed as a PATH, not a module — and the Python-only derivation could not see it. With `lib/repo_root.sh` deleted, `kit_doctor` reported the tree `0 differ, 1 missing` and exit 0, filing it as an ordinary sized-down omission, while `bash scripts/dev_session.sh` died on line 63. That is #41's own failure mode, reproduced by the change written to close it. Worse, the /upgrade wording this PR added made the assurance STRONGER than the text it replaced: "declining one is safe" where the old text said "decide, don't assume". For repo_root.sh that was false. The bullet now scopes the claim to what the graph derives, names what it cannot see, and calls itself a prior rather than a proof. Shell `source` is now scanned, dispatched on the `.py` SUFFIX rather than on whether `ast` happened to fail — a short shell file can be accidentally valid Python, and resting a correctness property on "bash never parses" is the kind of implicit bound this report exists to avoid. The operand is captured to end-of-argument rather than with a quote-excluding class, because `$(dirname "$0")/lib/x.sh` contains a quote and a space INSIDE the expansion and a class-based match truncates it to `$(dirname`. **The exit-code test was confounded by its own fixture (both lenses, independently).** `_manifest`'s default `None` hash made the PRESENT dependent `unknown-version` — already counted in `report.drifted` — so deleting `or report.broken` from main()'s return left all 72 tests green. Reproduced exactly. The dependent now gets its real hash, and the test asserts `report.drifted == []` as a positive control on the fixture before touching the exit code. Sub-HIGH, fixed anyway because they are false claims rather than missing coverage, and this round was already open: - The real-tree test's docstring said it proves ast beats a docstring usage example. It does not: `devmodel_config.py`'s example names its own file, so the `candidate != rel` self-loop guard drops it whether ast or the regex matched — forcing the regex branch leaves that test green. The property is real and pinned by `test_a_docstring_usage_example_is_not_an_import`, whose fixture names a different module. Docstring corrected to say so. - "Nothing in the tree does this today" about dynamic imports was false — `importlib.util` is used in three test modules. They are neither `engine` nor `hook` so they are out of scope, but the sentence claimed the whole tree. Also corrected a word the fix exposed: bash `source` is not an import, so the report says "needed by" rather than "imported by". Sub-HIGH coverage gaps on correct code — the inert self-loop guard, and level-≥2 relative imports — were logged to #227 rather than fixed, per the round's declared stopping criterion. #227 stays open. New tests: the real repo_root.sh edge, four shell-expansion forms, a runtime- computed source path producing no edge, a `.py` file not being shell-scanned, and the previously dead `__init__.py` resolution candidate. make test -> 705 passed. kit_doctor -> 32 unchanged, 0 differ, 0 missing, 0 unknown, exit 0.
…h wrong
Panel round 2. Both lenses, both directions of the same mechanism.
**False negatives (adversarial, HIGH).** `_SOURCE_RE` matched only when
`source` was the first token on its physical line, so every one of these
produced NO edge — verified by executing `_sourced_paths` on each:
[ -f "$LIB" ] && source "$LIB" # guarded — the more careful idiom
if true; then source "$LIB"; fi
source "$LIB" || true # trailing content
source lib/dep.sh && echo ready
The last two the lens did not enumerate; they fell out of probing the fix. The
scanner was blindest to engines written defensively about the very dependency it
exists to find. `source` is now recognised after a separator or a block keyword,
and the operand stops at a separator rather than running to end of line.
**False positives (correctness, HIGH).** The scanner had no heredoc awareness,
so a `cat <<'EOF'` help block containing a line about how to wire up a sibling
engine became a real dependency edge. `pre-push` already ships two such blocks.
A false edge is the worse direction: it tells an adopter whose install is fine
that it is BROKEN and to install a file they do not need — `missing-required`
firing in reverse, which is the exact thing the `missing` split exists to
prevent. Heredoc bodies are now skipped by the shell scan and ONLY by it: the
Python-import scan must keep reading them, because `pre-push`'s kitconfig import
lives inside a `python3 - <<'PY'` body and genuinely executes. Both halves are
asserted in one test so the asymmetry cannot be flattened by accident.
Widening the anchor is what made comment handling load-bearing — `# cmd &&
source lib/dep.sh` would otherwise match through the `&&`. Comments are stripped
at an unquoted `#`, approximated in the false-negative direction only.
**`..` never resolved (correctness, MED).** `PurePosixPath` does not collapse
`..`, so `source "../lib/dep.sh"` built `scripts/sub/../lib/dep.sh` and could
never equal the canonical path it names. Nothing about that path is computed, so
it was not covered by the documented "computed at run time" bound — it was a
literal hard dependency dropped in silence, #41's class by another route. Now
normalised, and a target climbing out of the repo is refused rather than recorded.
Claims corrected, both of them mine:
- The dynamic-import limit named three test modules using `importlib`; there are
four. That sentence has now been wrong twice — first as "nothing in the tree
does this", then as a short list — so it no longer enumerates. `grep -rln
importlib scripts/` is the answer and cannot go stale.
- "thirty empty lists" in `generate_manifest` was a guess. It is 26 (32 entries,
6 with a dependent).
The LIMITS section now states the line-oriented bound it previously omitted, and
records that every bound errs toward NO edge deliberately — a missing edge
degrades to the pre-#41 behaviour and is merely unhelpful; a false one is
actively misleading.
`_NOT_A_LITERAL_PATH` is documented as NOT independently pinnable: `record()`'s
exact match against KIT_OWNED already discards anything it rejects, so deleting
it changes no output and fails no test. Kept as the second of two checks because
the operand capture is deliberately permissive; the backtick was added to its
class, since `source `depfile`` slipped through a filter whose whole job is
rejecting substitutions.
All four new branches mutation-tested and killed: heredoc skip (2 failures),
comment strip (1), the widened anchor (3), normpath (1).
make test -> 718 passed. kit_doctor -> 32 unchanged, 0 differ, 0 missing,
0 unknown, exit 0. The derived graph is unchanged on the real tree: 6 targets,
repo_root.sh still needed by dev_session.sh and reconcile_sessions.sh.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/tests/test_kit_doctor.py (1)
1051-1055: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the missing count instead of hard-coding 31.
The literal
31islen(kit_doctor.KIT_OWNED) - 1. Any new KIT_OWNED entry fails this test for a reason unrelated to the property under test, which is that the parenthetical is absent. Derive the count so the test keeps pinning only that property.♻️ Proposed refactor
- assert line == " files: 1 unchanged, 0 differ, 31 missing, 0 unknown" + absent = len(kit_doctor.KIT_OWNED) - 1 + assert line == f" files: 1 unchanged, 0 differ, {absent} missing, 0 unknown"🤖 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` around lines 1051 - 1055, Update the assertion in the test around the `line` variable to derive the missing-file count from `len(kit_doctor.KIT_OWNED) - 1` rather than hard-coding `31`, while preserving the expected output format and the property that the parenthetical is absent.
🤖 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 @.claude/commands/upgrade.md:
- Around line 128-136: Update the upgrade guidance around the “missing”
component decision to require rerunning kit_doctor after installing any
previously missing engine or hook, then install or address newly reported
required files before using that component. Keep the existing operator-selection
and PR documentation guidance intact.
- Around line 123-133: Update the missing-required guidance to use
component-neutral terminology: replace “engine” with “installed component” and
“imports” with “depends on,” while preserving the existing distinction from the
graph-derived missing conversation.
- Around line 115-125: Update the status list in Step 1 to include
missing-required alongside the existing statuses. Keep it distinct from ordinary
missing so operators recognize it as a required dependency that must be
installed first, consistent with the handling described later in the upgrade
procedure.
In `@scripts/kit_doctor.py`:
- Around line 378-383: Update the heredoc detection in the scanner loop to
search the comment-stripped `code` variable instead of the raw `line`. Keep
`_COMMENT_RE` processing before `_HEREDOC_RE` so real heredoc openers remain
detectable while `<<WORD` inside comments cannot enter heredoc-skip mode.
---
Nitpick comments:
In `@scripts/tests/test_kit_doctor.py`:
- Around line 1051-1055: Update the assertion in the test around the `line`
variable to derive the missing-file count from `len(kit_doctor.KIT_OWNED) - 1`
rather than hard-coding `31`, while preserving the expected output format and
the property that the parenthetical is absent.
🪄 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: ddf0e8b8-9d65-4e10-a459-ce6b267e1034
📒 Files selected for processing (4)
.claude/commands/upgrade.mdkit-manifest.jsonscripts/kit_doctor.pyscripts/tests/test_kit_doctor.py
…r structurally
Panel round 3. Both lenses, and both HIGHs were regressions introduced by my own
round-2 fix — so this commit removes that fix rather than patching it again.
**The widened anchor is withdrawn.** Round 2 taught `_SOURCE_RE` to match
`source` after a separator or block keyword, so guarded forms like
`[ -f "$LIB" ] && source "$LIB"` would be seen. The scan reads raw text and has
no notion of quote state, so this also matched:
echo "run this; source lib/dep.sh to finish"
producing a real dependency edge from ordinary printed prose. dev_session.sh
already prints exactly that kind of instruction (lines 420/423 tell a human to
run `source "$session_dir/activate"`) and was safe only by phrasing.
That is the harmful direction, by the rule this file states about itself: a
bogus required_by tells an adopter whose install is FINE that it is broken and
to install a file they do not need, while a missed edge only degrades to plain
`missing` — the pre-#41 behaviour, unhelpful but never misleading. Anchoring has
exactly one failure direction and it is the safe one. Both shell engines here
use the plain form, so nothing real is missed. #228 holds the proper fix and why
a bigger regex is not it. The miss is pinned by a test so it stays a stated
bound; `_COMMENT_RE` went with the widening, because it existed only to defend
against a `&&` inside a comment reaching the widened anchor.
**The heredoc tracker is bounded structurally, not patched again.** Round 2's
single-pass tracker read `<<<` herestrings AND `<<` inside `$(( … ))` arithmetic
as heredoc openers, and one such misparse silently deleted every real edge for
the REST OF THE FILE. dev_session.sh already contains eight herestrings, safe
today only because each is followed by `"$…` rather than a bare word. Patching
the regex per-construct is what produced three rounds of this, so instead: a
heredoc region now counts only if its delimiter ACTUALLY CLOSES on a later line.
A bogus delimiter (`greeting`, `shift_amount`) essentially never appears as a
standalone line, so the region is discarded and those lines scan normally. The
failure mode stops scaling with file length, which is what made it dangerous
rather than merely imprecise.
The `<<<` lookbehind is kept and is now pinned rather than redundant: `done`
closes every shell loop, so `read -r flag <<< done` inside a `while … do … done`
is the one realistic case where a bogus delimiter DOES recur as a line. Removing
the lookbehind fails exactly that test and nothing else.
Two precision fixes, both on claims of mine:
- The LIMITS section said "both directions of this bound are now pinned by
tests". The correctness lens checked: `grep -n "continuation\|eval"` over the
whole test file returns nothing. It now says which forms are pinned (guarded,
nested) and which are not (continuation, eval, shell function).
- `test_a_source_climbing_out_of_the_repo_produces_no_edge` pinned nothing —
routed through `derive_dependencies`, `record()`'s KIT_OWNED membership check
discards an escaped path regardless, so deleting the `..` guard left the suite
green while `_sourced_paths` really did return `{'../outside/dep.sh'}`. It now
asserts at `_sourced_paths` directly. Same unpinnable-by-construction trap the
code documents for `_NOT_A_LITERAL_PATH`, walked into one function away.
Mutation-tested, each killing only its own test: the must-close requirement,
the heredoc skip, the `<<<` lookbehind, the `..` escape guard.
make test -> 729 passed. kit_doctor -> 32 unchanged, 0 differ, 0 missing,
0 unknown, exit 0. Derived graph unchanged on the real tree: 6 targets,
repo_root.sh needed by dev_session.sh and reconcile_sessions.sh.
#228 stays open for command-position parsing.
…se edges
Panel round 4, adversarial lens, HIGH — and the declared escape hatch for this
round was "a HIGH means the shell scan comes out of this PR". It does.
**The finding.** `_HEREDOC_RE` recognised a narrower set of openers than bash
accepts, so a REAL heredoc could go undetected and its printed body get scanned
as code — manufacturing a dependency edge from text. All valid bash, all
reproduced by execution:
cmd <<A <<B # only the first opener on a line was seen
cat <<123 # numeric delimiter fails an identifier pattern
cat <<'MULTI WORD' # quoted multi-word delimiter, likewise
Round 3 bounded the mirror-image failure (over-detection: `<<<` and arithmetic
`<<` putting the scanner into a heredoc that never closed) by requiring a region
to actually close. That does nothing for under-detection: an opener never
matched is never tracked.
**Why removal rather than a fourth patch.** Every review pass since this scanner
was added found a defect in it — rounds 2, 3 and 4 of the panel, plus the bot,
which independently found that a `<<EOF` mentioned in a COMMENT swallows lines
up to the next real `EOF`. Making it correct in both directions requires
knowing whether a `source` is in command position AND recognising every heredoc
opener form: quote state, escapes, `$( )` and `$(( ))` nesting, continuations,
multi-line strings. That is a tokenizer, not a regex, and #228 now carries it
with the full account and an executable acceptance bar.
The asymmetry decides it. A MISSED edge degrades a file to plain `missing` —
the pre-#41 behaviour, unhelpful but never misleading. A FALSE edge makes
kit_doctor tell an adopter whose install is fine that it is broken and to
install a file they do not need, which is the missing/missing-required split
firing in reverse. Shipping the Python-only graph with a documented hole beats
shipping a mechanism that can produce the harmful direction.
**What replaces it, in five lines instead of thirty-five.**
`test_the_shell_source_dependency_is_a_KNOWN_GAP_not_an_oversight` asserts the
gap (repo_root.sh has no dependents) AND the complete derived graph — so a
future false edge from any non-Python source fails in the kit's own repo, which
is where such a file would be authored. A heuristic became an assertion.
`pre-push` still contributes its kitconfig edge: non-Python files are still
scanned for Python-style imports, and its `from kitconfig import …` lives in a
`python3 - <<'PY'` body that genuinely executes.
Also, all five CodeRabbit findings on this head:
- Step 1's status list omitted `missing-required` — operators read that
enumeration before Step 3's guidance. Added.
- **Re-run kit_doctor after installing anything** (its Major, and correct): the
set is computed against components present WHEN THE REPORT RAN, so installing
a previously-`missing` engine can introduce requirements the first report had
no reason to classify. Step 3 now says to re-run, and to treat the report as
converged only when a run that installed nothing shows no `missing-required`.
- Component-neutral wording: `pre-push` is a hook, not an engine, and it depends
on kitconfig. "engine … imports" -> "installed component … depends on".
- The `missing` bullet now says the graph is Python-only and names repo_root.sh
as the file that will appear there because of it.
- The hard-coded `31` in the healthy-report test is derived from KIT_OWNED, so a
new entry cannot fail it for a reason unrelated to its property.
make test -> 699 passed. kit_doctor -> 32 unchanged, 0 differ, 0 missing,
0 unknown, exit 0. Graph: 5 targets, repo_root.sh deliberately absent.
#228 stays open and now carries the whole shell scan, not just widening it.
topij
added a commit
that referenced
this pull request
Aug 2, 2026
… 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.
topij
added a commit
that referenced
this pull request
Aug 2, 2026
#232) * feat: skip kit-repo-only tests instead of failing a sized-down adopter 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. * fix: scope the positive control, compose markers, and correct a false 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. * fix: withdraw the no-.git skip guard, and stop a malformed manifest aborting 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. * fix: stop guessing the repo root, and make a second module-scope read 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
kit_doctorfiledlib/kitconfig.pyundermissing— rendered as "notinstalled (sized-down adoption, or incomplete)" — in the same list as
docs/templates/*.tmpl, which genuinely are optional./upgradeStep 3 then saidof
missing: "decide, don't assume … Ask the operator whether each missing pieceis wanted before installing it."
So the documented path invited an operator to decline a file every Python engine
imports. With it absent,
check_doc_budget.pydies withModuleNotFoundErrorandpr_watch.pyfalls back to built-in defaults, leaving the adopter's wholereview.*config inert.What ships
A
required_byaxis onkit-manifest.json, derived from the Python importgraph at
--generate-manifesttime rather than hand-declared — a hand-writtenlist of hard dependencies goes stale exactly when a new engine starts importing
something, which is the moment it needed to be right.
It is a mapping, not the boolean the issue proposed. "Required" is a property
of a pair:
kitconfig.pybreaks a repo that installed an engine and is alegitimate omission for one that installed none. A missing file becomes the new
missing-requiredstate only when at least one file that depends on it is itselfinstalled, resolved through the
paths.enginesremap.missing-requiredrenders as its own ✗ section ahead of everything else, names theinstalled dependents, and joins the exit-1 set. It is deliberately not folded
into
Report.drifted: an absent file has not drifted from anything. They meet onlyat the exit code.
/upgradeStep 3 gains the ordering step (installmissing-requiredfirst), there-run guidance (the set is computed against components present when the report
ran, so installing a previously-
missingengine can surface new requirements),and
missing-requiredis added to Step 1's status enumeration.What was withdrawn, and why it matters more than what shipped
Round 1 found that the Python-only graph missed
lib/repo_root.sh, whichdev_session.shandreconcile_sessions.shbothsource— a real fail-open in#41's own bug class. A shell
sourcescanner was built to close it, and removedin round 4 after failing review in both directions:
[ -f "$LIB" ] && source "$LIB"; widening the anchor madeecho "run this; source lib/dep.sh"produce a real edge, because regex has nonotion of quote state.
cmd <<A <<B,cat <<123and
cat <<'MULTI WORD'all defeated the opener regex, letting a printed heredocbody become a dependency. Over-detection was worse still:
<<<and arithmetic<<put the scanner into a heredoc that never closed, deleting every edge forthe rest of the file.
Both are tokenizer problems. The asymmetry decided it: a missed edge degrades a
file to plain
missing— the pre-#41 behaviour, unhelpful but never misleading —while a false edge tells an adopter whose install is fine that it is broken and
to install a file they do not need. Shipping the Python-only graph with a
documented hole beats shipping a mechanism that can produce the harmful direction.
lib/repo_root.sh's gap is therefore knowingly open, pinned bytest_the_shell_source_dependency_is_a_KNOWN_GAP_not_an_oversightso it staysstated, and carried by #228 with an executable four-case acceptance bar.
Verification
make test→699 passed in 43.60s(686 at base)uv run scripts/kit_doctor.py→files: 32 unchanged, 0 differ, 0 missing, 0 unknown, exit 0repo_root.shdeliberately absent;hooks/pre-pushstill contributes its kitconfig edge via the text-scan fallback,since its import lives in a
python3 - <<'PY'body that genuinely executesruff checkandruff format --checkclean on both changed Python filesReview
Five fallback-panel rounds (adversarial + correctness, assembler-built prompts
with
--carry-forward), plus a CodeRabbit pass whose five findings are allapplied. Rounds 2–5 reviewed the fixes, not the original change, and each found
a real defect in the shell scanner — which is what made withdrawal the right call
rather than a fourth patch.
Filed rather than folded in: #226 (a by-the-book
/adopttree runs zero tests),#227 (two unpinned derivation branches), #228 (the shell scan), #229 (this PR's
replacement guard checks graph keys while claiming to check edges, and an orphaned
comment survived the removal). All stay open.
Closes #41. #37 and #18 stay open — the same family, neither addressed here.