From d24af8b77ac9f6d58c063ccf8ef0754fe94c6a42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 10:54:06 +0300 Subject: [PATCH 1/5] feat: derive which kit files are required, so /upgrade stops inviting an operator to decline one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .claude/commands/upgrade.md | 14 +- kit-manifest.json | 24 +++- scripts/kit_doctor.py | 194 ++++++++++++++++++++++++++-- scripts/tests/test_kit_doctor.py | 212 ++++++++++++++++++++++++++++++- 4 files changed, 427 insertions(+), 17 deletions(-) diff --git a/.claude/commands/upgrade.md b/.claude/commands/upgrade.md index ef1cf86..a78e35b 100644 --- a/.claude/commands/upgrade.md +++ b/.claude/commands/upgrade.md @@ -112,12 +112,24 @@ Work through `kit_doctor`'s file list. You are already on the branch from Step 2 `init.sh` refreshed itself and migrated the config there, so those changes are captured too. Confirm with `git branch --show-current` before the first copy. +**Install every `missing-required` file first, before any other copy in this step.** +Those are the kit's own libraries — `lib/kitconfig.py` above all, which every Python +engine imports — and refreshing an engine on top of an absent one produces a broken +install: `check_doc_budget.py` dies with `ModuleNotFoundError`, and `pr_watch.py` warns +and silently falls back to built-in defaults, leaving the adopter's entire `review.*` +config inert. `kit_doctor` derives this set from the import graph, so it is answering +"what does *this* tree's installed engines need", not a fixed list. + +- **`missing-required`** → install it. This is the one absent-file case that is **not** + an operator decision: an engine that is already installed imports it, and the report + names which. Do not carry it into the `missing` conversation below. - **`unchanged`** → copy the new version straight in. It is provably untouched, so there is nothing to lose. - **`missing`** → decide, don't assume. A sized-down adoption omits engines deliberately (one surveyed repo installs 2 of 6 on purpose). Ask the operator whether each missing piece is wanted before installing it. If a piece stays out, note it in the PR body so - the next upgrade doesn't re-litigate it. + the next upgrade doesn't re-litigate it. Nothing installed here imports these — that + is what separates them from the bullet above, and it is why declining one is safe. - **`differs`** → `diff` the local file against the kit's, and read the diff: - Only kit-authored changes (the local copy is simply older) → replace it. - Local edits present → for each, find where that value now lives in diff --git a/kit-manifest.json b/kit-manifest.json index 31a7a12..c6fad80 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -96,9 +96,12 @@ }, "scripts/kit_doctor.py": { "role": "engine", - "sha256": "78ff1093fb5f243b05d0f32b7946552883322f4869bd42730fee3315867d4605" + "sha256": "5f5061e44f565e58ac5a877eacfe91ee4abe80a3461044fcca91de0402b3928f" }, "scripts/lib/atomic_write.py": { + "required_by": [ + "scripts/archive_plan_sessions.py" + ], "role": "engine", "sha256": "f9657f36f8bbcf3741382b616ae19be19cf516fc5fa4f36c53384579e96202b3" }, @@ -107,6 +110,16 @@ "sha256": "a4c29fb1808e0b3ec0a2ba80ffa97ba6c894f1a784463a5d126169542c300ef2" }, "scripts/lib/kitconfig.py": { + "required_by": [ + "scripts/archive_plan_sessions.py", + "scripts/check_doc_budget.py", + "scripts/check_memory_budget.py", + "scripts/hooks/pr_followup_hook.py", + "scripts/hooks/pre-push", + "scripts/kit_doctor.py", + "scripts/panel_prompt.py", + "scripts/pr_watch.py" + ], "role": "engine", "sha256": "4ab496661883d8f4ad590a6612a48b31f8cbf770283bb09794096149276634e6" }, @@ -119,14 +132,23 @@ "sha256": "691a7df06a3bf9019b5faca8b8ebdb9d54463d231d492a632fd54c16cd51a2c6" }, "scripts/lib/state_paths/paths.py": { + "required_by": [ + "scripts/lib/state_paths/resolver.py" + ], "role": "engine", "sha256": "f54fd7d970402f8581f6c5080aa9614ce41cb2dafa2010c08406129731799397" }, "scripts/lib/state_paths/repo_root.py": { + "required_by": [ + "scripts/lib/state_paths/resolver.py" + ], "role": "engine", "sha256": "d9412f1fe395b944a86126bcc39f85d6838382b0fc29d68c8b29b965cd530b5a" }, "scripts/lib/state_paths/resolver.py": { + "required_by": [ + "scripts/lib/state_paths/__init__.py" + ], "role": "engine", "sha256": "e39fca241d6fbf28596ed3954f28a55e65831181cedb02c7c15e372895bb5bd2" }, diff --git a/scripts/kit_doctor.py b/scripts/kit_doctor.py index 3974c1c..4651c8d 100755 --- a/scripts/kit_doctor.py +++ b/scripts/kit_doctor.py @@ -27,8 +27,19 @@ schema version, and either way the required action is the same: diff before replacing, never clobber. ``missing`` - Not installed. Either a deliberately sized-down adoption or an incomplete - one; the report can't tell, so it says so rather than guessing. + Not installed, and nothing installed here needs it. Either a deliberately + sized-down adoption or an incomplete one; the report can't tell, so it says + so rather than guessing. +``missing-required`` + Not installed, but an engine that IS installed imports it — so this install + is broken, not sized down. The distinction exists because the old report had + only ``missing``, which files a hard dependency under "sized-down adoption, + or incomplete" alongside `docs/templates/*.tmpl`, which genuinely are + optional. `/upgrade` then tells the operator that a missing piece may be a + deliberate omission and to ask before installing it — so the documented path + invited someone to decline `lib/kitconfig.py`, which every Python engine + imports (issue #41). Which files these are is DERIVED from the import graph + at ``--generate-manifest`` time, not restated by hand. ``unknown-version`` The manifest has no entry for this file, so drift can't be judged. @@ -50,7 +61,10 @@ Exit codes: 0 — every kit-owned file is `unchanged` (or intentionally absent) - 1 — at least one file `differs` or is `unknown-version` + 1 — at least one file `differs`, is `unknown-version`, or is + `missing-required`. The last one is not drift, but it is a broken + install, and the exit code an adopter gates CI on should not be green + for a tree whose engines cannot import their own library. 2 — usage error (no config, no manifest, unreadable input) — including a `kit.version` that is present but not a number. That is deliberately NOT a warning-and-exit-0: CI gates on this exit code, and a config the @@ -60,11 +74,13 @@ from __future__ import annotations import argparse +import ast import hashlib import json +import re import sys from dataclasses import dataclass, field -from pathlib import Path +from pathlib import Path, PurePosixPath sys.path.insert(0, str(Path(__file__).resolve().parent / "lib")) from kitconfig import get, load_config, repo_root # noqa: E402 @@ -191,6 +207,7 @@ # adopter's configured engines directory. KIT_ENGINE_PREFIX = "scripts" + def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, ...]: """Engine paths as they sit UNDER `paths.engines`, derived from KIT_OWNED. @@ -225,6 +242,103 @@ def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, . _ENGINE_NAMES: tuple[str, ...] = _derive_engine_names(KIT_OWNED) +# Import statements in a file Python cannot parse. Applied ONLY to `engine` and +# `hook` roles, never to docs or templates: markdown cannot import anything, and +# a doctrine file quoting `from kitconfig import get` inside a code fence would +# otherwise manufacture a dependency edge out of prose. The one real user is +# `scripts/hooks/pre-push`, a bash file whose kitconfig import lives inside a +# `python3 - <<'PY'` heredoc. +_TEXT_IMPORT_RE = re.compile(r"^\s*(?:from|import)\s+([A-Za-z_][A-Za-z0-9_.]*)", re.MULTILINE) + + +def _imported_modules(text: str) -> set[tuple[int, str]]: + """``(relative_level, dotted_name)`` for every import in `text`. + + `ast` first, and the choice matters rather than being stylistic: two library + modules here open with a usage example in the module docstring — the literal + line ``from devmodel_config import get, load_config, resolve_path`` sits in + `devmodel_config.py`'s own docstring. A text scan reads that as an import + and marks the module required by itself; `ast` sees a string constant. The + regex is the fallback for files Python cannot parse at all. + """ + modules: set[tuple[int, str]] = set() + try: + tree = ast.parse(text) + except SyntaxError: + return {(0, m.group(1)) for m in _TEXT_IMPORT_RE.finditer(text)} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + modules.update((0, alias.name) for alias in node.names) + elif isinstance(node, ast.ImportFrom): + if node.module: + modules.add((node.level, node.module)) + elif node.level: + # `from . import resolver` — the imported NAME is the module. + modules.update((node.level, alias.name) for alias in node.names) + return modules + + +def _module_targets(module: str, package_dir: str) -> tuple[str, ...]: + """Kit-layout paths a dotted module name could resolve to under `package_dir`.""" + base = "/".join(module.split(".")) + return (f"{package_dir}/{base}.py", f"{package_dir}/{base}/__init__.py") + + +def derive_dependencies( + root: Path, kit_owned: tuple[tuple[str, str], ...] = KIT_OWNED +) -> dict[str, list[str]]: + """Map each kit-owned file to the kit-owned files that import it. + + This is the ``required`` axis #41 asks for, DERIVED rather than declared — + for the same reason `_derive_engine_names` is derived (#59, #67): a + hand-maintained list of hard dependencies goes stale exactly when a new + engine starts importing something, which is the moment it needed to be + right. A boolean would also be less true than this mapping. "Required" is + not a property of a file, it is a property of a PAIR: `lib/kitconfig.py` + matters to a repo that installed an engine and does not matter to one that + installed none, and both are supported adoptions. + + Resolution mirrors what the engines actually do at run time: every one of + them does ``sys.path.insert(0, /"lib")`` before importing, so an + absolute import resolves against ``scripts/lib``. Relative imports resolve + against the importing file's own package, which is what keeps the + `state_paths` package's internal edges (``from .resolver import …``) in the + graph rather than only its top-level name. + + LIMITS, because a graph that overstates its coverage is worse than a short + one: + + - Only `engine` and `hook` roles are scanned; see `_TEXT_IMPORT_RE`. + - Only imports resolving to a KIT_OWNED path are recorded. Third-party and + stdlib imports are not this function's business, and an engine importing + an UNTRACKED kit file produces no edge at all — that is #37's class, not + one this can see. + - A dynamic import (``importlib``, an ``__import__`` call) is invisible. + Nothing in the tree does this today; a future engine that did would get a + silently thin graph, and no test here would catch it. + """ + owned = {rel for rel, _ in kit_owned} + dependents: dict[str, set[str]] = {} + for rel, role in kit_owned: + if role not in ("engine", "hook"): + continue + source = root / rel + if not source.is_file(): + continue + text = source.read_text(encoding="utf-8", errors="replace") + for level, module in _imported_modules(text): + if level: + package = PurePosixPath(rel).parent + for _ in range(level - 1): + package = package.parent + package_dir = str(package) + else: + package_dir = f"{KIT_ENGINE_PREFIX}/lib" + for candidate in _module_targets(module, package_dir): + if candidate in owned and candidate != rel: + dependents.setdefault(candidate, set()).add(rel) + return {path: sorted(deps) for path, deps in sorted(dependents.items())} + @dataclass class FileStatus: @@ -266,6 +380,16 @@ def drifted(self) -> list[FileStatus]: def missing(self) -> list[FileStatus]: return [f for f in self.files if f.state == "missing"] + @property + def broken(self) -> list[FileStatus]: + """Files an installed engine imports and that are not installed. + + Deliberately NOT folded into `drifted`: a file that is absent has not + drifted from anything, and this report's whole position is that it does + not claim more than it knows. They meet only at the exit code. + """ + return [f for f in self.files if f.state == "missing-required"] + def sha256_of(path: Path) -> str: digest = hashlib.sha256() @@ -399,7 +523,16 @@ def generate_manifest(root: Path, kit_version: int) -> dict: absent here is recorded as ``None`` rather than skipped, so a packaging mistake shows up as an explicit hole instead of silently narrowing what later gets checked. + + ``required_by`` is written only where the derived dependent set is non-empty + — most kit files are imported by nothing, and an entry per file would be + thirty empty lists to read past in every manifest diff. A reader must + therefore treat an ABSENT key as "no known dependents", which is also what + an older manifest (written before this field existed) yields: it reports + every missing file as an ordinary `missing`, exactly as it did before. + Degrading to the previous behaviour is the intended failure mode. """ + dependents = derive_dependencies(root) files: dict[str, dict] = {} for rel, role in KIT_OWNED: target = root / rel @@ -407,6 +540,8 @@ def generate_manifest(root: Path, kit_version: int) -> dict: "sha256": sha256_of(target) if target.is_file() else None, "role": role, } + if dependents.get(rel): + files[rel]["required_by"] = dependents[rel] return { "kit_version": kit_version, "files": files, @@ -418,6 +553,11 @@ def inspect(root: Path, manifest: dict, config: dict) -> Report: engines_dir = str(get(config, "paths.engines", KIT_ENGINE_PREFIX)) manifest_files = manifest.get("files") or {} + # Presence of EVERY kit-owned file, resolved before the status loop: whether + # a missing file is a broken install or a sized-down one is a question about + # its dependents, and those can sit anywhere in KIT_OWNED order. + present = {rel: (root / _remap(rel, engines_dir)).is_file() for rel, _ in KIT_OWNED} + statuses: list[FileStatus] = [] for rel, role in KIT_OWNED: local_rel = _remap(rel, engines_dir) @@ -425,12 +565,21 @@ def inspect(root: Path, manifest: dict, config: dict) -> Report: entry = manifest_files.get(rel) or {} expected = entry.get("sha256") if not target.is_file(): - statuses.append(FileStatus(local_rel, role, "missing")) + # Filtered to INSTALLED dependents. A repo that installed no engine + # is a supported sized-down adoption and must not be told its + # missing library breaks it — the same distinction `_ENGINE_NAMES` + # exists to keep the engines probe from getting wrong (#59). + needed_by = [dep for dep in (entry.get("required_by") or []) if present.get(dep)] + if needed_by: + names = ", ".join(PurePosixPath(dep).name for dep in needed_by) + statuses.append( + FileStatus(local_rel, role, "missing-required", f"imported by {names}") + ) + else: + statuses.append(FileStatus(local_rel, role, "missing")) continue if expected is None: - statuses.append( - FileStatus(local_rel, role, "unknown-version", "no manifest entry") - ) + statuses.append(FileStatus(local_rel, role, "unknown-version", "no manifest entry")) continue actual = sha256_of(target) if actual == expected: @@ -552,11 +701,20 @@ def render(report: Report) -> str: for f in report.files: by_state.setdefault(f.state, []).append(f) + # `missing` counts BOTH absent states, so the total does not change meaning + # for anyone reading this line as "how much of the kit is not here"; the + # parenthetical is what says how much of that absence is breakage. Rendered + # only when non-zero, so a healthy install's summary line is unchanged. + n_required_missing = len(by_state.get("missing-required", [])) + n_absent = len(by_state.get("missing", [])) + n_required_missing + absent_note = ( + f" ({n_required_missing} required by an installed engine)" if n_required_missing else "" + ) lines.append("") lines.append( f" files: {len(by_state.get('unchanged', []))} unchanged, " f"{len(by_state.get('differs', []))} differ, " - f"{len(by_state.get('missing', []))} missing, " + f"{n_absent} missing{absent_note}, " f"{len(by_state.get('unknown-version', []))} unknown" ) # Narrow "differs" using the schema version rather than asserting a cause. @@ -598,6 +756,14 @@ def render(report: Report) -> str: "diff before replacing" ) for state, label in ( + # First, and phrased to leave no room for the "deliberate omission" + # reading: this is the one absent-file case where the answer is not + # "decide, don't assume" but "install it". + ( + "missing-required", + "✗ NOT INSTALLED, and imported by an engine that is — this install is " + "broken, not sized down. Install these before refreshing any engine", + ), ("differs", differs_label), ("unknown-version", "no manifest entry — drift cannot be judged"), ("missing", "not installed (sized-down adoption, or incomplete)"), @@ -687,11 +853,15 @@ def main(argv: list[str] | None = None) -> int: # a manifest git TRACKS. Tracked is the property that makes that work, not # location: `--manifest` and `--root` can both aim at an untracked path, # inside the repo or outside it. - manifest_path.write_text(json.dumps(manifest, indent=2, sort_keys=True) + "\n", encoding="utf-8") + manifest_path.write_text( + json.dumps(manifest, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) holes = [p for p, e in manifest["files"].items() if e["sha256"] is None] print(f"wrote {manifest_path} ({len(manifest['files'])} files, kit_version={version})") if holes: - print(f"warning: {len(holes)} listed file(s) absent from this checkout:", file=sys.stderr) + print( + f"warning: {len(holes)} listed file(s) absent from this checkout:", file=sys.stderr + ) for h in holes: print(f" · {h}", file=sys.stderr) return 0 @@ -742,7 +912,7 @@ def main(argv: list[str] | None = None) -> int: # the adopter sees the ⚠ line, then a non-zero status so CI cannot go # green on a config this very run called UNREADABLE. return 2 - return 1 if report.drifted else 0 + return 1 if report.drifted or report.broken else 0 if __name__ == "__main__": diff --git a/scripts/tests/test_kit_doctor.py b/scripts/tests/test_kit_doctor.py index 8ae6890..44c7ee9 100644 --- a/scripts/tests/test_kit_doctor.py +++ b/scripts/tests/test_kit_doctor.py @@ -58,7 +58,9 @@ def _manifest(entries: dict[str, str | None], version: int = 2) -> dict: def test_remap_follows_configured_engines_dir(): assert kit_doctor._remap("scripts/pr_watch.py", "scripts") == "scripts/pr_watch.py" - assert kit_doctor._remap("scripts/pr_watch.py", "scripts/devkit") == "scripts/devkit/pr_watch.py" + assert ( + kit_doctor._remap("scripts/pr_watch.py", "scripts/devkit") == "scripts/devkit/pr_watch.py" + ) assert ( kit_doctor._remap("scripts/lib/state_paths/resolver.py", "tools/devkit") == "tools/devkit/lib/state_paths/resolver.py" @@ -441,7 +443,9 @@ def test_a_manifest_without_a_usable_version_is_not_a_silent_checkmark( report = kit_doctor.inspect(root, manifest, config) assert report.kit_version_manifest is None rendered = kit_doctor.render(report) - assert f"manifest kit_version is {shown} — cannot tell whether this config is behind" in rendered + assert ( + f"manifest kit_version is {shown} — cannot tell whether this config is behind" in rendered + ) assert "cannot narrow this" in rendered assert "LOCAL EDITS" not in rendered assert "OLDER version" not in rendered @@ -583,4 +587,206 @@ def test_generate_manifest_refuses_an_unreadable_version(tmp_path, capsys): def test_remap_tolerates_a_trailing_slash(): - assert kit_doctor._remap("scripts/pr_watch.py", "scripts/devkit/") == "scripts/devkit/pr_watch.py" + assert ( + kit_doctor._remap("scripts/pr_watch.py", "scripts/devkit/") == "scripts/devkit/pr_watch.py" + ) + + +# --- the required-by axis (#41) ------------------------------------------- +# +# The bug these pin: `missing` filed a hard dependency under "sized-down +# adoption, or incomplete", and /upgrade tells the operator that such a piece +# may be a deliberate omission and to ask before installing it. So the +# documented path invited someone to decline `lib/kitconfig.py`, which every +# Python engine imports. + + +def _tree(tmp_path: Path, files: dict[str, str]) -> Path: + for rel, text in files.items(): + _write(tmp_path / rel, text) + return tmp_path + + +def test_dependency_graph_of_the_real_kit_names_kitconfigs_importers(): + """Measured against the kit's own tree, not a fixture. + + Two properties a fixture cannot pin, both of which a plausible + implementation gets wrong: + + - `scripts/hooks/pre-push` is BASH, and its kitconfig import lives inside a + `python3 - <<'PY'` heredoc. An ast-only scan drops it. + - `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, so a text scan reads prose as an import. + """ + graph = kit_doctor.derive_dependencies(REPO_ROOT) + importers = set(graph.get("scripts/lib/kitconfig.py", [])) + assert "scripts/hooks/pre-push" in importers, "the bash heredoc import was dropped" + assert "scripts/pr_watch.py" in importers, "a function-level import was dropped" + assert "scripts/lib/devmodel_config.py" not in graph, "a docstring example became an edge" + # Every engine that imports it, by the kit's own grep. Equality, not a + # subset: a NEW importer that this misses is exactly the staleness the + # derivation exists to prevent. + assert importers == { + "scripts/archive_plan_sessions.py", + "scripts/check_doc_budget.py", + "scripts/check_memory_budget.py", + "scripts/hooks/pr_followup_hook.py", + "scripts/hooks/pre-push", + "scripts/kit_doctor.py", + "scripts/panel_prompt.py", + "scripts/pr_watch.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. + + This is the guard the KIT_OWNED comments say does not exist for the + tracked/untracked pairing (#216) — it is affordable here only because the + axis is derived rather than hand-written. + """ + manifest = json.loads((REPO_ROOT / kit_doctor.MANIFEST_NAME).read_text(encoding="utf-8")) + shipped = { + rel: entry["required_by"] + for rel, entry in manifest["files"].items() + if entry.get("required_by") + } + assert shipped == kit_doctor.derive_dependencies(REPO_ROOT), ( + "kit-manifest.json's required_by is stale: run " + "`uv run scripts/kit_doctor.py --generate-manifest` and commit it." + ) + + +def test_a_docstring_usage_example_is_not_an_import(tmp_path): + root = _tree( + tmp_path, + { + "scripts/lib/thing.py": '"""Use it:\n\n from other import go\n"""\n', + "scripts/lib/other.py": "def go(): ...\n", + }, + ) + owned = (("scripts/lib/thing.py", "engine"), ("scripts/lib/other.py", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == {} + + +def test_relative_imports_inside_a_package_are_edges(tmp_path): + root = _tree( + tmp_path, + { + "scripts/lib/pkg/__init__.py": "from .inner import thing\nfrom . import sibling\n", + "scripts/lib/pkg/inner.py": "thing = 1\n", + "scripts/lib/pkg/sibling.py": "other = 2\n", + }, + ) + owned = ( + ("scripts/lib/pkg/__init__.py", "engine"), + ("scripts/lib/pkg/inner.py", "engine"), + ("scripts/lib/pkg/sibling.py", "engine"), + ) + assert kit_doctor.derive_dependencies(root, owned) == { + "scripts/lib/pkg/inner.py": ["scripts/lib/pkg/__init__.py"], + "scripts/lib/pkg/sibling.py": ["scripts/lib/pkg/__init__.py"], + } + + +def test_doctrine_prose_quoting_an_import_is_not_scanned(tmp_path): + """Roles other than engine/hook are skipped, and this is why: the fallback + text scan cannot tell a code fence from code, and kit doctrine quotes engine + imports when explaining them.""" + root = _tree( + tmp_path, + { + "docs/kit/explainer.md": "The engines do:\n\n```python\nfrom kitconfig import get\n```\n", + "scripts/lib/kitconfig.py": "def get(): ...\n", + }, + ) + owned = (("docs/kit/explainer.md", "doctrine"), ("scripts/lib/kitconfig.py", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == {} + + +def test_a_missing_library_an_installed_engine_imports_is_broken_not_sized_down(tmp_path): + root = _fake_repo(tmp_path) # installs scripts/check_doc_budget.py, no lib/ + manifest = _manifest({"scripts/check_doc_budget.py": None, "scripts/lib/kitconfig.py": None}) + manifest["files"]["scripts/lib/kitconfig.py"]["required_by"] = ["scripts/check_doc_budget.py"] + config = kit_doctor.load_config(root / "config" / "dev-model.yaml") + report = kit_doctor.inspect(root, manifest, config) + states = {f.path: f.state for f in report.files} + assert states["scripts/lib/kitconfig.py"] == "missing-required" + assert [f.path for f in report.broken] == ["scripts/lib/kitconfig.py"] + detail = next(f.detail for f in report.files if f.path == "scripts/lib/kitconfig.py") + assert detail == "imported by check_doc_budget.py" + + +def test_a_missing_library_nothing_installed_imports_stays_an_ordinary_omission(tmp_path): + """The pair, not the file, is what makes a dependency required. A repo that + installed no engine is a supported sized-down adoption and must not be told + its absent library breaks it.""" + root = _fake_repo(tmp_path) + (root / "scripts" / "check_doc_budget.py").unlink() + manifest = _manifest({"scripts/check_doc_budget.py": None, "scripts/lib/kitconfig.py": None}) + manifest["files"]["scripts/lib/kitconfig.py"]["required_by"] = ["scripts/check_doc_budget.py"] + config = kit_doctor.load_config(root / "config" / "dev-model.yaml") + report = kit_doctor.inspect(root, manifest, config) + states = {f.path: f.state for f in report.files} + assert states["scripts/lib/kitconfig.py"] == "missing" + assert report.broken == [] + + +def test_the_dependent_is_looked_up_through_the_engines_remap(tmp_path): + """`required_by` records kit-layout paths; whether the dependent is INSTALLED + is a question about the adopter's layout. Without the remap, a repo that + vendored engines under `scripts/devkit/` looks like it installed nothing and + a genuinely broken install reports as sized-down.""" + root = _fake_repo(tmp_path, engines="scripts/devkit") + manifest = _manifest({"scripts/check_doc_budget.py": None, "scripts/lib/kitconfig.py": None}) + manifest["files"]["scripts/lib/kitconfig.py"]["required_by"] = ["scripts/check_doc_budget.py"] + config = kit_doctor.load_config(root / "config" / "dev-model.yaml") + report = kit_doctor.inspect(root, manifest, config) + states = {f.path: f.state for f in report.files} + assert states["scripts/devkit/lib/kitconfig.py"] == "missing-required" + + +def test_a_manifest_without_the_field_degrades_to_the_old_report(tmp_path): + """An adopter comparing against a kit release older than this field gets the + previous behaviour, not a crash and not a false all-clear.""" + root = _fake_repo(tmp_path) + manifest = _manifest({"scripts/check_doc_budget.py": None, "scripts/lib/kitconfig.py": None}) + config = kit_doctor.load_config(root / "config" / "dev-model.yaml") + report = kit_doctor.inspect(root, manifest, config) + states = {f.path: f.state for f in report.files} + assert states["scripts/lib/kitconfig.py"] == "missing" + assert report.broken == [] + + +def test_a_broken_install_is_not_a_green_exit(tmp_path, capsys): + root = _fake_repo(tmp_path) + manifest = _manifest({"scripts/check_doc_budget.py": None, "scripts/lib/kitconfig.py": None}) + manifest["files"]["scripts/lib/kitconfig.py"]["required_by"] = ["scripts/check_doc_budget.py"] + manifest_path = root / "kit-manifest.json" + manifest_path.write_text(json.dumps(manifest), encoding="utf-8") + code = kit_doctor.main(["--root", str(root), "--manifest", str(manifest_path)]) + out = capsys.readouterr().out + assert code == 1, "a tree whose engines cannot import their own library exited green" + assert "this install is broken, not sized down" in out + assert "1 required by an installed engine" in out + + +def test_a_healthy_report_does_not_grow_the_parenthetical(tmp_path, capsys): + """The count line's new clause is conditional; a report with no breakage + must read exactly as it did.""" + root = _fake_repo(tmp_path) + target = root / "scripts" / "check_doc_budget.py" + manifest = _manifest({"scripts/check_doc_budget.py": kit_doctor.sha256_of(target)}) + config = kit_doctor.load_config(root / "config" / "dev-model.yaml") + print(kit_doctor.render(kit_doctor.inspect(root, manifest, config))) + out = capsys.readouterr().out + line = next(ln for ln in out.splitlines() if ln.startswith(" files:")) + # Everything but check_doc_budget.py is absent here, so the count is large — + # the property under test is that none of it is called REQUIRED, because + # nothing that imports it is installed either. + assert line == " files: 1 unchanged, 0 differ, 31 missing, 0 unknown" + assert "required by an installed engine" not in out + assert "this install is broken" not in out From 63d9b5bf9f1ca51896892165b6fac5b92145b428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 11:20:05 +0300 Subject: [PATCH 2/5] =?UTF-8?q?fix:=20scan=20shell=20`source`=20too=20?= =?UTF-8?q?=E2=80=94=20the=20derived=20graph=20missed=20repo=5Froot.sh=20e?= =?UTF-8?q?ntirely?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .claude/commands/upgrade.md | 8 +- kit-manifest.json | 6 +- scripts/kit_doctor.py | 108 +++++++++++++++++++---- scripts/tests/test_kit_doctor.py | 144 ++++++++++++++++++++++++++++--- 4 files changed, 236 insertions(+), 30 deletions(-) diff --git a/.claude/commands/upgrade.md b/.claude/commands/upgrade.md index a78e35b..b2414ad 100644 --- a/.claude/commands/upgrade.md +++ b/.claude/commands/upgrade.md @@ -128,8 +128,12 @@ config inert. `kit_doctor` derives this set from the import graph, so it is answ - **`missing`** → decide, don't assume. A sized-down adoption omits engines deliberately (one surveyed repo installs 2 of 6 on purpose). Ask the operator whether each missing piece is wanted before installing it. If a piece stays out, note it in the PR body so - the next upgrade doesn't re-litigate it. Nothing installed here imports these — that - is what separates them from the bullet above, and it is why declining one is safe. + the next upgrade doesn't re-litigate it. Nothing installed here imports these **by + the graph `kit_doctor` derives** — Python imports plus shell `source` — which is what + separates them from the bullet above. That graph cannot see a dependency whose path + is computed at run time, so it is a much better prior than the old blanket "decide, + don't assume", not a proof; if a piece you are declining is one an engine plausibly + reaches for, check before dropping it. - **`differs`** → `diff` the local file against the kit's, and read the diff: - Only kit-authored changes (the local copy is simply older) → replace it. - Local edits present → for each, find where that value now lives in diff --git a/kit-manifest.json b/kit-manifest.json index c6fad80..df67377 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -96,7 +96,7 @@ }, "scripts/kit_doctor.py": { "role": "engine", - "sha256": "5f5061e44f565e58ac5a877eacfe91ee4abe80a3461044fcca91de0402b3928f" + "sha256": "bf95f87e48e2ad322033af2d7c6edb1cf138522def56a6362173ebdf3faa269b" }, "scripts/lib/atomic_write.py": { "required_by": [ @@ -124,6 +124,10 @@ "sha256": "4ab496661883d8f4ad590a6612a48b31f8cbf770283bb09794096149276634e6" }, "scripts/lib/repo_root.sh": { + "required_by": [ + "scripts/dev_session.sh", + "scripts/reconcile_sessions.sh" + ], "role": "engine", "sha256": "980cbf5596cea67033a5dd02d53630f2a92c24afd693ba7727d5fc50303ff555" }, diff --git a/scripts/kit_doctor.py b/scripts/kit_doctor.py index 4651c8d..473d303 100755 --- a/scripts/kit_doctor.py +++ b/scripts/kit_doctor.py @@ -31,15 +31,16 @@ sized-down adoption or an incomplete one; the report can't tell, so it says so rather than guessing. ``missing-required`` - Not installed, but an engine that IS installed imports it — so this install + Not installed, but an engine that IS installed needs it — so this install is broken, not sized down. The distinction exists because the old report had only ``missing``, which files a hard dependency under "sized-down adoption, or incomplete" alongside `docs/templates/*.tmpl`, which genuinely are optional. `/upgrade` then tells the operator that a missing piece may be a deliberate omission and to ask before installing it — so the documented path invited someone to decline `lib/kitconfig.py`, which every Python engine - imports (issue #41). Which files these are is DERIVED from the import graph - at ``--generate-manifest`` time, not restated by hand. + imports (issue #41). Which files these are is DERIVED at + ``--generate-manifest`` time from the dependency graph — Python imports and + shell ``source`` — not restated by hand. ``unknown-version`` The manifest has no entry for this file, so drift can't be judged. @@ -64,7 +65,7 @@ 1 — at least one file `differs`, is `unknown-version`, or is `missing-required`. The last one is not drift, but it is a broken install, and the exit code an adopter gates CI on should not be green - for a tree whose engines cannot import their own library. + for a tree whose engines cannot load their own library. 2 — usage error (no config, no manifest, unreadable input) — including a `kit.version` that is present but not a number. That is deliberately NOT a warning-and-exit-0: CI gates on this exit code, and a config the @@ -245,11 +246,37 @@ def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, . # Import statements in a file Python cannot parse. Applied ONLY to `engine` and # `hook` roles, never to docs or templates: markdown cannot import anything, and # a doctrine file quoting `from kitconfig import get` inside a code fence would -# otherwise manufacture a dependency edge out of prose. The one real user is +# otherwise manufacture a dependency edge out of prose. Its user is # `scripts/hooks/pre-push`, a bash file whose kitconfig import lives inside a # `python3 - <<'PY'` heredoc. _TEXT_IMPORT_RE = re.compile(r"^\s*(?:from|import)\s+([A-Za-z_][A-Za-z0-9_.]*)", re.MULTILINE) +# Bash `source` / `.` of another kit file. A shell engine's dependency is a PATH, +# not a module name, so it needs its own scanner — and without one the graph +# fails open on exactly the pair #41 is about: `dev_session.sh` and +# `reconcile_sessions.sh` both `source "$SCRIPT_DIR/lib/repo_root.sh"`, so a tree +# missing `lib/repo_root.sh` reported `0 missing`-that-matters and exit 0 while +# `bash scripts/dev_session.sh` died on line 63. The Python-only version of this +# function shipped that hole and the /upgrade wording built on it went further +# than the text it replaced — "declining one is safe" — which is a worse false +# assurance than the "decide, don't assume" it superseded. Found by the +# adversarial lens on PR #225. +# The operand is captured to end-of-argument rather than with a quote-excluding +# character class, because `$(dirname "$0")/lib/x.sh` — the commonest form of +# this idiom — contains both a quote and a space INSIDE the expansion, and a +# class-based match truncates it to `$(dirname`. +_SOURCE_RE = re.compile(r"""^[ \t]*(?:source|\.)[ \t]+(\S[^;#\n]*)""", re.MULTILINE) + +# A leading shell expansion — `$SCRIPT_DIR/`, `${VAR}/`, `$(dirname "$0")/` — +# stripped so what remains is a path relative to the sourcing file's own +# directory, which is what every such prefix in this tree resolves to. +_SHELL_PREFIX_RE = re.compile(r"^(?:\$\([^)]*\)|\$\{[^}]*\}|\$[A-Za-z_][A-Za-z0-9_]*)/") + +# What is left must be a literal relative path. Anything still carrying a shell +# metacharacter is computed at run time, and the honest answer there is no edge +# rather than a guessed one. +_NOT_A_LITERAL_PATH = re.compile(r"""[\s$"'*?\[\]{}()|&<>]""") + def _imported_modules(text: str) -> set[tuple[int, str]]: """``(relative_level, dotted_name)`` for every import in `text`. @@ -284,6 +311,28 @@ def _module_targets(module: str, package_dir: str) -> tuple[str, ...]: return (f"{package_dir}/{base}.py", f"{package_dir}/{base}/__init__.py") +def _sourced_paths(text: str, rel: str) -> set[str]: + """Kit-layout paths this shell file `source`s, resolved against its own dir. + + Only the leading shell expansion is stripped; a `source` whose path is + genuinely computed at run time (a loop variable, a value read from config) + resolves to nothing and produces no edge. Nothing in the tree does that + today, and no test here would catch a future engine that did — the same + stated limit `derive_dependencies` records for dynamic Python imports. + """ + here = PurePosixPath(rel).parent + found: set[str] = set() + for match in _SOURCE_RE.finditer(text): + operand = match.group(1).strip() + if len(operand) > 1 and operand[0] in "\"'" and operand[-1] == operand[0]: + operand = operand[1:-1] + tail = _SHELL_PREFIX_RE.sub("", operand) + if not tail or tail.startswith("/") or _NOT_A_LITERAL_PATH.search(tail): + continue + found.add(str(PurePosixPath(here / tail))) + return found + + def derive_dependencies( root: Path, kit_owned: tuple[tuple[str, str], ...] = KIT_OWNED ) -> dict[str, list[str]]: @@ -298,13 +347,22 @@ def derive_dependencies( matters to a repo that installed an engine and does not matter to one that installed none, and both are supported adoptions. - Resolution mirrors what the engines actually do at run time: every one of - them does ``sys.path.insert(0, /"lib")`` before importing, so an + Resolution mirrors what the engines actually do at run time: every Python + one does ``sys.path.insert(0, /"lib")`` before importing, so an absolute import resolves against ``scripts/lib``. Relative imports resolve against the importing file's own package, which is what keeps the `state_paths` package's internal edges (``from .resolver import …``) in the graph rather than only its top-level name. + **The shell engines are scanned too, and must be.** `dev_session.sh` and + `reconcile_sessions.sh` `source "$SCRIPT_DIR/lib/repo_root.sh"`, which is a + hard dependency expressed as a path rather than a module. A Python-only + version of this function reported such a tree exit-0 clean while + ``bash scripts/dev_session.sh`` died on its `source` line — #41's own bug + class, reproduced by the change meant to close it. `.py` files take the + `ast` path and only that; everything else is scanned for both Python-style + imports (the `pre-push` heredoc) and shell `source`. + LIMITS, because a graph that overstates its coverage is worse than a short one: @@ -313,12 +371,24 @@ def derive_dependencies( stdlib imports are not this function's business, and an engine importing an UNTRACKED kit file produces no edge at all — that is #37's class, not one this can see. - - A dynamic import (``importlib``, an ``__import__`` call) is invisible. - Nothing in the tree does this today; a future engine that did would get a - silently thin graph, and no test here would catch it. + - A dynamic import (``importlib``, an ``__import__`` call) is invisible, as + is a `source` whose path is computed at run time. No file THIS FUNCTION + SCANS does either today — the qualifier is load-bearing and an earlier + version of this sentence dropped it, claiming "nothing in the tree", + which is false: `importlib.util` is used in `test_portability.py`, + `test_check_memory_budget.py` and `test_pr_followup_hook.py`. Those are + test files, neither `engine` nor `hook`, so they are out of scope here — + but the sentence read as a claim about the whole tree. A future ENGINE + that did either would get a silently thin graph, and no test here would + catch it. (Correctness lens, PR #225.) """ owned = {rel for rel, _ in kit_owned} dependents: dict[str, set[str]] = {} + + def record(target: str, importer: str) -> None: + if target in owned and target != importer: + dependents.setdefault(target, set()).add(importer) + for rel, role in kit_owned: if role not in ("engine", "hook"): continue @@ -335,8 +405,14 @@ def derive_dependencies( else: package_dir = f"{KIT_ENGINE_PREFIX}/lib" for candidate in _module_targets(module, package_dir): - if candidate in owned and candidate != rel: - dependents.setdefault(candidate, set()).add(rel) + record(candidate, rel) + # Dispatched on the SUFFIX, not on whether `ast` happened to succeed: 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. + if not rel.endswith(".py"): + for candidate in _sourced_paths(text, rel): + record(candidate, rel) return {path: sorted(deps) for path, deps in sorted(dependents.items())} @@ -382,7 +458,7 @@ def missing(self) -> list[FileStatus]: @property def broken(self) -> list[FileStatus]: - """Files an installed engine imports and that are not installed. + """Files an installed engine needs and that are not installed. Deliberately NOT folded into `drifted`: a file that is absent has not drifted from anything, and this report's whole position is that it does @@ -525,7 +601,7 @@ def generate_manifest(root: Path, kit_version: int) -> dict: later gets checked. ``required_by`` is written only where the derived dependent set is non-empty - — most kit files are imported by nothing, and an entry per file would be + — most kit files are needed by nothing, and an entry per file would be thirty empty lists to read past in every manifest diff. A reader must therefore treat an ABSENT key as "no known dependents", which is also what an older manifest (written before this field existed) yields: it reports @@ -573,7 +649,7 @@ def inspect(root: Path, manifest: dict, config: dict) -> Report: if needed_by: names = ", ".join(PurePosixPath(dep).name for dep in needed_by) statuses.append( - FileStatus(local_rel, role, "missing-required", f"imported by {names}") + FileStatus(local_rel, role, "missing-required", f"needed by {names}") ) else: statuses.append(FileStatus(local_rel, role, "missing")) @@ -761,7 +837,7 @@ def render(report: Report) -> str: # "decide, don't assume" but "install it". ( "missing-required", - "✗ NOT INSTALLED, and imported by an engine that is — this install is " + "✗ NOT INSTALLED, and needed by an engine that is — this install is " "broken, not sized down. Install these before refreshing any engine", ), ("differs", differs_label), diff --git a/scripts/tests/test_kit_doctor.py b/scripts/tests/test_kit_doctor.py index 44c7ee9..9042df0 100644 --- a/scripts/tests/test_kit_doctor.py +++ b/scripts/tests/test_kit_doctor.py @@ -610,15 +610,24 @@ def _tree(tmp_path: Path, files: dict[str, str]) -> Path: def test_dependency_graph_of_the_real_kit_names_kitconfigs_importers(): """Measured against the kit's own tree, not a fixture. - Two properties a fixture cannot pin, both of which a plausible - implementation gets wrong: - - - `scripts/hooks/pre-push` is BASH, and its kitconfig import lives inside a - `python3 - <<'PY'` heredoc. An ast-only scan drops it. - - `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, so a text scan reads prose as an import. + What this pins that a fixture cannot: `scripts/hooks/pre-push` is BASH, and + its kitconfig import lives inside a `python3 - <<'PY'` heredoc, so an + ast-only scan drops it; and the equality assertion below fails on any NEW + importer the derivation misses, which is the staleness the whole axis exists + to prevent. + + **What it does NOT pin, corrected from an earlier version of this docstring + that claimed otherwise.** The `devmodel_config.py` assertion looks like it + proves `ast` beats a text scan on a docstring usage example — that module's + docstring opens with the literal line `from devmodel_config import get, + load_config, resolve_path`. It does not: that example names the module's own + file, so `derive_dependencies`'s `candidate != rel` self-loop guard drops it + whether ast or the regex produced the match. Forcing the regex branch leaves + this test GREEN. The ast-vs-prose property is real and IS pinned — by + `test_a_docstring_usage_example_is_not_an_import`, whose fixture names a + DIFFERENT module and so is not rescued by the self-loop guard. The assertion + is kept here as a cheap regression check on the graph's shape, not as + evidence of the mechanism. Found by the correctness lens on PR #225. """ graph = kit_doctor.derive_dependencies(REPO_ROOT) importers = set(graph.get("scripts/lib/kitconfig.py", [])) @@ -640,6 +649,94 @@ def test_dependency_graph_of_the_real_kit_names_kitconfigs_importers(): } +def test_the_shell_engines_source_dependency_is_in_the_graph(): + """A shell engine's dependency is a PATH, not a module, and the first version + of `derive_dependencies` scanned only Python. + + Consequence, reproduced by the adversarial lens on PR #225: with + `lib/repo_root.sh` deleted, `kit_doctor` reported the tree exit-0 clean and + listed the file as an ordinary sized-down omission, while + `bash scripts/dev_session.sh` died on its `source` line. That is #41's own + failure mode, reproduced by the change written to close it. + """ + graph = kit_doctor.derive_dependencies(REPO_ROOT) + assert graph.get("scripts/lib/repo_root.sh") == [ + "scripts/dev_session.sh", + "scripts/reconcile_sessions.sh", + ] + + +@pytest.mark.parametrize( + "line", + [ + 'source "$SCRIPT_DIR/lib/dep.sh"', + "source ${SCRIPT_DIR}/lib/dep.sh", + 'source "$(dirname "$0")/lib/dep.sh"', + ". $SCRIPT_DIR/lib/dep.sh", + ], +) +def test_the_shell_scan_strips_the_leading_expansion(tmp_path, line): + root = _tree( + tmp_path, + { + "scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", + "scripts/lib/dep.sh": "x() { :; }\n", + }, + ) + owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == { + "scripts/lib/dep.sh": ["scripts/engine.sh"] + } + + +def test_a_source_path_computed_at_runtime_produces_no_edge(tmp_path): + """The stated limit, pinned so it stays stated rather than drifting into an + unnoticed false edge: a bare variable resolves to nothing, not to a guess.""" + root = _tree( + tmp_path, + { + "scripts/engine.sh": '#!/usr/bin/env bash\nsource "$COMPUTED"\n', + "scripts/lib/dep.sh": "\n", + }, + ) + owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == {} + + +def test_a_python_file_is_not_scanned_for_shell_source(tmp_path): + """Dispatch is on the `.py` suffix, not on whether `ast` happened to succeed. + A Python engine mentioning `source lib/dep.sh` in a docstring or a subprocess + string must not manufacture an edge.""" + root = _tree( + tmp_path, + { + "scripts/engine.py": '"""Run:\n\n source lib/dep.sh\n"""\n', + "scripts/lib/dep.sh": "\n", + }, + ) + owned = (("scripts/engine.py", "engine"), ("scripts/lib/dep.sh", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == {} + + +def test_importing_a_package_by_name_resolves_to_its_init(tmp_path): + """The `/__init__.py` candidate in `_module_targets`. Nothing in the kit + imports a KIT_OWNED package by bare name today, so deleting that candidate + left the whole suite green — flagged by the adversarial lens on PR #225 as + an untested resolution rule, and pinned here rather than left to a future + engine to discover.""" + root = _tree( + tmp_path, + { + "scripts/engine.py": "import pkg\n", + "scripts/lib/pkg/__init__.py": "value = 1\n", + }, + ) + owned = (("scripts/engine.py", "engine"), ("scripts/lib/pkg/__init__.py", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == { + "scripts/lib/pkg/__init__.py": ["scripts/engine.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. @@ -717,7 +814,7 @@ def test_a_missing_library_an_installed_engine_imports_is_broken_not_sized_down( assert states["scripts/lib/kitconfig.py"] == "missing-required" assert [f.path for f in report.broken] == ["scripts/lib/kitconfig.py"] detail = next(f.detail for f in report.files if f.path == "scripts/lib/kitconfig.py") - assert detail == "imported by check_doc_budget.py" + assert detail == "needed by check_doc_budget.py" def test_a_missing_library_nothing_installed_imports_stays_an_ordinary_omission(tmp_path): @@ -762,11 +859,36 @@ def test_a_manifest_without_the_field_degrades_to_the_old_report(tmp_path): def test_a_broken_install_is_not_a_green_exit(tmp_path, capsys): + """The dependent is given its REAL hash, so it lands `unchanged` and + `report.drifted` is empty — otherwise the exit code proves nothing. + + The first version of this test used `_manifest`'s default `None` hash. That + makes the *present* dependent `unknown-version`, which is already counted in + `drifted`, so `assert code == 1` passed for a reason unrelated to the + mechanism under test: deleting `report.broken` from the exit expression + entirely left all 72 tests green. Found by the adversarial lens on PR #225 — + the doctrine's "named by a test and pinned by nothing" case, in a test + written to pin exactly that. + """ root = _fake_repo(tmp_path) - manifest = _manifest({"scripts/check_doc_budget.py": None, "scripts/lib/kitconfig.py": None}) + target = root / "scripts" / "check_doc_budget.py" + manifest = _manifest( + { + "scripts/check_doc_budget.py": kit_doctor.sha256_of(target), + "scripts/lib/kitconfig.py": None, + } + ) manifest["files"]["scripts/lib/kitconfig.py"]["required_by"] = ["scripts/check_doc_budget.py"] manifest_path = root / "kit-manifest.json" manifest_path.write_text(json.dumps(manifest), encoding="utf-8") + # Positive control on the fixture itself: if a future edit reintroduces an + # unrelated drifted entry, this fails rather than silently restoring the + # confound the assertion below is meant to be free of. + config = kit_doctor.load_config(root / "config" / "dev-model.yaml") + report = kit_doctor.inspect(root, manifest, config) + assert report.drifted == [], f"fixture is confounded: {[f.path for f in report.drifted]}" + assert [f.path for f in report.broken] == ["scripts/lib/kitconfig.py"] + code = kit_doctor.main(["--root", str(root), "--manifest", str(manifest_path)]) out = capsys.readouterr().out assert code == 1, "a tree whose engines cannot import their own library exited green" From b36ab689c133b3f95ad8f7f8d1a6d4f3f0a64be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Topi=20J=C3=A4rvinen?= Date: Sun, 2 Aug 2026 11:41:35 +0300 Subject: [PATCH 3/5] =?UTF-8?q?fix:=20the=20shell=20scan=20read=20one=20an?= =?UTF-8?q?chored=20line=20and=20trusted=20heredocs=20=E2=80=94=20both=20w?= =?UTF-8?q?rong?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- kit-manifest.json | 2 +- scripts/kit_doctor.py | 130 +++++++++++++++++++++++----- scripts/tests/test_kit_doctor.py | 143 +++++++++++++++++++++++++++++++ 3 files changed, 252 insertions(+), 23 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index df67377..3bb2e02 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -96,7 +96,7 @@ }, "scripts/kit_doctor.py": { "role": "engine", - "sha256": "bf95f87e48e2ad322033af2d7c6edb1cf138522def56a6362173ebdf3faa269b" + "sha256": "72f2aea41122d5f0fbbcc46e69191728187d8e5843cdf2e041c9b7b2eab0365d" }, "scripts/lib/atomic_write.py": { "required_by": [ diff --git a/scripts/kit_doctor.py b/scripts/kit_doctor.py index 473d303..cca8222 100755 --- a/scripts/kit_doctor.py +++ b/scripts/kit_doctor.py @@ -78,6 +78,7 @@ import ast import hashlib import json +import posixpath import re import sys from dataclasses import dataclass, field @@ -261,11 +262,47 @@ def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, . # than the text it replaced — "declining one is safe" — which is a worse false # assurance than the "decide, don't assume" it superseded. Found by the # adversarial lens on PR #225. -# The operand is captured to end-of-argument rather than with a quote-excluding -# character class, because `$(dirname "$0")/lib/x.sh` — the commonest form of -# this idiom — contains both a quote and a space INSIDE the expansion, and a -# class-based match truncates it to `$(dirname`. -_SOURCE_RE = re.compile(r"""^[ \t]*(?:source|\.)[ \t]+(\S[^;#\n]*)""", re.MULTILINE) +# The operand: a double-quoted string, a single-quoted string, or a bare word. +# The double-quoted alternative admits `$( … )` chunks explicitly, because +# `"$(dirname "$0")/lib/x.sh"` — the commonest form of this idiom — nests a +# quote INSIDE the quoted operand, and a plain `"[^"]*"` truncates it to +# `"$(dirname "`. +# +# The BARE alternative stops at a shell separator rather than at whitespace: +# `{ source lib/dep.sh; }` and `source lib/dep.sh; cat <]+))""" + +# What may precede `source` on its line: nothing, or a command separator, or a +# block keyword. The first version of this anchored at line start only, which +# silently dropped `[ -f "$LIB" ] && source "$LIB"` — a defensively-written and +# arguably *better* engine than the bare form the two current shell engines use +# — and dropped `source x && echo` too, because the operand ran to end of line +# and then failed the literal-path filter. Both reproduced; found by the +# adversarial lens, PR #225 round 2. +_SOURCE_RE = re.compile(r"(?:^|[;&|{]|\bthen\b|\bdo\b)[ \t]*(?:source|\.)[ \t]+" + _OPERAND) + +# A `#` at line start or after whitespace begins a comment. Approximate — a `#` +# inside a quoted string preceded by a space is treated as one — and the +# approximation is deliberately in the false-NEGATIVE direction: it can only +# suppress an edge, never invent one. Dropping the line-start anchor above +# without this would make a commented-out `# source lib/dep.sh` produce a real +# dependency edge. +_COMMENT_RE = re.compile(r"(?:^|[ \t])#") + +# `<&2 < tuple[str, . # What is left must be a literal relative path. Anything still carrying a shell # metacharacter is computed at run time, and the honest answer there is no edge # rather than a guessed one. -_NOT_A_LITERAL_PATH = re.compile(r"""[\s$"'*?\[\]{}()|&<>]""") +# +# NOT INDEPENDENTLY PINNABLE, and recorded as such rather than left looking like +# tested behaviour: `record()` already requires an exact match against +# KIT_OWNED, so a candidate this filter rejects would be discarded there anyway +# — deleting the filter changes no observable output and fails no test. It is +# kept as the second of two checks because the operand capture is deliberately +# permissive, and it states an intent `record()`'s membership test does not. +# Adversarial lens, PR #225 round 2; the backtick was added to the class in the +# same round, since a single-token `source \`depfile\`` slipped through a class +# whose whole job is rejecting substitutions. +_NOT_A_LITERAL_PATH = re.compile(r"""[\s$`"'*?\[\]{}()|&<>]""") def _imported_modules(text: str) -> set[tuple[int, str]]: @@ -322,14 +369,38 @@ def _sourced_paths(text: str, rel: str) -> set[str]: """ here = PurePosixPath(rel).parent found: set[str] = set() - for match in _SOURCE_RE.finditer(text): - operand = match.group(1).strip() - if len(operand) > 1 and operand[0] in "\"'" and operand[-1] == operand[0]: - operand = operand[1:-1] - tail = _SHELL_PREFIX_RE.sub("", operand) - if not tail or tail.startswith("/") or _NOT_A_LITERAL_PATH.search(tail): + delimiter: str | None = None + for line in text.splitlines(): + if delimiter is not None: + if line.strip() == delimiter: + delimiter = None continue - found.add(str(PurePosixPath(here / tail))) + # Detected before the scan but applied after it: the line that OPENS a + # heredoc is itself ordinary code and can carry its own `source`. + opening = _HEREDOC_RE.search(line) + comment = _COMMENT_RE.search(line) + code = line[: comment.start()] if comment else line + for match in _SOURCE_RE.finditer(code): + quoted_double, quoted_single, bare = match.groups() + operand = next(g for g in (quoted_double, quoted_single, bare) if g is not None) + tail = _SHELL_PREFIX_RE.sub("", operand) + if not tail or tail.startswith("/") or _NOT_A_LITERAL_PATH.search(tail): + continue + # normpath, not a bare join: `PurePosixPath` does NOT collapse `..`, + # so `scripts/sub` / `../lib/dep.sh` stayed `scripts/sub/../lib/dep.sh` + # and could never equal the canonical KIT_OWNED path it genuinely + # names — a literal, unconditional dependency dropped in silence. + # That is #41's own failure class reached by a different route, and + # it is NOT the documented "computed at run time" bound: nothing + # here is computed. Correctness lens, PR #225 round 2. + target = posixpath.normpath(str(here / tail)) + # A path that climbs out of the repo cannot name a KIT_OWNED file; + # recording it would put `../…` in the manifest. + if target.startswith(".."): + continue + found.add(target) + if opening: + delimiter = opening.group(1) return found @@ -373,14 +444,27 @@ def derive_dependencies( one this can see. - A dynamic import (``importlib``, an ``__import__`` call) is invisible, as is a `source` whose path is computed at run time. No file THIS FUNCTION - SCANS does either today — the qualifier is load-bearing and an earlier - version of this sentence dropped it, claiming "nothing in the tree", - which is false: `importlib.util` is used in `test_portability.py`, - `test_check_memory_budget.py` and `test_pr_followup_hook.py`. Those are - test files, neither `engine` nor `hook`, so they are out of scope here — - but the sentence read as a claim about the whole tree. A future ENGINE - that did either would get a silently thin graph, and no test here would - catch it. (Correctness lens, PR #225.) + SCANS does either today; the qualifier is load-bearing, because several + TEST modules do use `importlib.util`, and they are neither `engine` nor + `hook` so they are out of scope. A future ENGINE that did either would get + a silently thin graph, and no test here would catch it. + + This sentence has now been wrong twice — first claiming "nothing in the + tree does this", then naming three test modules when there are four — so + it no longer enumerates them. `grep -rln importlib scripts/` is the + answer, and unlike a list in a docstring it cannot go stale. + (Correctness lens, PR #225 rounds 1 and 2.) + - The shell scan reads one physical line at a time. `source` is found after + a separator or a block keyword, and inside a `{ … }` group, but a + continuation (`source \\` + newline + path) or a `source` reached only + through a shell function or `eval` is not. Both directions of this bound + were reported as undocumented in round 2 and are now pinned by tests + rather than left implicit. + - Every one of these bounds errs toward NO edge. That direction is chosen, + not incidental: a missing edge degrades a file to plain `missing`, which + is the pre-#41 behaviour and merely unhelpful, while a false edge tells an + adopter their working install is broken and to install something they do + not need. `missing-required` firing wrongly is worse than it not firing. """ owned = {rel for rel, _ in kit_owned} dependents: dict[str, set[str]] = {} @@ -602,7 +686,9 @@ def generate_manifest(root: Path, kit_version: int) -> dict: ``required_by`` is written only where the derived dependent set is non-empty — most kit files are needed by nothing, and an entry per file would be - thirty empty lists to read past in every manifest diff. A reader must + twenty-six empty lists to read past in every manifest diff (32 KIT_OWNED + entries, 6 with a dependent; the earlier figure of "thirty" was a guess and + the correctness lens on PR #225 computed the real one). A reader must therefore treat an ABSENT key as "no known dependents", which is also what an older manifest (written before this field existed) yields: it reports every missing file as an ordinary `missing`, exactly as it did before. diff --git a/scripts/tests/test_kit_doctor.py b/scripts/tests/test_kit_doctor.py index 9042df0..633dc1b 100644 --- a/scripts/tests/test_kit_doctor.py +++ b/scripts/tests/test_kit_doctor.py @@ -673,6 +673,18 @@ def test_the_shell_engines_source_dependency_is_in_the_graph(): "source ${SCRIPT_DIR}/lib/dep.sh", 'source "$(dirname "$0")/lib/dep.sh"', ". $SCRIPT_DIR/lib/dep.sh", + # Everything below was a SILENT MISS in the first version of this + # scanner, which anchored `source` to the start of its physical line. + # A guarded source is the more defensive idiom — arguably better bash + # than the bare form both current shell engines use — so the scanner + # was blindest to the engines most careful about the very dependency + # it exists to find. Adversarial lens, PR #225 round 2. + '[ -f "$SCRIPT_DIR/lib/dep.sh" ] && source "$SCRIPT_DIR/lib/dep.sh"', + 'if true; then source "$SCRIPT_DIR/lib/dep.sh"; fi', + 'source "$SCRIPT_DIR/lib/dep.sh" || true', + "source lib/dep.sh && echo ready", + "{ source lib/dep.sh; }", + "source lib/dep.sh; echo ready", ], ) def test_the_shell_scan_strips_the_leading_expansion(tmp_path, line): @@ -689,6 +701,137 @@ def test_the_shell_scan_strips_the_leading_expansion(tmp_path, line): } +@pytest.mark.parametrize( + "line", + [ + "# source lib/dep.sh", + "echo ready # source lib/dep.sh", + # The one that matters: dropping the line-start anchor to catch guarded + # `source` also lets a `&&` inside a COMMENT reach the scanner. Nothing + # pinned that, so the mutation removing the anchor passed the whole + # suite while turning commented-out prose into a dependency edge. + "# run cmd && source lib/dep.sh to wire it up", + ], +) +def test_a_commented_out_source_is_not_an_edge(tmp_path, line): + """A false edge is worse than a missing one, and this is the direction the + round-2 widening could have broken. + + A bogus `required_by` entry baked into the manifest tells every adopter who + has the importing file and not the target that their install is BROKEN and + to install something they do not need — the `missing-required` state firing + in reverse, which is the failure `missing` vs `missing-required` exists to + prevent.""" + root = _tree( + tmp_path, + {"scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", "scripts/lib/dep.sh": "\n"}, + ) + owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == {} + + +def test_a_source_inside_a_heredoc_body_is_not_an_edge(tmp_path): + """A `source` line inside a heredoc is text being PRINTED, not a dependency. + + `pre-push` already carries two `cat >&2 <&2 <<'EOF'\n" + "To wire this up by hand:\n" + ' source "$SCRIPT_DIR/lib/dep.sh"\n' + "EOF\n" + "python3 - <<'PY'\n" + "from kitconfig import get\n" + "PY\n" + ), + "scripts/lib/dep.sh": "\n", + "scripts/lib/kitconfig.py": "def get(): ...\n", + }, + ) + owned = ( + ("scripts/engine.sh", "engine"), + ("scripts/lib/dep.sh", "engine"), + ("scripts/lib/kitconfig.py", "engine"), + ) + assert kit_doctor.derive_dependencies(root, owned) == { + "scripts/lib/kitconfig.py": ["scripts/engine.sh"] + } + + +def test_code_on_a_heredoc_opening_line_is_still_scanned(tmp_path): + """The line that OPENS a heredoc is ordinary code and can carry its own + `source`; only the body is skipped. Detecting the opener before scanning + the line, and applying it after, is what makes that true — the obvious + ordering drops a real edge.""" + root = _tree( + tmp_path, + { + "scripts/engine.sh": ( + "#!/usr/bin/env bash\nsource lib/dep.sh; cat < Date: Sun, 2 Aug 2026 12:09:29 +0300 Subject: [PATCH 4/5] fix: withdraw the widened source anchor, and bound the heredoc tracker structurally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- kit-manifest.json | 2 +- scripts/kit_doctor.py | 134 ++++++++++++++++------ scripts/tests/test_kit_doctor.py | 189 ++++++++++++++++++++++++++----- 3 files changed, 260 insertions(+), 65 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index 3bb2e02..fe8af49 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -96,7 +96,7 @@ }, "scripts/kit_doctor.py": { "role": "engine", - "sha256": "72f2aea41122d5f0fbbcc46e69191728187d8e5843cdf2e041c9b7b2eab0365d" + "sha256": "b8ba4e595297805f22dbd9744fffdbae6fab50f2914ee30a8f665b32d49921cd" }, "scripts/lib/atomic_write.py": { "required_by": [ diff --git a/scripts/kit_doctor.py b/scripts/kit_doctor.py index cca8222..e5ebaee 100755 --- a/scripts/kit_doctor.py +++ b/scripts/kit_doctor.py @@ -276,22 +276,28 @@ def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, . # separator handling, not by a lens. _OPERAND = r"""(?:"((?:\$\([^)]*\)|[^"])*)"|'([^']*)'|([^\s;&|<>]+))""" -# What may precede `source` on its line: nothing, or a command separator, or a -# block keyword. The first version of this anchored at line start only, which -# silently dropped `[ -f "$LIB" ] && source "$LIB"` — a defensively-written and -# arguably *better* engine than the bare form the two current shell engines use -# — and dropped `source x && echo` too, because the operand ran to end of line -# and then failed the literal-path filter. Both reproduced; found by the -# adversarial lens, PR #225 round 2. -_SOURCE_RE = re.compile(r"(?:^|[;&|{]|\bthen\b|\bdo\b)[ \t]*(?:source|\.)[ \t]+" + _OPERAND) - -# A `#` at line start or after whitespace begins a comment. Approximate — a `#` -# inside a quoted string preceded by a space is treated as one — and the -# approximation is deliberately in the false-NEGATIVE direction: it can only -# suppress an edge, never invent one. Dropping the line-start anchor above -# without this would make a commented-out `# source lib/dep.sh` produce a real -# dependency edge. -_COMMENT_RE = re.compile(r"(?:^|[ \t])#") +# `source` must be the FIRST token on its line. This is a deliberate, +# re-affirmed bound, not an oversight — and the history is worth keeping because +# the obvious improvement was tried and withdrawn: +# +# Round 2 widened it to match after a separator or block keyword, so that +# `[ -f "$LIB" ] && source "$LIB"` would be seen. Round 3 showed what that cost. +# The scan reads raw text and has no notion of "inside a string", so +# `echo "please then source lib/dep.sh now"` — ordinary prose in an ordinary +# echo — produced a real dependency edge. That is the harmful direction: a bogus +# `required_by` in the manifest tells an adopter whose install is FINE that it is +# broken and to install a file they do not need, which is `missing-required` +# firing in reverse. +# +# Deciding command position correctly needs a bash tokenizer — quote state, +# escapes, `$'…'`, line continuations, multi-line strings — and every regex +# approximation of it found in three rounds leaked in one direction or the +# other. Anchoring has exactly ONE failure direction, and it is the safe one: a +# guarded `source` is missed, which degrades that file to plain `missing`, i.e. +# the pre-#41 behaviour. Unhelpful, never misleading. Both shell engines in this +# tree use the anchored form, so nothing real is missed today; #228 holds the +# proper fix. The miss is pinned by a test so it stays a stated bound. +_SOURCE_RE = re.compile(r"^[ \t]*(?:source|\.)[ \t]+" + _OPERAND) # `< tuple[str, . # install is broken. Note the Python-import scan deliberately does NOT skip # heredocs: `pre-push`'s kitconfig import lives inside a `python3 - <<'PY'` # body and genuinely executes. Adversarial lens, PR #225 round 2. -_HEREDOC_RE = re.compile(r"<<-?[ \t]*[\"']?([A-Za-z_][A-Za-z0-9_]*)[\"']?") +# +# The `<` guards are load-bearing and were missing at first: `<<<` is a +# HERESTRING with no body, but an unguarded `<<-?` matched its second and third +# characters and took the following word as a delimiter — so `read x <<< "$out"` +# put the scanner into a heredoc that never closed, silently deleting every real +# edge for the REST OF THE FILE. That is strictly worse than the false positive +# the heredoc skip was added to fix, and the pre-round-2 code got it right by +# having no heredoc concept at all. Adversarial lens, PR #225 round 3. +_HEREDOC_RE = re.compile(r"(? tuple[str, ...]: return (f"{package_dir}/{base}.py", f"{package_dir}/{base}/__init__.py") +def _heredoc_body_lines(lines: list[str]) -> set[int]: + """Indices of lines inside a heredoc body — but only for bodies that CLOSE. + + The two-pass shape is the point, and it is a structural bound rather than + another regex patch. A single-pass tracker treats a mis-detected opener as + the start of a body that runs to end of file, so ONE false positive deletes + every real dependency edge after it. Three separate constructs were found + tripping that in one review round — `<<<` herestrings, and `<<` inside + `$(( … ))` arithmetic, both of which look exactly like `< set[str]: """Kit-layout paths this shell file `source`s, resolved against its own dir. @@ -366,21 +418,26 @@ def _sourced_paths(text: str, rel: str) -> set[str]: resolves to nothing and produces no edge. Nothing in the tree does that today, and no test here would catch a future engine that did — the same stated limit `derive_dependencies` records for dynamic Python imports. + + Heredoc bodies are skipped: a `source` line inside one is text being + printed, not a dependency. The line that OPENS a heredoc is ordinary code + and is still scanned. """ here = PurePosixPath(rel).parent found: set[str] = set() - delimiter: str | None = None - for line in text.splitlines(): - if delimiter is not None: - if line.strip() == delimiter: - delimiter = None + lines = text.splitlines() + skip = _heredoc_body_lines(lines) + for number, line in enumerate(lines): + if number in skip: continue - # Detected before the scan but applied after it: the line that OPENS a - # heredoc is itself ordinary code and can carry its own `source`. - opening = _HEREDOC_RE.search(line) - comment = _COMMENT_RE.search(line) - code = line[: comment.start()] if comment else line - for match in _SOURCE_RE.finditer(code): + # No comment stripping: with `source` anchored to the first token, a + # commented line cannot match in the first place — `#` is not `source`. + # Round 2 carried a `_COMMENT_RE` because the widened anchor could reach + # a `source` after a `&&` INSIDE a comment; withdrawing that widening + # removed the only thing it defended against, and keeping a guard whose + # justification no longer holds is how dead code acquires authority. + # The commented-out-source tests still pass, now via the anchor. + for match in _SOURCE_RE.finditer(line): quoted_double, quoted_single, bare = match.groups() operand = next(g for g in (quoted_double, quoted_single, bare) if g is not None) tail = _SHELL_PREFIX_RE.sub("", operand) @@ -399,8 +456,6 @@ def _sourced_paths(text: str, rel: str) -> set[str]: if target.startswith(".."): continue found.add(target) - if opening: - delimiter = opening.group(1) return found @@ -454,12 +509,19 @@ def derive_dependencies( it no longer enumerates them. `grep -rln importlib scripts/` is the answer, and unlike a list in a docstring it cannot go stale. (Correctness lens, PR #225 rounds 1 and 2.) - - The shell scan reads one physical line at a time. `source` is found after - a separator or a block keyword, and inside a `{ … }` group, but a - continuation (`source \\` + newline + path) or a `source` reached only - through a shell function or `eval` is not. Both directions of this bound - were reported as undocumented in round 2 and are now pinned by tests - rather than left implicit. + - The shell scan requires `source` to be the FIRST token on its line. A + guarded (`[ -f "$L" ] && source "$L"`), nested, continued, or + `eval`-reached `source` is not seen. Widening this was tried in round 2 + and withdrawn in round 3: without a bash tokenizer the widened form could + not tell a command position from the word `then` inside a string, and + manufactured edges out of prose. #228 holds the proper fix. + + Of these, the GUARDED and NESTED forms are pinned by a test so they stay + stated rather than implicit. The continuation, `eval` and shell-function + forms are NOT pinned by anything — said plainly because an earlier version + of this paragraph claimed "both directions of this bound are now pinned by + tests", which the correctness lens checked and found false: `grep -n + "continuation\\|eval" scripts/tests/test_kit_doctor.py` returns nothing. - Every one of these bounds errs toward NO edge. That direction is chosen, not incidental: a missing edge degrades a file to plain `missing`, which is the pre-#41 behaviour and merely unhelpful, while a false edge tells an diff --git a/scripts/tests/test_kit_doctor.py b/scripts/tests/test_kit_doctor.py index 633dc1b..5a53edc 100644 --- a/scripts/tests/test_kit_doctor.py +++ b/scripts/tests/test_kit_doctor.py @@ -673,18 +673,12 @@ def test_the_shell_engines_source_dependency_is_in_the_graph(): "source ${SCRIPT_DIR}/lib/dep.sh", 'source "$(dirname "$0")/lib/dep.sh"', ". $SCRIPT_DIR/lib/dep.sh", - # Everything below was a SILENT MISS in the first version of this - # scanner, which anchored `source` to the start of its physical line. - # A guarded source is the more defensive idiom — arguably better bash - # than the bare form both current shell engines use — so the scanner - # was blindest to the engines most careful about the very dependency - # it exists to find. Adversarial lens, PR #225 round 2. - '[ -f "$SCRIPT_DIR/lib/dep.sh" ] && source "$SCRIPT_DIR/lib/dep.sh"', - 'if true; then source "$SCRIPT_DIR/lib/dep.sh"; fi', + # Trailing content after the operand: the operand stops at a shell + # separator rather than running to end of line, so these are found. 'source "$SCRIPT_DIR/lib/dep.sh" || true', "source lib/dep.sh && echo ready", - "{ source lib/dep.sh; }", "source lib/dep.sh; echo ready", + "source 'lib/dep.sh'", ], ) def test_the_shell_scan_strips_the_leading_expansion(tmp_path, line): @@ -706,22 +700,30 @@ def test_the_shell_scan_strips_the_leading_expansion(tmp_path, line): [ "# source lib/dep.sh", "echo ready # source lib/dep.sh", - # The one that matters: dropping the line-start anchor to catch guarded - # `source` also lets a `&&` inside a COMMENT reach the scanner. Nothing - # pinned that, so the mutation removing the anchor passed the whole - # suite while turning commented-out prose into a dependency edge. "# run cmd && source lib/dep.sh to wire it up", + # Ordinary prose in an ordinary echo. This is what killed round 2's + # widened anchor: matching `source` after a separator or block keyword + # cannot tell a real command position from the word `then` inside a + # string, so this line produced a REAL dependency edge. Regex has no + # notion of quote state; deciding command position needs a tokenizer. + # Adversarial lens, PR #225 round 3. + 'echo "please then source lib/dep.sh now"', + "printf '%s' 'run: source lib/dep.sh'", ], ) -def test_a_commented_out_source_is_not_an_edge(tmp_path, line): - """A false edge is worse than a missing one, and this is the direction the - round-2 widening could have broken. +def test_prose_mentioning_source_is_not_an_edge(tmp_path, line): + """A false edge is worse than a missing one, and this is the direction that + withdrew the round-2 widening. A bogus `required_by` entry baked into the manifest tells every adopter who has the importing file and not the target that their install is BROKEN and - to install something they do not need — the `missing-required` state firing - in reverse, which is the failure `missing` vs `missing-required` exists to - prevent.""" + to install something they do not need — `missing-required` firing in + reverse, which is the failure the `missing` split exists to prevent. + + These pass via the line-start anchor, not via any comment-stripping step: + round 2 carried one, and withdrawing the widening removed the only thing it + defended against. + """ root = _tree( tmp_path, {"scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", "scripts/lib/dep.sh": "\n"}, @@ -730,6 +732,133 @@ def test_a_commented_out_source_is_not_an_edge(tmp_path, line): assert kit_doctor.derive_dependencies(root, owned) == {} +@pytest.mark.parametrize( + "line", + [ + '[ -f "$SCRIPT_DIR/lib/dep.sh" ] && source "$SCRIPT_DIR/lib/dep.sh"', + 'if true; then source "$SCRIPT_DIR/lib/dep.sh"; fi', + "{ source lib/dep.sh; }", + "while read -r x; do source lib/dep.sh; done", + ], +) +def test_a_guarded_source_is_a_KNOWN_MISS_not_an_accident(tmp_path, line): + """Asserted so the bound stays STATED. `source` must be the first token on + its line; a guarded or nested one is not seen. + + Round 2 widened the anchor to catch exactly these and round 3 withdrew it — + the widened form could not tell a command position from the word `then` + inside a string, and manufactured edges from prose. The trade is deliberate: + anchoring fails in ONE direction, and a missed edge degrades the file to + plain `missing`, which is the pre-#41 behaviour — unhelpful, never + misleading. Neither shell engine in this tree uses a guarded form, so + nothing real is missed today. #228 holds the proper fix. + + If a future change makes any of these produce an edge, this test fails — + which is correct: that change must arrive with a real command-position + parser and this test rewritten, not as an incidental loosening. + """ + root = _tree( + tmp_path, + {"scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", "scripts/lib/dep.sh": "\n"}, + ) + owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) + assert kit_doctor.derive_dependencies(root, owned) == {} + + +@pytest.mark.parametrize( + "herestring", + [ + 'read -r x <<< "$out"', + "printf '%s' greeting <<< greeting", + "mapfile -t arr <<<$out", + # `<<` inside arithmetic looks identical to `< Date: Sun, 2 Aug 2026 12:36:11 +0300 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20withdraw=20the=20shell=20source=20sc?= =?UTF-8?q?an=20entirely=20=E2=80=94=20it=20can=20manufacture=20false=20ed?= =?UTF-8?q?ges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 < "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. --- .claude/commands/upgrade.md | 34 +-- kit-manifest.json | 6 +- scripts/kit_doctor.py | 225 +++--------------- scripts/tests/test_kit_doctor.py | 385 ++++--------------------------- 4 files changed, 90 insertions(+), 560 deletions(-) diff --git a/.claude/commands/upgrade.md b/.claude/commands/upgrade.md index b2414ad..d624b67 100644 --- a/.claude/commands/upgrade.md +++ b/.claude/commands/upgrade.md @@ -46,8 +46,8 @@ kit's copy against this repo: `uv run /tmp/agentic-dev-kit/scripts/kit_doctor.py --manifest /tmp/agentic-dev-kit/kit-manifest.json`. The report gives you, per kit-owned file: `unchanged` / `differs` / `missing` / -`unknown-version`, plus four installation-level checks. **Read all four** — each is a -silent failure mode: +`missing-required` / `unknown-version`, plus four installation-level checks. +**Read all four** — each is a silent failure mode: - **config schema version** — unversioned or behind means migrations are pending. - **`paths.engines` resolves to a directory that actually holds engines.** A `✗` here is @@ -114,26 +114,34 @@ too. Confirm with `git branch --show-current` before the first copy. **Install every `missing-required` file first, before any other copy in this step.** Those are the kit's own libraries — `lib/kitconfig.py` above all, which every Python -engine imports — and refreshing an engine on top of an absent one produces a broken +engine imports — and refreshing a component on top of an absent one produces a broken install: `check_doc_budget.py` dies with `ModuleNotFoundError`, and `pr_watch.py` warns and silently falls back to built-in defaults, leaving the adopter's entire `review.*` -config inert. `kit_doctor` derives this set from the import graph, so it is answering -"what does *this* tree's installed engines need", not a fixed list. +config inert. `kit_doctor` derives this set from the Python import graph, so it is +answering "what do *this* tree's installed components need", not a fixed list. + +**Then re-run `kit_doctor` after installing anything.** The set is computed against the +components present *when the report ran*: a file is `missing-required` only if something +that depends on it is already installed. So installing a previously-`missing` engine or +hook can introduce requirements the first report had no reason to classify. Re-run +before you rely on the list again, and treat the report as converged only when a run +that installed nothing still shows no `missing-required`. - **`missing-required`** → install it. This is the one absent-file case that is **not** - an operator decision: an engine that is already installed imports it, and the report - names which. Do not carry it into the `missing` conversation below. + an operator decision: an installed component depends on it, and the report names + which. Do not carry it into the `missing` conversation below. - **`unchanged`** → copy the new version straight in. It is provably untouched, so there is nothing to lose. - **`missing`** → decide, don't assume. A sized-down adoption omits engines deliberately (one surveyed repo installs 2 of 6 on purpose). Ask the operator whether each missing piece is wanted before installing it. If a piece stays out, note it in the PR body so - the next upgrade doesn't re-litigate it. Nothing installed here imports these **by - the graph `kit_doctor` derives** — Python imports plus shell `source` — which is what - separates them from the bullet above. That graph cannot see a dependency whose path - is computed at run time, so it is a much better prior than the old blanket "decide, - don't assume", not a proof; if a piece you are declining is one an engine plausibly - reaches for, check before dropping it. + the next upgrade doesn't re-litigate it. Nothing installed here depends on these **by + the graph `kit_doctor` derives**, which is what separates them from the bullet above. + That graph covers **Python imports only** — it does not read shell `source`, so + `lib/repo_root.sh` (which `dev_session.sh` and `reconcile_sessions.sh` both source) + will appear here rather than above. It is a much better prior than the old blanket + "decide, don't assume", not a proof: if a piece you are declining is a library a + shell component plausibly reaches for, check before dropping it. - **`differs`** → `diff` the local file against the kit's, and read the diff: - Only kit-authored changes (the local copy is simply older) → replace it. - Local edits present → for each, find where that value now lives in diff --git a/kit-manifest.json b/kit-manifest.json index fe8af49..97b30c4 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -96,7 +96,7 @@ }, "scripts/kit_doctor.py": { "role": "engine", - "sha256": "b8ba4e595297805f22dbd9744fffdbae6fab50f2914ee30a8f665b32d49921cd" + "sha256": "a70626b4f784f56ec683d2239150a2e39ed866ff0f28f599cc877c4abbf13b6f" }, "scripts/lib/atomic_write.py": { "required_by": [ @@ -124,10 +124,6 @@ "sha256": "4ab496661883d8f4ad590a6612a48b31f8cbf770283bb09794096149276634e6" }, "scripts/lib/repo_root.sh": { - "required_by": [ - "scripts/dev_session.sh", - "scripts/reconcile_sessions.sh" - ], "role": "engine", "sha256": "980cbf5596cea67033a5dd02d53630f2a92c24afd693ba7727d5fc50303ff555" }, diff --git a/scripts/kit_doctor.py b/scripts/kit_doctor.py index e5ebaee..7c7b3db 100755 --- a/scripts/kit_doctor.py +++ b/scripts/kit_doctor.py @@ -39,8 +39,9 @@ deliberate omission and to ask before installing it — so the documented path invited someone to decline `lib/kitconfig.py`, which every Python engine imports (issue #41). Which files these are is DERIVED at - ``--generate-manifest`` time from the dependency graph — Python imports and - shell ``source`` — not restated by hand. + ``--generate-manifest`` time from the PYTHON import graph, not restated by + hand. Shell ``source`` is deliberately NOT scanned — see + `derive_dependencies` for the gap that leaves and why it is left open. ``unknown-version`` The manifest has no entry for this file, so drift can't be judged. @@ -78,7 +79,6 @@ import ast import hashlib import json -import posixpath import re import sys from dataclasses import dataclass, field @@ -252,6 +252,7 @@ def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, . # `python3 - <<'PY'` heredoc. _TEXT_IMPORT_RE = re.compile(r"^\s*(?:from|import)\s+([A-Za-z_][A-Za-z0-9_.]*)", re.MULTILINE) + # Bash `source` / `.` of another kit file. A shell engine's dependency is a PATH, # not a module name, so it needs its own scanner — and without one the graph # fails open on exactly the pair #41 is about: `dev_session.sh` and @@ -262,83 +263,6 @@ def _derive_engine_names(kit_owned: tuple[tuple[str, str], ...]) -> tuple[str, . # than the text it replaced — "declining one is safe" — which is a worse false # assurance than the "decide, don't assume" it superseded. Found by the # adversarial lens on PR #225. -# The operand: a double-quoted string, a single-quoted string, or a bare word. -# The double-quoted alternative admits `$( … )` chunks explicitly, because -# `"$(dirname "$0")/lib/x.sh"` — the commonest form of this idiom — nests a -# quote INSIDE the quoted operand, and a plain `"[^"]*"` truncates it to -# `"$(dirname "`. -# -# The BARE alternative stops at a shell separator rather than at whitespace: -# `{ source lib/dep.sh; }` and `source lib/dep.sh; cat <]+))""" - -# `source` must be the FIRST token on its line. This is a deliberate, -# re-affirmed bound, not an oversight — and the history is worth keeping because -# the obvious improvement was tried and withdrawn: -# -# Round 2 widened it to match after a separator or block keyword, so that -# `[ -f "$LIB" ] && source "$LIB"` would be seen. Round 3 showed what that cost. -# The scan reads raw text and has no notion of "inside a string", so -# `echo "please then source lib/dep.sh now"` — ordinary prose in an ordinary -# echo — produced a real dependency edge. That is the harmful direction: a bogus -# `required_by` in the manifest tells an adopter whose install is FINE that it is -# broken and to install a file they do not need, which is `missing-required` -# firing in reverse. -# -# Deciding command position correctly needs a bash tokenizer — quote state, -# escapes, `$'…'`, line continuations, multi-line strings — and every regex -# approximation of it found in three rounds leaked in one direction or the -# other. Anchoring has exactly ONE failure direction, and it is the safe one: a -# guarded `source` is missed, which degrades that file to plain `missing`, i.e. -# the pre-#41 behaviour. Unhelpful, never misleading. Both shell engines in this -# tree use the anchored form, so nothing real is missed today; #228 holds the -# proper fix. The miss is pinned by a test so it stays a stated bound. -_SOURCE_RE = re.compile(r"^[ \t]*(?:source|\.)[ \t]+" + _OPERAND) - -# `<&2 <]""") - - def _imported_modules(text: str) -> set[tuple[int, str]]: """``(relative_level, dotted_name)`` for every import in `text`. @@ -372,93 +296,6 @@ def _module_targets(module: str, package_dir: str) -> tuple[str, ...]: return (f"{package_dir}/{base}.py", f"{package_dir}/{base}/__init__.py") -def _heredoc_body_lines(lines: list[str]) -> set[int]: - """Indices of lines inside a heredoc body — but only for bodies that CLOSE. - - The two-pass shape is the point, and it is a structural bound rather than - another regex patch. A single-pass tracker treats a mis-detected opener as - the start of a body that runs to end of file, so ONE false positive deletes - every real dependency edge after it. Three separate constructs were found - tripping that in one review round — `<<<` herestrings, and `<<` inside - `$(( … ))` arithmetic, both of which look exactly like `< set[str]: - """Kit-layout paths this shell file `source`s, resolved against its own dir. - - Only the leading shell expansion is stripped; a `source` whose path is - genuinely computed at run time (a loop variable, a value read from config) - resolves to nothing and produces no edge. Nothing in the tree does that - today, and no test here would catch a future engine that did — the same - stated limit `derive_dependencies` records for dynamic Python imports. - - Heredoc bodies are skipped: a `source` line inside one is text being - printed, not a dependency. The line that OPENS a heredoc is ordinary code - and is still scanned. - """ - here = PurePosixPath(rel).parent - found: set[str] = set() - lines = text.splitlines() - skip = _heredoc_body_lines(lines) - for number, line in enumerate(lines): - if number in skip: - continue - # No comment stripping: with `source` anchored to the first token, a - # commented line cannot match in the first place — `#` is not `source`. - # Round 2 carried a `_COMMENT_RE` because the widened anchor could reach - # a `source` after a `&&` INSIDE a comment; withdrawing that widening - # removed the only thing it defended against, and keeping a guard whose - # justification no longer holds is how dead code acquires authority. - # The commented-out-source tests still pass, now via the anchor. - for match in _SOURCE_RE.finditer(line): - quoted_double, quoted_single, bare = match.groups() - operand = next(g for g in (quoted_double, quoted_single, bare) if g is not None) - tail = _SHELL_PREFIX_RE.sub("", operand) - if not tail or tail.startswith("/") or _NOT_A_LITERAL_PATH.search(tail): - continue - # normpath, not a bare join: `PurePosixPath` does NOT collapse `..`, - # so `scripts/sub` / `../lib/dep.sh` stayed `scripts/sub/../lib/dep.sh` - # and could never equal the canonical KIT_OWNED path it genuinely - # names — a literal, unconditional dependency dropped in silence. - # That is #41's own failure class reached by a different route, and - # it is NOT the documented "computed at run time" bound: nothing - # here is computed. Correctness lens, PR #225 round 2. - target = posixpath.normpath(str(here / tail)) - # A path that climbs out of the repo cannot name a KIT_OWNED file; - # recording it would put `../…` in the manifest. - if target.startswith(".."): - continue - found.add(target) - return found - - def derive_dependencies( root: Path, kit_owned: tuple[tuple[str, str], ...] = KIT_OWNED ) -> dict[str, list[str]]: @@ -480,14 +317,27 @@ def derive_dependencies( `state_paths` package's internal edges (``from .resolver import …``) in the graph rather than only its top-level name. - **The shell engines are scanned too, and must be.** `dev_session.sh` and - `reconcile_sessions.sh` `source "$SCRIPT_DIR/lib/repo_root.sh"`, which is a - hard dependency expressed as a path rather than a module. A Python-only - version of this function reported such a tree exit-0 clean while - ``bash scripts/dev_session.sh`` died on its `source` line — #41's own bug - class, reproduced by the change meant to close it. `.py` files take the - `ast` path and only that; everything else is scanned for both Python-style - imports (the `pre-push` heredoc) and shell `source`. + **Shell `source` is NOT scanned, and this is the one real gap.** + `dev_session.sh` and `reconcile_sessions.sh` both + `source "$SCRIPT_DIR/lib/repo_root.sh"` — a hard dependency expressed as a + path rather than a module — so a tree missing `lib/repo_root.sh` reports it + as an ordinary `missing` while `bash scripts/dev_session.sh` dies on its + `source` line. That is a live instance of #41's own bug class and it is + knowingly left open; #228 carries it. + + A scanner for it was built across three review rounds and withdrawn. The + short version: preventing FALSE edges requires recognising every real + heredoc opener, and preventing MISSED edges requires knowing whether a + `source` is in command position — both are tokenizer problems, and each + regex approximation leaked in one direction or the other. A false edge is + the harmful direction (it tells an adopter their working install is broken), + and shipping a mechanism that can produce them is worse than shipping the + Python-only graph plus a documented hole. #228 has the full account, + including the four constructs that defeated the last version. + + Non-Python files ARE still scanned for Python-style imports: `pre-push` is + bash, and its `from kitconfig import …` lives inside a `python3 - <<'PY'` + heredoc that genuinely executes. LIMITS, because a graph that overstates its coverage is worse than a short one: @@ -509,24 +359,14 @@ def derive_dependencies( it no longer enumerates them. `grep -rln importlib scripts/` is the answer, and unlike a list in a docstring it cannot go stale. (Correctness lens, PR #225 rounds 1 and 2.) - - The shell scan requires `source` to be the FIRST token on its line. A - guarded (`[ -f "$L" ] && source "$L"`), nested, continued, or - `eval`-reached `source` is not seen. Widening this was tried in round 2 - and withdrawn in round 3: without a bash tokenizer the widened form could - not tell a command position from the word `then` inside a string, and - manufactured edges out of prose. #228 holds the proper fix. - - Of these, the GUARDED and NESTED forms are pinned by a test so they stay - stated rather than implicit. The continuation, `eval` and shell-function - forms are NOT pinned by anything — said plainly because an earlier version - of this paragraph claimed "both directions of this bound are now pinned by - tests", which the correctness lens checked and found false: `grep -n - "continuation\\|eval" scripts/tests/test_kit_doctor.py` returns nothing. + - Shell `source` is not scanned at all, so `lib/repo_root.sh`'s two + dependents are absent from the graph. Stated above; #228 carries it. - Every one of these bounds errs toward NO edge. That direction is chosen, not incidental: a missing edge degrades a file to plain `missing`, which is the pre-#41 behaviour and merely unhelpful, while a false edge tells an adopter their working install is broken and to install something they do - not need. `missing-required` firing wrongly is worse than it not firing. + not need. `missing-required` firing wrongly is worse than it not firing, + and that asymmetry is what decided the shell-scan question above. """ owned = {rel for rel, _ in kit_owned} dependents: dict[str, set[str]] = {} @@ -552,13 +392,6 @@ def record(target: str, importer: str) -> None: package_dir = f"{KIT_ENGINE_PREFIX}/lib" for candidate in _module_targets(module, package_dir): record(candidate, rel) - # Dispatched on the SUFFIX, not on whether `ast` happened to succeed: 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. - if not rel.endswith(".py"): - for candidate in _sourced_paths(text, rel): - record(candidate, rel) return {path: sorted(deps) for path, deps in sorted(dependents.items())} diff --git a/scripts/tests/test_kit_doctor.py b/scripts/tests/test_kit_doctor.py index 5a53edc..d3c9a65 100644 --- a/scripts/tests/test_kit_doctor.py +++ b/scripts/tests/test_kit_doctor.py @@ -649,351 +649,6 @@ def test_dependency_graph_of_the_real_kit_names_kitconfigs_importers(): } -def test_the_shell_engines_source_dependency_is_in_the_graph(): - """A shell engine's dependency is a PATH, not a module, and the first version - of `derive_dependencies` scanned only Python. - - Consequence, reproduced by the adversarial lens on PR #225: with - `lib/repo_root.sh` deleted, `kit_doctor` reported the tree exit-0 clean and - listed the file as an ordinary sized-down omission, while - `bash scripts/dev_session.sh` died on its `source` line. That is #41's own - failure mode, reproduced by the change written to close it. - """ - graph = kit_doctor.derive_dependencies(REPO_ROOT) - assert graph.get("scripts/lib/repo_root.sh") == [ - "scripts/dev_session.sh", - "scripts/reconcile_sessions.sh", - ] - - -@pytest.mark.parametrize( - "line", - [ - 'source "$SCRIPT_DIR/lib/dep.sh"', - "source ${SCRIPT_DIR}/lib/dep.sh", - 'source "$(dirname "$0")/lib/dep.sh"', - ". $SCRIPT_DIR/lib/dep.sh", - # Trailing content after the operand: the operand stops at a shell - # separator rather than running to end of line, so these are found. - 'source "$SCRIPT_DIR/lib/dep.sh" || true', - "source lib/dep.sh && echo ready", - "source lib/dep.sh; echo ready", - "source 'lib/dep.sh'", - ], -) -def test_the_shell_scan_strips_the_leading_expansion(tmp_path, line): - root = _tree( - tmp_path, - { - "scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", - "scripts/lib/dep.sh": "x() { :; }\n", - }, - ) - owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) - assert kit_doctor.derive_dependencies(root, owned) == { - "scripts/lib/dep.sh": ["scripts/engine.sh"] - } - - -@pytest.mark.parametrize( - "line", - [ - "# source lib/dep.sh", - "echo ready # source lib/dep.sh", - "# run cmd && source lib/dep.sh to wire it up", - # Ordinary prose in an ordinary echo. This is what killed round 2's - # widened anchor: matching `source` after a separator or block keyword - # cannot tell a real command position from the word `then` inside a - # string, so this line produced a REAL dependency edge. Regex has no - # notion of quote state; deciding command position needs a tokenizer. - # Adversarial lens, PR #225 round 3. - 'echo "please then source lib/dep.sh now"', - "printf '%s' 'run: source lib/dep.sh'", - ], -) -def test_prose_mentioning_source_is_not_an_edge(tmp_path, line): - """A false edge is worse than a missing one, and this is the direction that - withdrew the round-2 widening. - - A bogus `required_by` entry baked into the manifest tells every adopter who - has the importing file and not the target that their install is BROKEN and - to install something they do not need — `missing-required` firing in - reverse, which is the failure the `missing` split exists to prevent. - - These pass via the line-start anchor, not via any comment-stripping step: - round 2 carried one, and withdrawing the widening removed the only thing it - defended against. - """ - root = _tree( - tmp_path, - {"scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", "scripts/lib/dep.sh": "\n"}, - ) - owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) - assert kit_doctor.derive_dependencies(root, owned) == {} - - -@pytest.mark.parametrize( - "line", - [ - '[ -f "$SCRIPT_DIR/lib/dep.sh" ] && source "$SCRIPT_DIR/lib/dep.sh"', - 'if true; then source "$SCRIPT_DIR/lib/dep.sh"; fi', - "{ source lib/dep.sh; }", - "while read -r x; do source lib/dep.sh; done", - ], -) -def test_a_guarded_source_is_a_KNOWN_MISS_not_an_accident(tmp_path, line): - """Asserted so the bound stays STATED. `source` must be the first token on - its line; a guarded or nested one is not seen. - - Round 2 widened the anchor to catch exactly these and round 3 withdrew it — - the widened form could not tell a command position from the word `then` - inside a string, and manufactured edges from prose. The trade is deliberate: - anchoring fails in ONE direction, and a missed edge degrades the file to - plain `missing`, which is the pre-#41 behaviour — unhelpful, never - misleading. Neither shell engine in this tree uses a guarded form, so - nothing real is missed today. #228 holds the proper fix. - - If a future change makes any of these produce an edge, this test fails — - which is correct: that change must arrive with a real command-position - parser and this test rewritten, not as an incidental loosening. - """ - root = _tree( - tmp_path, - {"scripts/engine.sh": f"#!/usr/bin/env bash\n{line}\n", "scripts/lib/dep.sh": "\n"}, - ) - owned = (("scripts/engine.sh", "engine"), ("scripts/lib/dep.sh", "engine")) - assert kit_doctor.derive_dependencies(root, owned) == {} - - -@pytest.mark.parametrize( - "herestring", - [ - 'read -r x <<< "$out"', - "printf '%s' greeting <<< greeting", - "mapfile -t arr <<<$out", - # `<<` inside arithmetic looks identical to `<&2 <&2 <<'EOF'\n" - "To wire this up by hand:\n" - ' source "$SCRIPT_DIR/lib/dep.sh"\n' - "EOF\n" - "python3 - <<'PY'\n" - "from kitconfig import get\n" - "PY\n" - ), - "scripts/lib/dep.sh": "\n", - "scripts/lib/kitconfig.py": "def get(): ...\n", - }, - ) - owned = ( - ("scripts/engine.sh", "engine"), - ("scripts/lib/dep.sh", "engine"), - ("scripts/lib/kitconfig.py", "engine"), - ) - assert kit_doctor.derive_dependencies(root, owned) == { - "scripts/lib/kitconfig.py": ["scripts/engine.sh"] - } - - -def test_code_on_a_heredoc_opening_line_is_still_scanned(tmp_path): - """The line that OPENS a heredoc is ordinary code and can carry its own - `source`; only the body is skipped. Detecting the opener before scanning - the line, and applying it after, is what makes that true — the obvious - ordering drops a real edge.""" - root = _tree( - tmp_path, - { - "scripts/engine.sh": ( - "#!/usr/bin/env bash\nsource lib/dep.sh; cat </__init__.py` candidate in `_module_targets`. Nothing in the kit imports a KIT_OWNED package by bare name today, so deleting that candidate @@ -1013,6 +668,40 @@ def test_importing_a_package_by_name_resolves_to_its_init(tmp_path): } +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 + in the graph. Asserted so the gap stays stated. + + A scanner for it was built and withdrawn across three review rounds. Both + directions failed: preventing FALSE edges needs every real heredoc opener + recognised (`cmd <