diff --git a/.agents/skills/align-recipe-pyproject/SKILL.md b/.agents/skills/align-recipe-pyproject/SKILL.md index 44d9561375..61d5397d67 100644 --- a/.agents/skills/align-recipe-pyproject/SKILL.md +++ b/.agents/skills/align-recipe-pyproject/SKILL.md @@ -15,7 +15,7 @@ description: > metadata: author: Google license: Apache-2.0 - version: 1.0.0 + version: 1.1.0 --- # Align Recipe pyproject.toml @@ -28,7 +28,7 @@ Scope: **`pyproject.toml` only**. Standalone `ruff.toml` / `.ruff.toml` files ar ## What This Skill Checks -Runs `scripts/align_pyproject.py` against a recipe directory. Six rules: +Runs `scripts/align_pyproject.py` against a recipe directory. Eight rules: | Rule ID | What it checks | Auto-fix | |---|---|---| @@ -38,6 +38,28 @@ Runs `scripts/align_pyproject.py` against a recipe directory. Six rules: | `description-matches-manifest` | If `[project].description` is set, it must equal `manifest.description`. Field is optional; skipped when absent. | Only with `--description-source={pyproject,manifest,delete}`. Refuses to touch description otherwise. | | `build-system-present` | `[build-system]` must have both `requires` and `build-backend`. Without it, `uv build` and `pip install .` fail. | **No** — backend choice is editorial. Reported for the human to fix. | | `default-pypi-index` | `[[tool.uv.index]]` must have an entry with `default = true` pointing at public PyPI (`https://pypi.org/simple[/]`). Required so `uv sync` works on Google corp workstations without corp Airlock auth — see the block comment in the root `pyproject.toml` for the full rationale. | Yes when the block is entirely missing — appends it. **No** when a default entry exists but points elsewhere (custom private index, TestPyPI, mirror) — reported for the human to reconcile, since the divergence may be intentional. | +| `stale-python-version-refs` | Scans **every text file in the recipe** for references to a Python version below the 3.11 floor. Raising `requires-python` is not a self-contained edit: the version is typically repeated in prose (`README.md`, `SKILL.md`) and in executable setup code. | **No** — report-only. Which references are stale and how to reword them is editorial. | +| `runnability-test-in-testpaths` | If `[tool.pytest.ini_options].testpaths` is set, at least one entry must collect `tests/test_runnability.py` (which `.github/policy.yml` requires every Python recipe to have). | **No** — report-only. Broadening `testpaths` changes what CI collects. | + +#### Why `stale-python-version-refs` exists + +A bootstrap script that picks an interpreter from an allowlist still +containing the old floor will happily build a venv the recipe then refuses to +install into: + +```bash +for py in python3.13 python3.12 python3.11 python3.10 python3; do +# ... +# ERROR: Package requires a different Python: 3.10.x not in '>=3.11' +``` + +The scan requires a Python-ish context (`python 3.10`, `Python :: 3.10`, +`>=3.10`) rather than matching bare digits, so a model name like +`gemini-3.5-flash` is not a false positive. It skips lockfiles, virtualenvs, +caches, and binary files, and ignores `requires-python` in `pyproject.toml` +itself (owned by `python-version-floor`, and still pre-rewrite in memory when +this check runs). Hits are capped at 40 in `details.hits`, but +`details.files` always lists every affected file. ### Edit safety @@ -66,9 +88,13 @@ Runs `scripts/align_pyproject.py` against a recipe directory. Six rules: 7. **If `default-pypi-index` returns `report_only`**, the recipe declares a default index that is NOT public PyPI (e.g. a private mirror, TestPyPI). The skill will not overwrite an intentional choice. Show the user the current `url` from `details.current_url` and ask whether it's deliberate. If yes, they can `# noqa`-comment it or update the repo standard; if no, they should change the URL to `https://pypi.org/simple/`. Do not auto-rewrite. -8. **After apply mode succeeds**, remind the user to run `uv sync` in the recipe directory if the pyproject changes touched dependencies (the script emits this in `notes` when relevant). +8. **If `stale-python-version-refs` returns `report_only`**, list the affected files from `details.files` and call out any **executable** ones first (`.sh`, `.py`, `Makefile`, CI YAML) — a stale interpreter allowlist in a bootstrap script is a live bug, not a docs nit, and it produces a confusing `requires a different Python` failure at install time. Prose files (`README.md`, `SKILL.md`) are lower priority but still inconsistent. Offer to update them; do not rewrite without asking, since some references are legitimately historical ("dropped 3.10 support in v2"). + +9. **If `runnability-test-in-testpaths` returns `report_only`**, the recipe's `testpaths` excludes `tests/test_runnability.py`, so a bare `uv run pytest` never runs it — the recipe looks tested while its import-smoke test silently never executes. Show `details.testpaths` and suggest adding `"tests"`. Do not auto-rewrite. + +10. **After apply mode succeeds**, remind the user to run `uv sync` in the recipe directory if the pyproject changes touched dependencies (the script emits this in `notes` when relevant). -9. **Do not commit any changes.** Show the diff or file contents; let the user commit. +11. **Do not commit any changes.** Show the diff or file contents; let the user commit. --- @@ -168,7 +194,7 @@ Status-specific guidance for what to put in the **Details** cell: - **`would_fix`** (dry-run) — describe the current-state problem, then say what apply would do. Include the `from` → `to` or the list of tables to be removed. - **`fixed`** (apply) — one-liner confirming the change (new value or list of removed tables). - **`needs_input`** (only `description-matches-manifest`) — the Details cell says something like `"descriptions differ — needs --description-source={pyproject,manifest,delete}"`. Do not put the two long descriptions inside the table. See "Follow-up content" below. -- **`report_only`** (two rules can hit this: `build-system-present` and `default-pypi-index` when a non-PyPI default is declared) — the Details cell names what's missing or non-conforming (e.g. `"[build-system] missing; recipe cannot be built as a package"` or `"default index is TestPyPI, not public PyPI"`). Follow-up content goes below the table (see next section). +- **`report_only`** (four rules can hit this: `build-system-present`, `default-pypi-index` when a non-PyPI default is declared, `stale-python-version-refs`, and `runnability-test-in-testpaths`) — the Details cell names what's missing or non-conforming (e.g. `"[build-system] missing; recipe cannot be built as a package"` or `"default index is TestPyPI, not public PyPI"`). Follow-up content goes below the table (see next section). - **`error`** — the Details cell shows the message verbatim; if it's very long, truncate with `…` and put the full text below. ### Follow-up content below the table @@ -210,6 +236,8 @@ Only for statuses that need extra context. Order: table first, then this content - **Dry-run with only `report_only`** (and no `would_fix` rows) — after the table, address the specific case: - `build-system-present`: show the two `[build-system]` template snippets and stop. This is a manual edit; the skill does not auto-fix it. - `default-pypi-index`: quote `details.current_url`, explain that this is not public PyPI, and ask whether it's intentional. If not, tell the user to change the URL to `https://pypi.org/simple/`. Do not auto-rewrite. + - `stale-python-version-refs`: list `details.files`, executable files first, and offer to update them. + - `runnability-test-in-testpaths`: quote `details.testpaths` and suggest adding `"tests"`. - **Dry-run with only `error` rows** — do not offer to apply. Errors mean the script bailed before it could compute a fix; the user has to resolve the underlying issue first. diff --git a/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py b/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py index 343bcf265c..a7a311af44 100644 --- a/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py +++ b/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py @@ -79,6 +79,8 @@ import argparse import json +import os +import re import sys from dataclasses import asdict, dataclass, field from pathlib import Path @@ -170,9 +172,7 @@ def to_json(self) -> str: # ---------- no-local-ruff-config: no [tool.ruff*] tables ------------------- -def check_no_local_ruff_config( - doc: tomlkit.TOMLDocument, apply: bool -) -> Check: +def check_no_local_ruff_config(doc: tomlkit.TOMLDocument, apply: bool) -> Check: """Remove any [tool.ruff*] table from pyproject.toml. Assumes doc["tool"] (if present) is a table — run() validates that the @@ -321,9 +321,7 @@ def _validate_and_apply_python_floor_rewrite( ) -def check_python_version_floor( - doc: tomlkit.TOMLDocument, apply: bool -) -> Check: +def check_python_version_floor(doc: tomlkit.TOMLDocument, apply: bool) -> Check: """Ensure [project].requires-python is compatible with MIN_PYTHON. Interpretation B (aligned with CI in .github/workflows/ @@ -905,6 +903,224 @@ def check_default_pypi_index(doc: tomlkit.TOMLDocument, apply: bool) -> Check: ) +# ---------- stale-python-version-refs (report-only) ------------------------ +# +# Raising [project].requires-python is not a self-contained edit. A recipe +# typically repeats its supported Python version in prose (README, SKILL.md) +# and — far more importantly — in EXECUTABLE setup code. A bootstrap script +# that picks an interpreter from an allowlist still containing the old floor +# will happily build a venv the recipe then refuses to install into: +# +# for py in python3.13 python3.12 python3.11 python3.10 python3; do +# ... +# ERROR: Package requires a different Python: 3.10.x not in '>=3.11' +# +# This check is report-only: which of the hits matter, and how to reword +# them, is the maintainer's call. Its job is simply that the bump never lands +# silently. + +# Directories that are never the recipe's own source. +_SCAN_SKIP_DIRS = frozenset( + { + ".git", + ".ruff_cache", + ".mypy_cache", + ".pytest_cache", + "__pycache__", + ".venv", + "venv", + "env", + "build", + "dist", + "node_modules", + } +) + +# Files whose Python-version mentions are generated, not authored. +_SCAN_SKIP_NAMES = frozenset({"uv.lock", "poetry.lock", "Pipfile.lock"}) + +# Two contexts that make "3.x" a Python-version reference rather than a +# coincidence. Without the context requirement, a model name like +# "gemini-3.5-flash" would match and drown the report in false positives. +_PY_REF_PATTERNS = ( + # python3.10 / python 3.10 / Python 3.10 / python@3.10 / python_3.10, and + # the trove classifier "Programming Language :: Python :: 3.10" (hence + # ':' in the separator class and a width of 4). + re.compile(r"python[\s@._:-]{0,4}3\.(\d+)", re.IGNORECASE), + # >=3.10 / ==3.9 / ~=3.10 — version-specifier syntax + re.compile(r"[<>=~!]=\s*3\.(\d+)"), +) + +# Cap the reported hits so a pathological recipe can't produce a wall of JSON. +_MAX_REPORTED_REFS = 40 + + +def _iter_scannable_files(recipe_dir: Path) -> list[Path]: + """Every text-ish file in the recipe that could carry a version claim.""" + found: list[Path] = [] + for root, dirs, files in os.walk(recipe_dir): + dirs[:] = [ + d + for d in dirs + if d not in _SCAN_SKIP_DIRS and not d.endswith(".egg-info") + ] + for name in sorted(files): + if name in _SCAN_SKIP_NAMES: + continue + found.append(Path(root) / name) + return sorted(found) + + +# `requires-python` is owned by the python-version-floor check, which rewrites +# it in-memory and persists only at the end of run(). Scanning it here would +# report the pre-rewrite value as a stale reference in apply mode even though +# the file on disk ends up correct — a pure false positive either way, since +# the floor check already reports that key authoritatively. +_REQUIRES_PYTHON_LINE_RE = re.compile(r"^\s*requires-python\s*=") + + +def _sub_floor_refs_in_text( + text: str, skip_requires_python: bool = False +) -> list[tuple[int, str]]: + """Return (line_number, line) for lines claiming a sub-floor Python.""" + hits: list[tuple[int, str]] = [] + for lineno, line in enumerate(text.splitlines(), start=1): + if skip_requires_python and _REQUIRES_PYTHON_LINE_RE.match(line): + continue + for pattern in _PY_REF_PATTERNS: + if any( + int(minor) < MIN_PYTHON[1] + for minor in pattern.findall(line) + if minor.isdigit() + ): + hits.append((lineno, line.strip())) + break + return hits + + +def check_stale_python_version_refs(recipe_dir: Path) -> Check: + """Report references to a Python version below the enforced floor.""" + offenders: dict[str, list[dict[str, Any]]] = {} + total = 0 + for path in _iter_scannable_files(recipe_dir): + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue # binary or unreadable — not a prose/version claim + rel = str(path.relative_to(recipe_dir)) + hits = _sub_floor_refs_in_text( + text, skip_requires_python=(rel == "pyproject.toml") + ) + if not hits: + continue + offenders[rel] = [ + {"line": lineno, "text": line} for lineno, line in hits + ] + total += len(hits) + + if not offenders: + return Check( + "stale-python-version-refs", + OK, + f"No references to a Python version below {MIN_PYTHON_STR}.", + ) + + # Trim the payload without losing the file list. + trimmed = dict(offenders) + if total > _MAX_REPORTED_REFS: + budget = _MAX_REPORTED_REFS + trimmed = {} + for rel, hits in offenders.items(): + if budget <= 0: + break + trimmed[rel] = hits[:budget] + budget -= len(trimmed[rel]) + + return Check( + "stale-python-version-refs", + REPORT_ONLY, + f"{total} reference(s) to a Python version below {MIN_PYTHON_STR} " + f"across {len(offenders)} file(s): {', '.join(sorted(offenders))}. " + f"[project].requires-python is enforced at >={MIN_PYTHON_STR}, so " + f"these are now inconsistent. Executable ones matter most — an " + f"interpreter-picking loop in a bootstrap script that still accepts " + f"the old floor will build a venv the recipe then refuses to install " + f"into. This skill does not auto-fix: which references are stale and " + f"how to reword them is editorial.", + { + "floor": MIN_PYTHON_STR, + "total": total, + "files": sorted(offenders), + "hits": trimmed, + "truncated": total > _MAX_REPORTED_REFS, + }, + ) + + +# ---------- runnability-test-in-testpaths (report-only) -------------------- + + +def _testpaths_cover_runnability(entries: list[str]) -> bool: + """Whether any testpaths entry collects tests/test_runnability.py.""" + for raw in entries: + entry = str(raw).strip().rstrip("/") + if entry in {"", ".", "tests", "tests/test_runnability.py"}: + return True + return False + + +def check_runnability_test_in_testpaths(doc: tomlkit.TOMLDocument) -> Check: + """Report a `testpaths` setting that excludes the runnability test. + + `tests/test_runnability.py` is a required file for Python recipes + (.github/policy.yml), but a narrower `testpaths` — e.g. + `["tests/unit", "tests/integration"]` — means a bare `uv run pytest` + never collects it. The recipe then looks tested while its one + import-smoke test silently never runs. + """ + ini = ( + doc.get("tool", {}).get("pytest", {}).get("ini_options", {}) + if doc.get("tool") is not None + else {} + ) + entries = ini.get("testpaths") if hasattr(ini, "get") else None + if entries is None: + return Check( + "runnability-test-in-testpaths", + OK, + "No [tool.pytest.ini_options].testpaths — pytest collects the " + "whole recipe, including tests/test_runnability.py.", + ) + if isinstance(entries, str): + entries = [entries] + if not isinstance(entries, (list, tuple)): + return Check( + "runnability-test-in-testpaths", + OK, + "testpaths is not a list; nothing to check.", + ) + + listed = [str(e) for e in entries] + if _testpaths_cover_runnability(listed): + return Check( + "runnability-test-in-testpaths", + OK, + f"testpaths {listed} collects tests/test_runnability.py.", + ) + + return Check( + "runnability-test-in-testpaths", + REPORT_ONLY, + f"[tool.pytest.ini_options].testpaths is {listed}, none of which " + f"collects tests/test_runnability.py — the required runnability " + f'test would never run under a bare `pytest`. Add "tests" to ' + f"testpaths, or narrow it deliberately and run the file by path. " + f"Not auto-fixed: broadening testpaths changes what CI collects, " + f"which is the maintainer's call.", + {"testpaths": listed, "expected": "tests/test_runnability.py"}, + ) + + # ---------- Orchestration ------------------------------------------------- @@ -1081,9 +1297,7 @@ def run( report.add( _run_check( "project-name-matches-folder", - lambda: check_project_name_matches_folder( - recipe_dir, doc, apply - ), + lambda: check_project_name_matches_folder(recipe_dir, doc, apply), ) ) report.add( @@ -1107,6 +1321,18 @@ def run( lambda: check_default_pypi_index(doc, apply), ) ) + report.add( + _run_check( + "stale-python-version-refs", + lambda: check_stale_python_version_refs(recipe_dir), + ) + ) + report.add( + _run_check( + "runnability-test-in-testpaths", + lambda: check_runnability_test_in_testpaths(doc), + ) + ) # Persist edits — only in apply mode, and only if at least one auto-fix # actually changed something. diff --git a/.agents/skills/align-recipe-pyproject/tests/conftest.py b/.agents/skills/align-recipe-pyproject/tests/conftest.py new file mode 100644 index 0000000000..2d55a0f3c1 --- /dev/null +++ b/.agents/skills/align-recipe-pyproject/tests/conftest.py @@ -0,0 +1,24 @@ +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Make this skill's scripts/ directory importable from its tests. + +Keeping the path shim inside the skill (rather than in the repo-root pytest +config) preserves the skill as a self-contained, portable bundle. +""" + +import sys +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).parent.parent / "scripts" +sys.path.insert(0, str(SCRIPTS_DIR)) diff --git a/.agents/skills/align-recipe-pyproject/tests/test_align_pyproject.py b/.agents/skills/align-recipe-pyproject/tests/test_align_pyproject.py new file mode 100644 index 0000000000..7b0c47a732 --- /dev/null +++ b/.agents/skills/align-recipe-pyproject/tests/test_align_pyproject.py @@ -0,0 +1,208 @@ +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Unit tests for the align-recipe-pyproject skill script. + +Covers the two checks added after a vertical skill under skills/ showed that +raising `requires-python` silently desynced the recipe's own bootstrap script, +and that a narrow `testpaths` can exclude the required runnability test. +""" + +from pathlib import Path + +import align_pyproject as m +import tomlkit + + +def _write(path: Path, content: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + return path + + +def _check(recipe_dir: Path) -> m.Check: + return m.check_stale_python_version_refs(recipe_dir) + + +# --------------------------------------------------------------------------- +# stale-python-version-refs +# --------------------------------------------------------------------------- + + +def test_clean_recipe_reports_ok(tmp_path): + _write(tmp_path / "README.md", "- Python 3.11+\n") + _write(tmp_path / "pyproject.toml", 'requires-python = ">=3.11"\n') + assert _check(tmp_path).status == m.OK + + +def test_flags_prose_version_claim(tmp_path): + _write(tmp_path / "README.md", "# R\n\n- Python 3.10+\n") + check = _check(tmp_path) + assert check.status == m.REPORT_ONLY + assert check.details["files"] == ["README.md"] + + +def test_flags_interpreter_allowlist_in_shell_script(tmp_path): + # The case that actually broke a recipe: bootstrap.sh would still build a + # 3.10 venv that the recipe then refuses to install into. + _write( + tmp_path / "scripts" / "bootstrap.sh", + "for py in python3.12 python3.11 python3.10 python3; do\n", + ) + check = _check(tmp_path) + assert check.status == m.REPORT_ONLY + assert check.details["files"] == ["scripts/bootstrap.sh"] + + +def test_flags_version_specifier_syntax(tmp_path): + _write(tmp_path / "TROUBLE.md", "not in '>=3.10'\n") + assert _check(tmp_path).status == m.REPORT_ONLY + + +def test_does_not_flag_model_names(tmp_path): + # "gemini-3.5-flash" contains "3.5" but is not a Python version claim. + # A context-free digit scan would drown the report in these. + _write( + tmp_path / "agent.py", + 'MODEL = "gemini-3.5-flash"\nOTHER = "gemini-2.0-pro"\n', + ) + assert _check(tmp_path).status == m.OK + + +def test_does_not_flag_versions_at_or_above_floor(tmp_path): + _write( + tmp_path / "bootstrap.sh", + "for py in python3.13 python3.12 python3.11; do\n", + ) + assert _check(tmp_path).status == m.OK + + +def test_ignores_requires_python_line_in_pyproject(tmp_path): + # Owned by the python-version-floor check, which rewrites it in memory + # and persists at the end of run(); reporting it here would be a false + # positive in apply mode. + _write(tmp_path / "pyproject.toml", 'requires-python = ">=3.10"\n') + assert _check(tmp_path).status == m.OK + + +def test_still_flags_other_pyproject_lines(tmp_path): + _write( + tmp_path / "pyproject.toml", + 'requires-python = ">=3.10"\n' + 'classifiers = ["Programming Language :: Python :: 3.10"]\n', + ) + check = _check(tmp_path) + assert check.status == m.REPORT_ONLY + assert check.details["hits"]["pyproject.toml"][0]["line"] == 2 + + +def test_skips_lockfiles_and_venvs(tmp_path): + _write(tmp_path / "uv.lock", 'requires-python = ">=3.10"\n') + _write(tmp_path / ".venv" / "x.py", "# python3.9\n") + _write(tmp_path / "__pycache__" / "y.txt", "python3.9\n") + assert _check(tmp_path).status == m.OK + + +def test_binary_file_does_not_crash_the_scan(tmp_path): + (tmp_path / "logo.png").write_bytes(b"\x89PNG\r\n\x1a\n\xff\xfe") + _write(tmp_path / "README.md", "- Python 3.10+\n") + check = _check(tmp_path) + assert check.status == m.REPORT_ONLY + assert check.details["files"] == ["README.md"] + + +def test_hits_are_truncated_but_file_list_is_not(tmp_path): + body = "\n".join(f"python3.10 line {i}" for i in range(60)) + _write(tmp_path / "many.txt", body) + check = _check(tmp_path) + assert check.details["truncated"] is True + assert check.details["total"] == 60 + reported = sum(len(v) for v in check.details["hits"].values()) + assert reported == m._MAX_REPORTED_REFS + assert check.details["files"] == ["many.txt"] + + +# --------------------------------------------------------------------------- +# runnability-test-in-testpaths +# --------------------------------------------------------------------------- + + +def _testpaths_check(toml: str) -> m.Check: + return m.check_runnability_test_in_testpaths(tomlkit.parse(toml)) + + +def test_no_testpaths_is_ok(): + assert _testpaths_check('[project]\nname = "x"\n').status == m.OK + + +def test_narrow_testpaths_is_reported(): + check = _testpaths_check( + "[tool.pytest.ini_options]\n" + 'testpaths = ["tests/unit", "tests/integration"]\n' + ) + assert check.status == m.REPORT_ONLY + assert check.details["testpaths"] == ["tests/unit", "tests/integration"] + + +def test_tests_dir_in_testpaths_is_ok(): + check = _testpaths_check( + '[tool.pytest.ini_options]\ntestpaths = ["tests", "docs"]\n' + ) + assert check.status == m.OK + + +def test_trailing_slash_is_normalised(): + check = _testpaths_check( + '[tool.pytest.ini_options]\ntestpaths = ["tests/"]\n' + ) + assert check.status == m.OK + + +def test_dot_testpath_is_ok(): + check = _testpaths_check('[tool.pytest.ini_options]\ntestpaths = ["."]\n') + assert check.status == m.OK + + +def test_explicit_runnability_path_is_ok(): + check = _testpaths_check( + '[tool.pytest.ini_options]\ntestpaths = ["tests/test_runnability.py"]\n' + ) + assert check.status == m.OK + + +def test_string_testpaths_is_handled(): + check = _testpaths_check( + '[tool.pytest.ini_options]\ntestpaths = "tests/unit"\n' + ) + assert check.status == m.REPORT_ONLY + + +# --------------------------------------------------------------------------- +# Both checks are wired into run() +# --------------------------------------------------------------------------- + + +def test_new_checks_appear_in_report(tmp_path): + _write( + tmp_path / "pyproject.toml", + '[project]\nname = "alrec"\ndescription = "d"\n' + 'requires-python = ">=3.11"\n' + '[tool.pytest.ini_options]\ntestpaths = ["tests/unit"]\n', + ) + _write(tmp_path / "README.md", "- Python 3.10+\n") + + report = m.run(tmp_path, dry_run=True, description_source=None) + by_id = {c.id: c for c in report.checks} + + assert by_id["stale-python-version-refs"].status == m.REPORT_ONLY + assert by_id["runnability-test-in-testpaths"].status == m.REPORT_ONLY diff --git a/.agents/skills/extract-python-environment-variables/SKILL.md b/.agents/skills/extract-python-environment-variables/SKILL.md index 9a9705cfd1..9951162d73 100644 --- a/.agents/skills/extract-python-environment-variables/SKILL.md +++ b/.agents/skills/extract-python-environment-variables/SKILL.md @@ -13,10 +13,15 @@ description: > (`"my-project-id"`, `"changeme"`, `"<...>"`) are downgraded to the TODO placeholder but the source string is preserved in the marker comment. Also detects hardcoded model-name string literals (e.g. - `"gemini-3.5-flash"` in `agent.py`) and rewrites them to bare - `os.getenv("MODEL_NAME")` (single model) or - `os.getenv("MODEL_NAME_GENERATED_1")` / `MODEL_NAME_GENERATED_2`, … - (multiple models) — no fallback default in the Python source. The + `"gemini-3.5-flash"` in `agent.py`) and rewrites them to an + `os.getenv(...)` call. The variable name is derived from the assignment + target when it names a model (`DEFAULT_EMBEDDING_MODEL` → + `EMBEDDING_MODEL`), else `MODEL_NAME` (single model) or + `MODEL_NAME_GENERATED_1` / `MODEL_NAME_GENERATED_2`, … (multiple models). + Normally no fallback default is written into the Python source; the one + exception is when no `load_dotenv()` bootstrap could be installed (no + package `__init__.py`), where the original literal is kept as the + fallback so the lookup cannot evaluate to `None` at runtime. The model string is written as the value in `.env.example` with a comment prompting a rename. When re-run against a recipe whose `.env.example` already has entries, the writer classifies each entry (skill-authored vs. user-authored, @@ -46,7 +51,7 @@ description: > metadata: author: Google license: Apache-2.0 - version: 2.1.0 + version: 2.3.0 --- # Extract Python Environment Variables @@ -115,6 +120,12 @@ Runs `scripts/extract_env_vars.py` against a recipe directory. The script: If `load_dotenv` is already present the injection is skipped. + **If no package `__init__.py` exists** (common in vertical skills under + `skills/`, where the code lives in a plain `scripts/` directory rather + than an importable package) the injection is skipped with a `[WARN]`, + and the step reports that no bootstrap is in place. Step 4 below depends + on that answer. + **Additionally — always, regardless of whether we injected** — appends `# noqa: E402 -- must come after load_dotenv()` to any top-level relative import (`from .x import y`) that sits AFTER a non-import module-level @@ -137,13 +148,66 @@ Runs `scripts/extract_env_vars.py` against a recipe directory. The script: Idempotent: a line that already carries `# noqa: E402` is skipped. 4. **Replaces hardcoded model names** in source (e.g. `model="gemini-3.5-flash"` - in `agent.py`) with **bare `os.getenv(...)`** — no default argument: - - Single model → `os.getenv("MODEL_NAME")` - - Multiple models → `os.getenv("MODEL_NAME_GENERATED_1")`, `os.getenv("MODEL_NAME_GENERATED_2")`, … (sorted alphabetically for determinism) + in `agent.py`) with an `os.getenv(...)` call. + + **Position decides eligibility.** A model string is only promoted when it + is a *configurable constant*. Three positions mean it is something else, + and are left untouched: + + - **Collection-literal entries** (dict keys and values, list/set/tuple + items) — lookup tables and enumerations of supported models: + + ```python + IMAGE_MODELS = { + "flash": "gemini-2.5-flash-image", + "pro": "gemini-2.5-pro-image", + } + ``` + + Rewriting the values collapses the table onto one env var; rewriting the + **keys** is worse, because `IMAGE_MODELS.get("flash")` then never + matches anything. A dict key must stay a static literal. + - **Subscript indices** — `IMAGE_MODELS["gemini-3.1"]` is a key *into* + such a table, so replacing it looks up a different entry. + - **Comparison operands** — `if "gemini-3.1" in model_id` tests a value + rather than configuring one. + + Skipping a legitimate extraction is cheap (lift it by hand); silently + breaking a lookup table is not, so this errs towards skipping. Every + skipped literal is listed in an `[INFO]` block, so nothing is hidden — and + because these strings never reach the naming step, a dict target like + `IMAGE_MODELS` can no longer leak into `.env.example` as a variable name. + + **Variable name.** The assignment target that holds the literal is used + when it names a model, since it carries far more meaning than a generic + fallback — `DEFAULT_EMBEDDING_MODEL = "gemini-embedding-001"` and + `embedding_model = cfg.get("embedding_model", "gemini-embedding-001")` + both yield `EMBEDDING_MODEL`. This matters in recipes that already read a + *different* model var (say `GEMINI_MODEL` for the LLM), where a second + bare `MODEL_NAME` would be actively misleading. When a string is assigned + to conflicting target names, the most frequent wins, ties broken + alphabetically. Otherwise: + - A single unnamed model → `MODEL_NAME` + - The rest → `MODEL_NAME_GENERATED_1`, `MODEL_NAME_GENERATED_2`, … (sorted + alphabetically for determinism) + + **Fallback argument.** Normally the emitted call is **bare** — + `os.getenv("EMBEDDING_MODEL")`, no default — because default values are + the maintainer's decision, not the skill's. That is only safe when the + `load_dotenv()` bootstrap from step 3 is in place to populate the + environment. + + **When step 3 could not install a bootstrap** (no package `__init__.py`), + nothing reads `.env`, so a bare lookup would evaluate to `None` at runtime + and silently break the recipe. In that case the original literal is + preserved as the fallback — `os.getenv("EMBEDDING_MODEL", + "gemini-embedding-001")` — which keeps behaviour identical to before the + rewrite while still lifting the value into the environment. The step logs + an `[INFO]` line explaining the choice. The actual model string is written as the value in `.env.example` (e.g. - `MODEL_NAME_GENERATED_1=gemini-3.5-flash`) with a comment prompting the - maintainer to rename the variable to something meaningful before shipping. + `EMBEDDING_MODEL=gemini-embedding-001`) with a comment prompting the + maintainer to rename the variable if the derived name isn't right. 5. **Updates `pyproject.toml`** — adds `python-dotenv>=1.0.0` to `[project]` dependencies if it is not already there. @@ -195,7 +259,9 @@ are LEFT UNTOUCHED. The skill's only writes to Python files are: (once, only if not already present). - Appending `# noqa: E402` to trailing relative imports that would otherwise trip Ruff after the env-bootstrap block. -- Replacing hardcoded model literals with bare `os.getenv(...)` calls. +- Replacing hardcoded model literals with `os.getenv(...)` calls — bare + when a `load_dotenv()` bootstrap is in place, otherwise retaining the + original literal as the fallback (see step 4 above). Note that scanning `os.environ.setdefault(...)` and lifting its value into `.env.example` (v2) does NOT violate Rule 2 — the skill READS from diff --git a/.agents/skills/extract-python-environment-variables/scripts/extract_env_vars.py b/.agents/skills/extract-python-environment-variables/scripts/extract_env_vars.py index ce9f0346da..3dc4451aec 100644 --- a/.agents/skills/extract-python-environment-variables/scripts/extract_env_vars.py +++ b/.agents/skills/extract-python-environment-variables/scripts/extract_env_vars.py @@ -1099,31 +1099,62 @@ def write_model_vars_to_env_example( # --------------------------------------------------------------------------- +# Opening quotes of a module docstring, allowing any legal string prefix. +# Python permits r/u/b (and Rb/bR/... pairs) before the quote; a docstring in +# practice is r"""...""" or plain, but the scan must not be fooled by any of +# them. Historical bug: this was a bare startswith('"""') test, so a file whose +# docstring was written as r\"\"\"...\"\"\" (common when the text contains +# backslashes) had `import os` injected ABOVE the docstring, demoting it to a +# dead string expression and losing __doc__. +_DOCSTRING_OPEN_RE = re.compile(r'^[rRuUbB]{0,2}("""|\'\'\'|"|\')') + + def _post_header_index(lines: list[str]) -> int: """ Return the line index after which new top-level code should be inserted. Skips (in order): 1. Leading license / comment block and blank lines. - 2. An optional module-level docstring (single- or triple-quoted). + 2. An optional module-level docstring, whatever its quote style or + string prefix. This prevents imports from being injected before the module docstring, which would cause documentation tools to miss it. + + The docstring boundary is resolved with the AST wherever the source + parses, since that is the only way to be right about every quote style, + prefix, and implicit string concatenation. The textual scan below is a + fallback for sources that do not parse (the caller may be mid-edit). """ - i = 0 n = len(lines) - # Skip license header (comment lines and blank lines) + # AST path — authoritative when the source is syntactically valid. + try: + tree = ast.parse("".join(lines)) + except (SyntaxError, ValueError): + tree = None + if tree is not None and tree.body: + first = tree.body[0] + if ( + isinstance(first, ast.Expr) + and isinstance(first.value, ast.Constant) + and isinstance(first.value.value, str) + ): + # end_lineno is the 1-based last line of the docstring, which is + # exactly the 0-based index of the line after it. + return min(first.value.end_lineno or first.value.lineno, n) + + # Fallback: skip the license header (comment and blank lines)... + i = 0 while i < n and (lines[i].strip().startswith("#") or not lines[i].strip()): i += 1 - # Skip module docstring if present + # ...then an optional module docstring. if i < n: - stripped = lines[i].strip() - for quote in ('"""', "'''"): - if not stripped.startswith(quote): - continue - rest = stripped[len(quote) :] + match = _DOCSTRING_OPEN_RE.match(lines[i].strip()) + if match: + quote = match.group(1) + rest = lines[i].strip()[match.end() :] if rest.endswith(quote) and len(rest) >= len(quote): i += 1 # single-line docstring else: @@ -1131,9 +1162,8 @@ def _post_header_index(lines: list[str]) -> int: while i < n and quote not in lines[i]: i += 1 i += 1 # include the line that contains the closing quotes - break - return i + return min(i, n) def _docstring_node_ids(tree: ast.AST) -> set[int]: @@ -1229,6 +1259,72 @@ def _mark_default(call: ast.Call) -> None: return ids +def _structural_exclusion_node_ids(tree: ast.AST) -> set[int]: + """ + Return the id() of every ast.Constant that must NOT be rewritten because + of WHERE it sits, regardless of what it looks like. + + A model string is only a candidate for promotion to an env var when it is + a *configurable constant*. Three positions mean it is something else, and + rewriting them corrupts working code: + + 1. **An element of a collection literal** (dict key or value, list, set, + or tuple entry). These are lookup tables and enumerations of supported + models, not a single configurable choice:: + + IMAGE_MODELS = { + "flash": "gemini-2.5-flash-image", + "pro": "gemini-2.5-pro-image", + } + + Rewriting the values makes the table resolve at import time from a + single env var; rewriting the KEYS is worse still, because + ``IMAGE_MODELS.get("flash")`` then never matches anything. A dict key + must stay a static literal. + + 2. **A subscript index** — ``IMAGE_MODELS["gemini-3.1"]`` is a key INTO + such a table. Replacing it looks up a different (probably absent) + entry. + + 3. **An operand of a comparison** — ``if "gemini-3.1" in model_id`` tests + a value rather than configuring one. Replacing it changes the + predicate's meaning. + + Skipping a legitimate extraction is cheap (the user can lift the value by + hand); silently breaking a lookup table is not. When in doubt this errs + towards skipping, and the skipped literals are reported so nothing is + hidden — see :func:`extract_skipped_model_literals`. + """ + ids: set[int] = set() + for parent in ast.walk(tree): + if isinstance(parent, ast.Dict): + # `keys` holds None for a `**expansion` entry — guard for it. + for node in list(parent.keys) + list(parent.values): + if isinstance(node, ast.Constant): + ids.add(id(node)) + elif isinstance(parent, (ast.List, ast.Set, ast.Tuple)): + for node in parent.elts: + if isinstance(node, ast.Constant): + ids.add(id(node)) + elif isinstance(parent, ast.Subscript): + if isinstance(parent.slice, ast.Constant): + ids.add(id(parent.slice)) + elif isinstance(parent, ast.Compare): + for node in [parent.left, *parent.comparators]: + if isinstance(node, ast.Constant): + ids.add(id(node)) + return ids + + +def _model_exclusion_node_ids(tree: ast.AST) -> set[int]: + """Every Constant the model-replacement path must leave alone.""" + return ( + _docstring_node_ids(tree) + | _getenv_default_node_ids(tree) + | _structural_exclusion_node_ids(tree) + ) + + def _flat_offset(lines: list[str], lineno: int, col: int) -> int: """Convert a 1-based lineno + 0-based col_offset to a flat char offset.""" return sum(len(ln) for ln in lines[: lineno - 1]) + col @@ -1254,7 +1350,7 @@ def _imports_os(tree: ast.AST) -> bool: _NOQA_E402_SUFFIX = " # noqa: E402 -- must come after load_dotenv()" -def _suppress_e402_on_late_relative_imports( # noqa: C901 +def _suppress_e402_on_late_relative_imports( tree: ast.Module | None, lines: list[str] ) -> tuple[list[str], int]: """Append `# noqa: E402` to top-level `from .x import y` statements that @@ -1579,6 +1675,74 @@ def _find_dependencies_close_bracket(content: str) -> int | None: return _scan_matching_close_bracket(content, deps_start.end() - 1) +def _split_trailing_comment(line: str) -> tuple[str, str]: + """ + Split a single TOML line into ``(code, comment)``. + + A ``#`` only opens a comment when it sits outside a string, so the scan + tracks basic strings (``"..."``, backslash escapes honoured) and literal + strings (``'...'``, no escapes). ``comment`` starts at the ``#`` and is + ``""`` when the line has none; any whitespace between the code and the + ``#`` stays on the ``code`` side so callers can preserve the layout. + """ + quote: str | None = None + escaped = False + for idx, ch in enumerate(line): + if quote is not None: + if escaped: + escaped = False + elif quote == '"' and ch == "\\": + escaped = True + elif ch == quote: + quote = None + continue + if ch in "\"'": + quote = ch + elif ch == "#": + return line[:idx], line[idx:] + return line, "" + + +def _array_body_has_entries(array_body: str) -> bool: + """Whether an array body holds a real entry (not just blanks/comments).""" + code_only = "".join( + _split_trailing_comment(ln)[0] for ln in array_body.split("\n") + ) + return bool(code_only.strip()) + + +def _ensure_trailing_comma(trimmed: str) -> str: + """ + Append a ``,`` to the last array entry in ``trimmed`` if it lacks one. + + The comma must land on the ENTRY, never inside a trailing comment. + Historical bug: a plain ``trimmed.endswith(",")`` test inspected the end + of the *comment* text, so an array ending in:: + + "pyOpenSSL>=23.0", # mTLS during auth in some envs + + grew a stray comma inside the comment, and one ending in:: + + "pyOpenSSL>=23.0" # no comma + + produced invalid TOML — the separating comma was commented out, which + the round-trip guard in :func:`ensure_python_dotenv_dependency` then + rejected, silently skipping the dependency insertion altogether. + """ + lines = trimmed.split("\n") + for i in range(len(lines) - 1, -1, -1): + code, comment = _split_trailing_comment(lines[i]) + if not code.strip(): + continue # blank or comment-only line — keep looking backwards + stripped = code.rstrip() + if stripped.endswith(",") or stripped.endswith("["): + return trimmed # already terminated, or the array is empty + gap = code[len(stripped) :] + lines[i] = stripped + "," + gap + comment + return "\n".join(lines) + return trimmed + + def _insert_before_close(content: str, close_idx: int) -> str: """ Insert a `"python-dotenv>=1.0.0",` line into the dependencies array @@ -1596,12 +1760,12 @@ def _insert_before_close(content: str, close_idx: int) -> str: # the layout of the insertion. trimmed = prefix.rstrip() - # If the array has any content, make sure the last entry has a trailing - # comma before we append our own. + # If the array holds any real entry, make sure the last one has a + # trailing comma before we append our own. array_open = trimmed.rfind("[") array_body = trimmed[array_open + 1 :] if array_open >= 0 else "" - if array_body.strip() and not trimmed.endswith(","): - trimmed += "," + if _array_body_has_entries(array_body): + trimmed = _ensure_trailing_comma(trimmed) return trimmed + '\n "python-dotenv>=1.0.0",\n' + suffix @@ -1699,6 +1863,9 @@ def extract_hardcoded_models( :func:`_getenv_default_node_ids`) — those are already serving as env-var defaults; replacing them would silently regress the type from ``str`` to ``str | None`` and could break at runtime. + * String literals whose POSITION means they aren't a configurable + constant — collection-literal entries, subscript indices, and + comparison operands (see :func:`_structural_exclusion_node_ids`). Returns: {file_path: [(line_number, model_string), ...]} @@ -1712,9 +1879,7 @@ def extract_hardcoded_models( except (SyntaxError, UnicodeDecodeError): continue - excluded_ids = _docstring_node_ids(tree) | _getenv_default_node_ids( - tree - ) + excluded_ids = _model_exclusion_node_ids(tree) for node in ast.walk(tree): if not isinstance(node, ast.Constant): @@ -1729,16 +1894,175 @@ def extract_hardcoded_models( return hits +def extract_skipped_model_literals( + py_files: list[Path], +) -> dict[Path, list[tuple[int, str]]]: + """ + Find model strings deliberately NOT rewritten because of their position. + + These are the literals :func:`_structural_exclusion_node_ids` protects — + lookup-table entries, subscript keys, comparison operands. They are + reported rather than silently dropped so the maintainer knows the recipe + still hardcodes a model somewhere, and can decide whether that table + ought to be configurable. + + Docstring mentions are excluded (prose, not code). Identical + (line, string) pairs are de-duplicated: ``{"gemini-x": "gemini-x"}`` + holds two distinct nodes on one line, and listing it twice only makes + the report harder to read. + + Returns: + {file_path: [(line_number, model_string), ...]} + """ + skipped: dict[Path, list[tuple[int, str]]] = {} + + for py_file in py_files: + try: + source = py_file.read_text(encoding="utf-8") + tree = ast.parse(source, filename=str(py_file)) + except (SyntaxError, UnicodeDecodeError): + continue + + structural = _structural_exclusion_node_ids(tree) + docstrings = _docstring_node_ids(tree) + seen: set[tuple[int, str]] = set() + + for node in ast.walk(tree): + if not isinstance(node, ast.Constant): + continue + if id(node) not in structural or id(node) in docstrings: + continue + if not isinstance(node.value, str): + continue + if not any(node.value.startswith(p) for p in MODEL_PREFIXES): + continue + key = (node.lineno, node.value) + if key in seen: + continue + seen.add(key) + skipped.setdefault(py_file, []).append(key) + + if skipped: + for hits in skipped.values(): + hits.sort() + + return skipped + + +def _assignment_target_name(node: ast.AST) -> str | None: + """Return the single ``Name`` target of an assignment, else None.""" + if ( + isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + ): + return node.targets[0].id + if ( + isinstance(node, ast.AnnAssign) + and node.value is not None + and isinstance(node.target, ast.Name) + ): + return node.target.id + return None + + +def _var_name_from_target(target: str) -> str | None: + """ + Derive an env-var name from the assignment target holding a model string. + + ``DEFAULT_EMBEDDING_MODEL = "gemini-embedding-001"`` tells us far more + about the variable's role than the generic ``MODEL_NAME`` fallback does — + especially in a recipe that already reads a *different* model var (an LLM) + from the environment, where a second bare ``MODEL_NAME`` is actively + misleading. + + Only targets that name a model are used; anything else returns None so the + caller falls back to the generic scheme. The leading ``DEFAULT_`` is + dropped because it describes the constant, not the model. + """ + name = target.strip().lstrip("_").upper() + if name.startswith("DEFAULT_"): + name = name[len("DEFAULT_") :] + if not name.isidentifier(): + return None + if "MODEL" not in name: + return None + if name == "MODEL": + return "MODEL_NAME" + return name + + +def extract_model_var_hints(py_files: list[Path]) -> dict[str, str]: + """ + Suggest an env-var name for each hardcoded model string, from the + assignment target that holds it. + + Both ``DEFAULT_EMBEDDING_MODEL = "gemini-embedding-001"`` and + ``embedding_model = cfg.get("embedding_model", "gemini-embedding-001")`` + yield the hint ``EMBEDDING_MODEL``. The same exclusions as + :func:`extract_hardcoded_models` apply, so a model name mentioned in a + docstring or already serving as a getenv default never votes. + + When a model string is assigned to differently-named targets across the + recipe, the most frequent target wins, ties broken alphabetically, so the + result is deterministic. + + Returns: + {model_string: suggested_var_name} — only for strings with a usable hint. + """ + votes: dict[str, dict[str, int]] = {} + + for py_file in py_files: + try: + source = py_file.read_text(encoding="utf-8") + tree = ast.parse(source, filename=str(py_file)) + except (SyntaxError, UnicodeDecodeError): + continue + + excluded_ids = _model_exclusion_node_ids(tree) + + for node in ast.walk(tree): + target = _assignment_target_name(node) + if target is None: + continue + derived = _var_name_from_target(target) + if derived is None: + continue + for child in ast.walk(node.value): + if not isinstance(child, ast.Constant): + continue + if id(child) in excluded_ids: + continue + if not isinstance(child.value, str): + continue + if any( + child.value.startswith(prefix) for prefix in MODEL_PREFIXES + ): + tally = votes.setdefault(child.value, {}) + tally[derived] = tally.get(derived, 0) + 1 + + return { + model_str: sorted(tally.items(), key=lambda kv: (-kv[1], kv[0]))[0][0] + for model_str, tally in votes.items() + } + + def assign_model_var_names( model_strings: set[str], existing_vars: set[str] | None = None, + hints: dict[str, str] | None = None, ) -> dict[str, str]: """ - Assign a standardised MODEL_NAME_* env var name to each unique model string. + Assign a standardised env var name to each unique model string. Rules (applied to the sorted list for determinism): - - If there is only one model → MODEL_NAME (no suffix), unless MODEL_NAME - is already taken, in which case the counter scheme below is used. + - If ``hints`` supplies a name derived from the assignment target (see + :func:`extract_model_var_hints`) and that name is free, use it. A + recipe that writes ``DEFAULT_EMBEDDING_MODEL`` gets ``EMBEDDING_MODEL`` + rather than a generic ``MODEL_NAME`` that collides conceptually with + an LLM model var it may already read. + - Otherwise, if exactly one model is left unnamed → MODEL_NAME (no + suffix), unless MODEL_NAME is already taken. - Otherwise → MODEL_NAME_GENERATED_1, MODEL_NAME_GENERATED_2, … skipping any index whose name is already present in existing_vars (e.g. from a prior run or a manually added entry in .env.example). @@ -1753,21 +2077,39 @@ def assign_model_var_names( model_strings: the set of unique hardcoded model strings found in source. existing_vars: names already declared in .env.example (or anywhere else that should be treated as taken). Defaults to empty set. + hints: optional {model_string: suggested_name} from the assignment + targets in source. Returns: {model_string: env_var_name} """ taken = set(existing_vars) if existing_vars else set() + hints = hints or {} sorted_strings = sorted(model_strings) - if len(sorted_strings) == 1: - if "MODEL_NAME" not in taken: - return {sorted_strings[0]: "MODEL_NAME"} - # Fall through to the counter scheme if MODEL_NAME is already taken. - mapping: dict[str, str] = {} - counter = 1 + unresolved: list[str] = [] + + # Pass 1 — honour a hint derived from the assignment target. for model_str in sorted_strings: + hint = hints.get(model_str) + if hint and hint not in taken: + mapping[model_str] = hint + taken.add(hint) + else: + unresolved.append(model_str) + + if not unresolved: + return mapping + + # Pass 2 — a lone unnamed model gets the unsuffixed MODEL_NAME. + if len(unresolved) == 1 and "MODEL_NAME" not in taken: + mapping[unresolved[0]] = "MODEL_NAME" + return mapping + + # Pass 3 — counter scheme for everything still unnamed. + counter = 1 + for model_str in unresolved: while f"MODEL_NAME_GENERATED_{counter}" in taken: counter += 1 var_name = f"MODEL_NAME_GENERATED_{counter}" @@ -1778,11 +2120,23 @@ def assign_model_var_names( return mapping +def _py_string_literal(value: str) -> str: + """Render ``value`` as a one-line, double-quoted Python string literal.""" + body = ( + value.replace("\\", "\\\\") + .replace('"', '\\"') + .replace("\n", "\\n") + .replace("\r", "\\r") + ) + return f'"{body}"' + + def _model_replacement( node: ast.AST, excluded_ids: set[int], name_map: dict[str, str], lines: list[str], + keep_defaults: bool = False, ) -> tuple[int, int, str, str, str] | None: """ Return (start, end, new_text, model_str, var_name) if node is a @@ -1792,6 +2146,22 @@ def _model_replacement( are already serving as ``os.getenv``/``environ.get``/``setdefault`` default arguments. The latter must NOT be replaced — see :func:`_getenv_default_node_ids` for the correctness rationale. + + ``keep_defaults`` controls the shape of the emitted call: + + * False (the default) → BARE ``os.getenv("VAR")``. This is the skill's + normal rule: we do NOT write inferred defaults into Python source. + Default values are the maintainer's decision; the skill's job is to + lift the constant out of the source, not to also decide what fallback + the maintainer wants. It is only SAFE because a ``load_dotenv()`` + bootstrap guarantees ``.env`` has been read by the time the call runs. + + * True → ``os.getenv("VAR", "")``. Used when no + bootstrap could be installed (see :func:`run_step_load_dotenv`), so + nothing loads ``.env`` and a bare lookup would evaluate to ``None`` + at runtime — silently breaking the recipe. Preserving the literal as + the fallback keeps behaviour identical to before the rewrite while + still lifting the value into the environment. """ if not isinstance(node, ast.Constant): return None @@ -1804,18 +2174,18 @@ def _model_replacement( return None start = _flat_offset(lines, node.lineno, node.col_offset) end = _flat_offset(lines, node.end_lineno, node.end_col_offset) - # Emit BARE `os.getenv("VAR")` — no default argument. This is a hard - # rule of the skill: we do NOT write inferred defaults into Python - # source, even though `os.getenv("MODEL_NAME", node.value)` would be - # trivially "safer" (the recipe would keep working after the rewrite - # even without .env set up). Default values are the maintainer's - # decision; the skill's job is to lift the constant out of the source, - # not to also decide what fallback the maintainer wants. - return start, end, f'os.getenv("{var_name}")', node.value, var_name + if keep_defaults: + new_text = f'os.getenv("{var_name}", {_py_string_literal(node.value)})' + else: + new_text = f'os.getenv("{var_name}")' + return start, end, new_text, node.value, var_name def _collect_model_replacements( - tree: ast.AST, source: str, name_map: dict[str, str] + tree: ast.AST, + source: str, + name_map: dict[str, str], + keep_defaults: bool = False, ) -> tuple[list[tuple[int, int, str]], dict[str, str]]: """Walk ``tree`` and collect model-string replacement plans. @@ -1833,12 +2203,14 @@ def _collect_model_replacements( type of a call like ``os.getenv("V", "gemini-3.5-flash")`` from ``str`` to ``str | None``. """ - excluded_ids = _docstring_node_ids(tree) | _getenv_default_node_ids(tree) + excluded_ids = _model_exclusion_node_ids(tree) lines = source.splitlines(keepends=True) replacements: list[tuple[int, int, str]] = [] file_substituted: dict[str, str] = {} for node in ast.walk(tree): - replacement = _model_replacement(node, excluded_ids, name_map, lines) + replacement = _model_replacement( + node, excluded_ids, name_map, lines, keep_defaults=keep_defaults + ) if replacement is None: continue start, end, new_text, model_str, var_name = replacement @@ -1883,12 +2255,16 @@ def replace_hardcoded_models( hits: dict[Path, list[tuple[int, str]]], name_map: dict[str, str], dry_run: bool = False, + keep_defaults: bool = False, ) -> dict[str, str]: """ Replace each hardcoded model string with the correct - os.getenv("MODEL_NAME_*") call in-place, using the mapping produced by + os.getenv(...) call in-place, using the mapping produced by assign_model_var_names(). + ``keep_defaults`` preserves the original literal as the ``os.getenv`` + fallback — see :func:`_model_replacement` for when and why. + Replacement is AST-position-based, which means: - All quote styles (single, double, triple, raw) are handled correctly because the AST abstracts away quoting entirely. @@ -1912,7 +2288,7 @@ def replace_hardcoded_models( continue replacements, file_substituted = _collect_model_replacements( - tree, source, name_map + tree, source, name_map, keep_defaults=keep_defaults ) if not replacements: continue @@ -2040,18 +2416,24 @@ def run_step_env_vars( return env_example -def run_step_load_dotenv(recipe_dir: Path, dry_run: bool = False) -> None: +def run_step_load_dotenv(recipe_dir: Path, dry_run: bool = False) -> bool: """Step 4: inject load_dotenv() bootstrap into the package __init__.py, and suppress Ruff E402 on any trailing relative imports that come after non-import statements (whether we just injected them or the author had - already written a bootstrap by hand).""" + already written a bootstrap by hand). + + Returns whether a ``load_dotenv()`` bootstrap is in place afterwards. + Step 6 needs this: without a bootstrap nothing reads ``.env``, so the + bare ``os.getenv("VAR")`` calls it would otherwise emit evaluate to + ``None`` at runtime. See :func:`_model_replacement`. + """ init_py = find_package_init(recipe_dir) if not init_py: print( "[WARN] No Python package (subdirectory with __init__.py) found. " "load_dotenv() injection skipped." ) - return + return False rel = init_py.relative_to(recipe_dir) injected, noqa_added = inject_load_dotenv(init_py, dry_run=dry_run) if injected: @@ -2068,6 +2450,7 @@ def run_step_load_dotenv(recipe_dir: Path, dry_run: bool = False) -> None: "E402 in Phase 4 (ordering is intentional: env must be " "populated before importing agent submodules)." ) + return True def run_step_pyproject(recipe_dir: Path, dry_run: bool = False) -> None: @@ -2105,9 +2488,33 @@ def run_step_model_names( py_files: list[Path], env_example: Path, dry_run: bool = False, + dotenv_active: bool = True, ) -> None: - """Step 6: detect hardcoded model strings, replace with os.getenv().""" + """Step 6: detect hardcoded model strings, replace with os.getenv(). + + ``dotenv_active`` comes from :func:`run_step_load_dotenv`. When it is + False nothing in the recipe reads ``.env``, so the original literal is + preserved as the ``os.getenv`` fallback rather than emitting a bare + lookup that would evaluate to ``None``. + """ model_hits = extract_hardcoded_models(py_files) + skipped = extract_skipped_model_literals(py_files) + + if skipped: + total = sum(len(v) for v in skipped.values()) + print( + f"\n[INFO] Left {total} model literal(s) in place — their position " + "means they are not a configurable constant (lookup-table entry, " + "subscript key, or comparison operand). Rewriting them would " + "break the code:" + ) + for py_file, file_hits in skipped.items(): + for lineno, model_str in file_hits: + print( + f" {py_file.relative_to(recipe_dir)}:{lineno}" + f' — "{model_str}"' + ) + if not model_hits: print("\n[PASS] No hardcoded model names detected.") return @@ -2118,7 +2525,8 @@ def run_step_model_names( for _lineno, model_str in file_hits } existing_vars = read_defined_vars(env_example) - name_map = assign_model_var_names(all_model_strings, existing_vars) + hints = extract_model_var_hints(py_files) + name_map = assign_model_var_names(all_model_strings, existing_vars, hints) print("\n[INFO] Detected hardcoded model name(s):") for py_file, file_hits in model_hits.items(): @@ -2128,8 +2536,21 @@ def run_step_model_names( f' — "{model_str}" → {name_map[model_str]}' ) + keep_defaults = not dotenv_active + if keep_defaults: + print( + "[INFO] No load_dotenv() bootstrap is in place, so nothing would " + "read .env at runtime. Keeping the original literal as the " + "os.getenv() fallback instead of emitting a bare lookup that " + "would evaluate to None." + ) + substituted = replace_hardcoded_models( - py_files, model_hits, name_map, dry_run=dry_run + py_files, + model_hits, + name_map, + dry_run=dry_run, + keep_defaults=keep_defaults, ) if not substituted: return @@ -2145,9 +2566,14 @@ def run_step_model_names( replace_verb = "Would replace" if dry_run else "Replaced" for model_str, var_name in substituted.items(): + call = ( + f'os.getenv("{var_name}", "{model_str}")' + if keep_defaults + else f'os.getenv("{var_name}")' + ) print( f'[{_tag(dry_run)}] {replace_verb} hardcoded "{model_str}" with' - f' os.getenv("{var_name}") in source.' + f" {call} in source." ) if added_models: add_verb = "Would add" if dry_run else "Added" @@ -2206,9 +2632,15 @@ def main() -> None: print(f" {f.relative_to(recipe_dir)}") env_example = run_step_env_vars(recipe_dir, py_files, dry_run=dry_run) - run_step_load_dotenv(recipe_dir, dry_run=dry_run) + dotenv_active = run_step_load_dotenv(recipe_dir, dry_run=dry_run) run_step_pyproject(recipe_dir, dry_run=dry_run) - run_step_model_names(recipe_dir, py_files, env_example, dry_run=dry_run) + run_step_model_names( + recipe_dir, + py_files, + env_example, + dry_run=dry_run, + dotenv_active=dotenv_active, + ) print(f"\n{'=' * 50}") if dry_run: diff --git a/.agents/skills/extract-python-environment-variables/tests/test_extract_env_vars.py b/.agents/skills/extract-python-environment-variables/tests/test_extract_env_vars.py index 19c1b2908e..55abc5cd26 100644 --- a/.agents/skills/extract-python-environment-variables/tests/test_extract_env_vars.py +++ b/.agents/skills/extract-python-environment-variables/tests/test_extract_env_vars.py @@ -2798,3 +2798,439 @@ def test_invariant_appended_entries_are_skill_owned(tmp_path): assert ( entries["DOWNGRADED"].classification == m.EntryClassification.SKILL_TODO ) + + +# --------------------------------------------------------------------------- +# _post_header_index — module-docstring detection +# +# Regression: the docstring scan was a bare startswith('"""') test, so any +# legal string prefix (r"""..., u'''...) slipped past it and `import os` was +# injected ABOVE the docstring. That demotes the docstring to a dead string +# expression and loses __doc__. +# --------------------------------------------------------------------------- + + +def _insert_point(source: str) -> str: + """Return the line `import os` would be inserted before.""" + lines = source.splitlines(keepends=True) + idx = m._post_header_index(lines) + return lines[idx] if idx < len(lines) else "" + + +def test_post_header_index_skips_plain_docstring(): + src = '# license\n\n"""Doc.\n\nMore.\n"""\n\nimport sys\n' + assert _insert_point(src) == "\n" + + +def test_post_header_index_skips_raw_prefixed_docstring(): + src = '# license\n\nr"""Doc.\n\nMore.\n"""\n\nimport sys\n' + assert _insert_point(src) == "\n" + + +def test_post_header_index_skips_unicode_prefixed_docstring(): + src = '# license\n\nu"""Doc.\n"""\n\nimport sys\n' + assert _insert_point(src) == "\n" + + +def test_post_header_index_skips_raw_single_quoted_docstring(): + src = "# license\n\nr'''Doc.\nMore.\n'''\n\nimport sys\n" + assert _insert_point(src) == "\n" + + +def test_post_header_index_skips_one_line_raw_docstring(): + src = '# license\n\nr"""Doc."""\n\nimport sys\n' + assert _insert_point(src) == "\n" + + +def test_post_header_index_handles_shebang_and_raw_docstring(): + src = '#!/usr/bin/env python3\n# license\n\nr"""Doc.\n"""\n\nimport sys\n' + assert _insert_point(src) == "\n" + + +def test_post_header_index_without_docstring_lands_after_comments(): + src = "# license\n\nimport sys\n" + assert _insert_point(src) == "import sys\n" + + +def test_post_header_index_unparseable_source_falls_back_to_scan(): + # The AST path is unavailable, but the textual fallback must still + # respect a prefixed docstring. + src = '# license\n\nr"""Doc.\n"""\n\nimport sys\ndef (\n' + assert _insert_point(src) == "\n" + + +def test_post_header_index_empty_file(): + assert m._post_header_index([]) == 0 + + +def test_os_import_injected_below_raw_docstring(tmp_path): + # End-to-end: the module docstring must remain the first statement, so + # ast.get_docstring() still finds it after the rewrite. + src = ( + "# Copyright\n" + "\n" + 'r"""Module docstring with a \\d regex mention."""\n' + "\n" + 'DEFAULT_MODEL = "gemini-embedding-001"\n' + ) + py = _write(tmp_path / "ingest.py", src) + + hits = m.extract_hardcoded_models([py]) + name_map = m.assign_model_var_names({"gemini-embedding-001"}) + m.replace_hardcoded_models([py], hits, name_map) + + result = py.read_text(encoding="utf-8") + tree = ast.parse(result) + assert ast.get_docstring(tree) is not None, ( + f"docstring lost — import injected above it:\n{result}" + ) + assert "import os" in result + + +# --------------------------------------------------------------------------- +# _insert_before_close — trailing comma must land on the entry, not a comment +# --------------------------------------------------------------------------- + + +def _deps_after_dotenv(body: str): + """Add python-dotenv to a dependencies array and parse the result.""" + src = '[project]\nname = "x"\n' + body + "\n" + out = m._compute_pyproject_with_dotenv(src) + assert out is not None + return out, m.tomllib.loads(out)["project"]["dependencies"] + + +def test_insert_dotenv_preserves_comment_when_entry_has_comma(): + out, deps = _deps_after_dotenv( + 'dependencies = [\n "requests>=2.28",\n' + ' "pyOpenSSL>=23.0", # mTLS during auth\n]' + ) + assert deps == ["requests>=2.28", "pyOpenSSL>=23.0", "python-dotenv>=1.0.0"] + # The comment must be untouched — no comma appended inside it. + assert "# mTLS during auth\n" in out + assert "# mTLS during auth," not in out + + +def test_insert_dotenv_adds_comma_before_trailing_comment(): + # Previously produced invalid TOML: the separating comma was written + # inside the comment, so the array never closed. + out, deps = _deps_after_dotenv( + 'dependencies = [\n "requests>=2.28",\n' + ' "pyOpenSSL>=23.0" # no comma here\n]' + ) + assert deps == ["requests>=2.28", "pyOpenSSL>=23.0", "python-dotenv>=1.0.0"] + assert '"pyOpenSSL>=23.0", # no comma here' in out + + +def test_insert_dotenv_with_comment_only_last_line(): + _out, deps = _deps_after_dotenv( + 'dependencies = [\n "requests>=2.28",\n # a note\n]' + ) + assert deps == ["requests>=2.28", "python-dotenv>=1.0.0"] + + +def test_insert_dotenv_into_empty_array(): + _out, deps = _deps_after_dotenv("dependencies = [\n]") + assert deps == ["python-dotenv>=1.0.0"] + + +def test_insert_dotenv_into_empty_array_with_comment(): + _out, deps = _deps_after_dotenv("dependencies = [\n # nothing yet\n]") + assert deps == ["python-dotenv>=1.0.0"] + + +def test_insert_dotenv_into_single_line_array(): + _out, deps = _deps_after_dotenv('dependencies = ["a", "b"]') + assert deps == ["a", "b", "python-dotenv>=1.0.0"] + + +def test_insert_dotenv_ignores_hash_inside_string(): + # A '#' inside a PEP 508 URL is not a comment marker. + _out, deps = _deps_after_dotenv( + 'dependencies = [\n "pkg @ https://x/y#egg=pkg"\n]' + ) + assert deps == ["pkg @ https://x/y#egg=pkg", "python-dotenv>=1.0.0"] + + +def test_split_trailing_comment_respects_quotes(): + assert m._split_trailing_comment('"a#b" # real') == ( + '"a#b" ', + "# real", + ) + assert m._split_trailing_comment("'a#b'") == ("'a#b'", "") + assert m._split_trailing_comment('"esc\\"#q" # c') == ( + '"esc\\"#q" ', + "# c", + ) + + +# --------------------------------------------------------------------------- +# Model rewrite: bare getenv only when a load_dotenv bootstrap exists +# --------------------------------------------------------------------------- + + +def test_model_rewrite_keeps_default_when_no_dotenv_bootstrap(tmp_path): + py = _write( + tmp_path / "ingest.py", + 'DEFAULT_MODEL = "gemini-embedding-001"\n', + ) + hits = m.extract_hardcoded_models([py]) + name_map = {"gemini-embedding-001": "EMBEDDING_MODEL"} + + m.replace_hardcoded_models([py], hits, name_map, keep_defaults=True) + + result = py.read_text(encoding="utf-8") + assert 'os.getenv("EMBEDDING_MODEL", "gemini-embedding-001")' in result + # Still valid Python, and the value can never be None. + ast.parse(result) + + +def test_model_rewrite_bare_getenv_by_default(tmp_path): + py = _write( + tmp_path / "agent.py", + 'MODEL = "gemini-3.5-flash"\n', + ) + hits = m.extract_hardcoded_models([py]) + name_map = {"gemini-3.5-flash": "MODEL_NAME"} + + m.replace_hardcoded_models([py], hits, name_map) + + result = py.read_text(encoding="utf-8") + assert 'os.getenv("MODEL_NAME")' in result + assert "gemini-3.5-flash" not in result + + +def test_run_step_load_dotenv_returns_false_without_package(tmp_path, capsys): + assert m.run_step_load_dotenv(tmp_path) is False + assert "No Python package" in capsys.readouterr().out + + +def test_run_step_load_dotenv_returns_true_with_package(tmp_path): + _write(tmp_path / "app" / "__init__.py", "") + _write(tmp_path / "app" / "agent.py", "") + assert m.run_step_load_dotenv(tmp_path) is True + + +def test_py_string_literal_escapes(): + assert m._py_string_literal("plain") == '"plain"' + assert m._py_string_literal('has"quote') == '"has\\"quote"' + assert m._py_string_literal("back\\slash") == '"back\\\\slash"' + assert m._py_string_literal("nl\n") == '"nl\\n"' + + +# --------------------------------------------------------------------------- +# Model var naming derived from the assignment target +# --------------------------------------------------------------------------- + + +def test_model_var_hint_from_default_prefixed_constant(tmp_path): + py = _write( + tmp_path / "ingest.py", + 'DEFAULT_EMBEDDING_MODEL = "gemini-embedding-001"\n', + ) + assert m.extract_model_var_hints([py]) == { + "gemini-embedding-001": "EMBEDDING_MODEL" + } + + +def test_model_var_hint_from_nested_call_value(tmp_path): + py = _write( + tmp_path / "setup.py", + 'embedding_model = cfg.get("embedding_model", "gemini-embedding-001")\n', + ) + assert m.extract_model_var_hints([py]) == { + "gemini-embedding-001": "EMBEDDING_MODEL" + } + + +def test_model_var_hint_ignores_target_without_model_in_name(tmp_path): + py = _write(tmp_path / "agent.py", 'LLM = "gemini-3.5-flash"\n') + assert m.extract_model_var_hints([py]) == {} + + +def test_model_var_hint_bare_model_target_maps_to_model_name(tmp_path): + py = _write(tmp_path / "agent.py", 'MODEL = "gemini-3.5-flash"\n') + assert m.extract_model_var_hints([py]) == {"gemini-3.5-flash": "MODEL_NAME"} + + +def test_model_var_hint_ignores_docstring_mention(tmp_path): + py = _write( + tmp_path / "agent.py", + '"""Uses gemini-3.5-flash by default."""\n', + ) + assert m.extract_model_var_hints([py]) == {} + + +def test_model_var_hint_deterministic_across_conflicting_targets(tmp_path): + a = _write(tmp_path / "a.py", 'EMBEDDING_MODEL = "gemini-embedding-001"\n') + b = _write(tmp_path / "b.py", 'VECTOR_MODEL = "gemini-embedding-001"\n') + c = _write(tmp_path / "c.py", 'VECTOR_MODEL = "gemini-embedding-001"\n') + # VECTOR_MODEL has two votes to EMBEDDING_MODEL's one. + assert m.extract_model_var_hints([a, b, c]) == { + "gemini-embedding-001": "VECTOR_MODEL" + } + + +def test_assign_model_var_names_uses_hint(): + mapping = m.assign_model_var_names( + {"gemini-embedding-001"}, + existing_vars={"GEMINI_MODEL"}, + hints={"gemini-embedding-001": "EMBEDDING_MODEL"}, + ) + assert mapping == {"gemini-embedding-001": "EMBEDDING_MODEL"} + + +def test_assign_model_var_names_falls_back_when_hint_taken(): + mapping = m.assign_model_var_names( + {"gemini-embedding-001"}, + existing_vars={"EMBEDDING_MODEL"}, + hints={"gemini-embedding-001": "EMBEDDING_MODEL"}, + ) + assert mapping == {"gemini-embedding-001": "MODEL_NAME"} + + +def test_assign_model_var_names_mixed_hinted_and_unhinted(): + mapping = m.assign_model_var_names( + {"gemini-embedding-001", "gemini-3.5-flash"}, + hints={"gemini-embedding-001": "EMBEDDING_MODEL"}, + ) + assert mapping["gemini-embedding-001"] == "EMBEDDING_MODEL" + # The lone remaining model still gets the unsuffixed generic name. + assert mapping["gemini-3.5-flash"] == "MODEL_NAME" + + +def test_assign_model_var_names_unchanged_without_hints(): + # Backwards compatibility with the pre-hint behaviour. + assert m.assign_model_var_names({"gemini-3.5-flash"}) == { + "gemini-3.5-flash": "MODEL_NAME" + } + two = m.assign_model_var_names({"gemini-a", "gemini-b"}) + assert set(two.values()) == { + "MODEL_NAME_GENERATED_1", + "MODEL_NAME_GENERATED_2", + } + + +# --------------------------------------------------------------------------- +# Structural exclusions: position decides whether a model literal is a +# configurable constant. +# +# Regression: the extractor rewrote every gemini-* string it found, including +# the keys AND values of a model lookup table: +# +# IMAGE_MODELS = {"flash": "gemini-2.5-flash-image", ...} +# +# Turning a dict key into os.getenv(...) means IMAGE_MODELS.get("flash") +# never matches again — a silent, total break of the lookup. +# --------------------------------------------------------------------------- + + +LOOKUP_TABLE_SRC = """IMAGE_MODELS = { + "gemini-2.5-flash-image": "gemini-2.5-flash-image", + "flash": "gemini-2.5-flash-image", + "pro": "gemini-2.5-pro-image", +} + + +def resolve(label): + if "gemini-3.1" in label: + return IMAGE_MODELS["gemini-2.5-pro-image"] + return IMAGE_MODELS.get(label, IMAGE_MODELS["gemini-2.5-flash-image"]) +""" + + +def test_dict_literal_entries_are_not_detected(tmp_path): + py = _write(tmp_path / "models.py", LOOKUP_TABLE_SRC) + assert m.extract_hardcoded_models([py]) == {} + + +def test_dict_literal_is_left_byte_identical(tmp_path): + py = _write(tmp_path / "models.py", LOOKUP_TABLE_SRC) + hits = m.extract_hardcoded_models([py]) + name_map = m.assign_model_var_names( + {"gemini-2.5-flash-image", "gemini-2.5-pro-image", "gemini-3.1"} + ) + + m.replace_hardcoded_models([py], hits, name_map) + + assert py.read_text(encoding="utf-8") == LOOKUP_TABLE_SRC + + +def test_list_set_and_tuple_entries_are_excluded(tmp_path): + py = _write( + tmp_path / "c.py", + 'ALLOWED = ["gemini-3.5-flash"]\n' + 'SUPPORTED = {"gemini-3.5-pro"}\n' + 'PAIR = ("gemini-3.1-flash-image",)\n', + ) + assert m.extract_hardcoded_models([py]) == {} + + +def test_subscript_index_is_excluded(tmp_path): + py = _write(tmp_path / "c.py", 'X = TABLE["gemini-3.5-flash"]\n') + assert m.extract_hardcoded_models([py]) == {} + + +def test_comparison_operand_is_excluded(tmp_path): + py = _write( + tmp_path / "c.py", + 'if "gemini-3.1" in model_id:\n pass\n', + ) + assert m.extract_hardcoded_models([py]) == {} + + +def test_scalar_assignment_is_still_detected(tmp_path): + # The exclusions must not swallow the case the skill exists for. + py = _write(tmp_path / "agent.py", 'MODEL = "gemini-3.5-flash"\n') + hits = m.extract_hardcoded_models([py]) + assert [v for hits_ in hits.values() for _, v in hits_] == [ + "gemini-3.5-flash" + ] + + +def test_call_keyword_argument_is_still_detected(tmp_path): + py = _write(tmp_path / "agent.py", 'a = Agent(model="gemini-3.5-flash")\n') + hits = m.extract_hardcoded_models([py]) + assert [v for hits_ in hits.values() for _, v in hits_] == [ + "gemini-3.5-flash" + ] + + +def test_dict_target_does_not_produce_a_var_name_hint(tmp_path): + # `IMAGE_MODELS = {...}` names a TABLE, not a model. Deriving the env var + # name IMAGE_MODELS from it produced a nonsensical .env.example entry. + py = _write(tmp_path / "models.py", LOOKUP_TABLE_SRC) + assert m.extract_model_var_hints([py]) == {} + + +def test_skipped_literals_are_reported(tmp_path): + py = _write(tmp_path / "models.py", LOOKUP_TABLE_SRC) + skipped = m.extract_skipped_model_literals([py]) + + values = [v for _, v in skipped[py]] + assert "gemini-2.5-flash-image" in values # dict key + value + assert "gemini-2.5-pro-image" in values + assert "gemini-3.1" in values # comparison operand + + +def test_skipped_literals_deduplicate_per_line(tmp_path): + # `"x": "x"` is two nodes on one line; report it once. + py = _write( + tmp_path / "d.py", + 'T = {\n "gemini-3.5-flash": "gemini-3.5-flash",\n}\n', + ) + skipped = m.extract_skipped_model_literals([py]) + assert skipped[py] == [(2, "gemini-3.5-flash")] + + +def test_skipped_literals_ignore_docstring_mentions(tmp_path): + py = _write( + tmp_path / "d.py", + '"""Uses gemini-3.5-flash by default."""\n', + ) + assert m.extract_skipped_model_literals([py]) == {} + + +def test_skipped_report_empty_when_nothing_excluded(tmp_path): + py = _write(tmp_path / "agent.py", 'MODEL = "gemini-3.5-flash"\n') + assert m.extract_skipped_model_literals([py]) == {} diff --git a/.agents/skills/generate-python-runnability-test/SKILL.md b/.agents/skills/generate-python-runnability-test/SKILL.md index c7cd88e1f0..094f26d09f 100644 --- a/.agents/skills/generate-python-runnability-test/SKILL.md +++ b/.agents/skills/generate-python-runnability-test/SKILL.md @@ -15,7 +15,7 @@ description: > metadata: author: Google license: Apache-2.0 - version: 1.0.0 + version: 1.1.0 --- # Generate Python Runnability Test @@ -45,12 +45,28 @@ Runs `scripts/generate_runnability_test.py` against a recipe directory. Steps: Emission is post-processed through `ruff format` when available, so multi-patch `with (...):` blocks come out already wrapped per the repo's ruff config. -5. **Write it** to `/tests/test_runnability.py` (creating `tests/` if needed). Refuses to clobber an existing file unless `--overwrite` is passed. +5. **Check that the import can actually resolve.** The generated test does `import `, which only works if the RECIPE ROOT is on `sys.path`. That is not automatic — under pytest's default `prepend` import mode only the test file's own directory (`/tests`) is inserted. The recipe root gets there in one of these ways, reported as `import_support`: + + | `import_support` | Meaning | + |---|---| + | `installable` | `pyproject.toml` declares a `[build-system]`, so `uv sync` installs the project and the package is importable. | + | `pythonpath-ini` | `[tool.pytest.ini_options].pythonpath` includes `"."`. | + | `existing-conftest` | A `conftest.py` sits at the recipe root (sufficient whatever it contains — pytest puts each conftest's own directory on `sys.path`, and for that one it IS the recipe root), or a `tests/conftest.py` that demonstrably extends `sys.path`. | + | `generated-conftest` | None of the above held, so the skill wrote `tests/conftest.py` with a path shim. | + + A `tests/conftest.py` is judged by AST, not text search: it counts only if it really touches `sys.path`, so a comment or docstring that merely *mentions* `sys.path` cannot wrongly certify it. + + **Historical bug closed by this:** the recipe root being importable was assumed rather than checked. A recipe with no `[build-system]` — common for vertical skills under `skills/`, where code lives in a plain `scripts/` directory rather than an installed package — got a test that always died with `ModuleNotFoundError`, while `prepare-python-recipe`'s `py_compile` verification still reported success. + + Adding a `[build-system]` is the better fix; the generated conftest says so and tells the maintainer to delete it once they do. + +6. **Write it** to `/tests/test_runnability.py` (creating `tests/` if needed), plus `tests/conftest.py` when step 5 called for it. Refuses to clobber an existing file unless `--overwrite` is passed. ### Edit safety - No files outside the target recipe directory are read (beyond the recipe's own `.py` files) or written. - Existing `tests/test_runnability.py` is never silently overwritten. The user must explicitly opt in with `--overwrite`. +- An existing `tests/conftest.py` is **never** overwritten, not even with `--overwrite` — the skill reports `conftest_action: skipped` and warns instead. A conftest it did not write may do something clobbering would break. - `tests/` directory is created if missing (`mkdir -p` equivalent). No other directory or file is added. - Ruff-clean by construction — the generated file passes `ruff check` and `ruff format --check` under the root config. @@ -102,7 +118,7 @@ uv run --no-project python3 .agents/skills/generate-python-runnability-test/scri --recipe-dir --dry-run ``` -Output on stdout: JSON with `agent_file`, `module_name`, `detections`, `test_content`, `action` (`would_write` / `refused_overwrite` / `error`), and `message`. Exit code `0`. +Output on stdout: JSON with `agent_file`, `module_name`, `detections`, `import_support`, `conftest_path`, `conftest_action` (`would_write` / `wrote` / `skipped` / `null`), `test_content`, `action` (`would_write` / `refused_overwrite` / `error`), `message`, and `warnings` (a list — surface every entry to the user). Exit code `0`. Note: no `--with` flags are needed — the script only uses Python's stdlib (`ast`, `argparse`, `json`, `pathlib`, `dataclasses`, `os`, `sys`, `subprocess`, `textwrap`). `uv run --no-project python3` is used (rather than a bare `python3`) to guarantee a modern managed interpreter, consistent with the other Python recipe skills; the system `python3` on macOS can still be an old version. Dry-runs remain cheap and side-effect-free. diff --git a/.agents/skills/generate-python-runnability-test/scripts/generate_runnability_test.py b/.agents/skills/generate-python-runnability-test/scripts/generate_runnability_test.py index c9e6dfaf3e..6f0c24c068 100644 --- a/.agents/skills/generate-python-runnability-test/scripts/generate_runnability_test.py +++ b/.agents/skills/generate-python-runnability-test/scripts/generate_runnability_test.py @@ -57,6 +57,7 @@ import ast import json import os +import re import subprocess import sys import textwrap @@ -64,6 +65,11 @@ from pathlib import Path from typing import Any +try: # tomllib is stdlib from 3.11; this script still supports 3.9. + import tomllib +except ModuleNotFoundError: # pragma: no cover - depends on interpreter + tomllib = None # type: ignore[assignment] + # ---------- Constants ------------------------------------------------------ # Directories the recipe walker must never descend into. @@ -111,6 +117,26 @@ """ +CONFTEST_BODY = '''"""Put the recipe root on ``sys.path`` for the runnability test. + +The test imports ``{module_name}`` from the recipe root, but this recipe +declares no ``[build-system]``, so nothing installs it as a package. Under +pytest's default ``prepend`` import mode only the test file's own directory +goes on ``sys.path``, so the import would fail with ``ModuleNotFoundError``. + +Delete this file once the recipe declares a ``[build-system]`` and is +installed with ``uv sync`` — the shim is then redundant. +""" + +import sys +from pathlib import Path + +RECIPE_ROOT = Path(__file__).resolve().parent.parent +if str(RECIPE_ROOT) not in sys.path: + sys.path.insert(0, str(RECIPE_ROOT)) +''' + + # ---------- Report dataclasses --------------------------------------------- @@ -134,6 +160,13 @@ class Report: test_content: str | None = None action: str | None = None # would_write / wrote / refused_overwrite / error message: str = "" + # How `import ` is expected to resolve when pytest runs. + # One of: installable / pythonpath-ini / existing-conftest / + # generated-conftest / unresolved. + import_support: str | None = None + conftest_path: str | None = None + conftest_action: str | None = None # would_write / wrote / skipped / none + warnings: list[str] = field(default_factory=list) def to_json(self) -> str: return json.dumps( @@ -144,9 +177,13 @@ def to_json(self) -> str: "module_name": self.module_name, "target_path": self.target_path, "detections": asdict(self.detections), + "import_support": self.import_support, + "conftest_path": self.conftest_path, + "conftest_action": self.conftest_action, "test_content": self.test_content, "action": self.action, "message": self.message, + "warnings": self.warnings, }, indent=2, ) @@ -202,6 +239,117 @@ def module_path_from_file(agent_file: Path, recipe_dir: Path) -> str: return ".".join(parts) +# ---------- Import-support detection --------------------------------------- +# +# The generated test does `import `, which only resolves if the +# RECIPE ROOT is on sys.path. That is not automatic: under pytest's default +# `prepend` import mode, only the test file's own basedir (`/tests`) +# is inserted. The recipe root gets there when the project is installed +# (`uv sync`, which needs a [build-system]) or when something explicitly puts +# it there. +# +# Historical gap: this was assumed rather than checked, so a recipe without a +# [build-system] — common for vertical skills under skills/, where code lives +# in a plain scripts/ directory — got a test that always died with +# ModuleNotFoundError, while the pipeline's py_compile check still reported +# success. + + +_BUILD_SYSTEM_RE = re.compile(r"^\s*\[build-system\]", re.MULTILINE) + + +def _load_pyproject(pyproject: Path) -> dict[str, Any] | None: + """Parse pyproject.toml, or None when unavailable/unparseable.""" + if tomllib is None or not pyproject.is_file(): + return None + try: + return tomllib.loads(pyproject.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, ValueError): + return None + + +def _has_build_system(pyproject: Path) -> bool: + """Whether pyproject.toml declares a [build-system] table.""" + data = _load_pyproject(pyproject) + if data is not None: + return isinstance(data.get("build-system"), dict) + # tomllib unavailable (< 3.11) or unparseable — fall back to a text scan + # rather than silently claiming the recipe is not installable. + try: + return bool(_BUILD_SYSTEM_RE.search(pyproject.read_text("utf-8"))) + except (OSError, UnicodeDecodeError): + return False + + +def _pytest_pythonpath_covers_root(pyproject: Path) -> bool: + """Whether [tool.pytest.ini_options].pythonpath puts the root on sys.path.""" + data = _load_pyproject(pyproject) + if not data: + return False + ini = data.get("tool", {}).get("pytest", {}).get("ini_options", {}) + entries = ini.get("pythonpath") + if isinstance(entries, str): + entries = [entries] + if not isinstance(entries, list): + return False + return any(str(e).strip() in {".", "./", ""} for e in entries) + + +def _extends_syspath(path: Path) -> bool: + """Whether a file actually touches ``sys.path`` in executable code. + + AST-based on purpose: a substring search counts a comment or docstring + that merely *mentions* sys.path (``# does not touch sys.path``), which + would wrongly certify a conftest as providing import support. Comments + never appear in the AST and a prose mention is an ``ast.Constant``, not + an attribute access, so both are excluded for free. + """ + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, SyntaxError, ValueError): + return False + for node in ast.walk(tree): + # `sys.path...` + if ( + isinstance(node, ast.Attribute) + and node.attr == "path" + and isinstance(node.value, ast.Name) + and node.value.id == "sys" + ): + return True + # `from sys import path` + if isinstance(node, ast.ImportFrom) and node.module == "sys": + if any(alias.name == "path" for alias in node.names): + return True + return False + + +def detect_import_support(recipe_dir: Path) -> str: + """Classify how the recipe root reaches sys.path under pytest. + + Returns one of ``installable`` / ``pythonpath-ini`` / + ``existing-conftest`` / ``unresolved``. + """ + pyproject = recipe_dir / "pyproject.toml" + if _has_build_system(pyproject): + return "installable" + if _pytest_pythonpath_covers_root(pyproject): + return "pythonpath-ini" + # A conftest.py AT THE RECIPE ROOT is sufficient whatever it contains: + # under prepend import mode pytest puts each conftest's own directory on + # sys.path, and for this one that directory IS the recipe root. Verified + # empirically — an entirely empty root conftest.py makes `import pkg.mod` + # resolve. + if (recipe_dir / "conftest.py").is_file(): + return "existing-conftest" + # A tests/conftest.py, by the same rule, only adds `/tests` — which + # does NOT help. It counts only if it explicitly extends sys.path itself. + tests_conftest = recipe_dir / "tests" / "conftest.py" + if tests_conftest.is_file() and _extends_syspath(tests_conftest): + return "existing-conftest" + return "unresolved" + + # ---------- AST detection helpers ------------------------------------------ @@ -734,6 +882,36 @@ def run( target = tests_dir / "test_runnability.py" report.target_path = str(target) + # Will `import ` actually resolve? If not, the test compiles + # fine but dies at run time — so provide the missing sys.path shim rather + # than emitting a test that cannot pass. + report.import_support = detect_import_support(recipe_dir) + conftest = tests_dir / "conftest.py" + if report.import_support == "unresolved": + if conftest.exists(): + # Never clobber a conftest we did not write; it may already do + # something we would break. + report.conftest_action = "skipped" + report.conftest_path = str(conftest) + report.warnings.append( + f"{recipe_dir / 'pyproject.toml'} declares no [build-system], " + f"so the recipe is not installed and the recipe root is not " + f"on sys.path. {conftest} already exists and was left alone — " + f"confirm it puts the recipe root on sys.path, or " + f"`import {report.module_name}` will fail at run time." + ) + else: + report.conftest_action = "would_write" if dry_run else "wrote" + report.conftest_path = str(conftest) + report.import_support = "generated-conftest" + report.warnings.append( + f"No [build-system] in pyproject.toml, so nothing installs " + f"the recipe and `import {report.module_name}` would fail " + f"under pytest. Generated {conftest} to put the recipe root " + f"on sys.path. Adding a [build-system] is the better fix; " + f"delete the conftest once you do." + ) + if dry_run: report.action = "would_write" return report @@ -754,8 +932,23 @@ def run( report.message = f"Failed to write {target}: {e}" return report + if report.conftest_action == "wrote": + content = LICENSE_HEADER + CONFTEST_BODY.format( + module_name=report.module_name + ) + try: + conftest.write_text(content, encoding="utf-8") + except OSError as e: + # The test itself landed; report the shim failure without + # pretending the whole run failed. + report.conftest_action = "error" + report.warnings.append(f"Failed to write {conftest}: {e}") + report.action = "wrote" - report.message = f"Wrote {target}." + written = [str(target)] + if report.conftest_action == "wrote": + written.append(str(conftest)) + report.message = f"Wrote {', '.join(written)}." return report diff --git a/.agents/skills/generate-python-runnability-test/tests/conftest.py b/.agents/skills/generate-python-runnability-test/tests/conftest.py new file mode 100644 index 0000000000..2d55a0f3c1 --- /dev/null +++ b/.agents/skills/generate-python-runnability-test/tests/conftest.py @@ -0,0 +1,24 @@ +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Make this skill's scripts/ directory importable from its tests. + +Keeping the path shim inside the skill (rather than in the repo-root pytest +config) preserves the skill as a self-contained, portable bundle. +""" + +import sys +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).parent.parent / "scripts" +sys.path.insert(0, str(SCRIPTS_DIR)) diff --git a/.agents/skills/generate-python-runnability-test/tests/test_generate_runnability_test.py b/.agents/skills/generate-python-runnability-test/tests/test_generate_runnability_test.py new file mode 100644 index 0000000000..5735a9166c --- /dev/null +++ b/.agents/skills/generate-python-runnability-test/tests/test_generate_runnability_test.py @@ -0,0 +1,187 @@ +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Unit tests for the generate-python-runnability-test skill script. + +Focused on import-support detection: the generated test does +``import ``, which only resolves when the RECIPE ROOT is on +``sys.path``. That was previously assumed rather than checked, so a recipe +with no ``[build-system]`` got a test that always died with +``ModuleNotFoundError`` while ``py_compile`` still reported success. +""" + +import ast +from pathlib import Path + +import generate_runnability_test as m + + +def _write(path: Path, content: str = "") -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + return path + + +def _recipe(tmp_path: Path, pyproject: str, pkg: str = "app") -> Path: + _write(tmp_path / pkg / "agent.py", "root_agent = object()\n") + _write(tmp_path / "pyproject.toml", pyproject) + return tmp_path + + +# --------------------------------------------------------------------------- +# detect_import_support +# --------------------------------------------------------------------------- + + +def test_build_system_means_installable(tmp_path): + recipe = _recipe( + tmp_path, + '[project]\nname = "x"\n' + '[build-system]\nrequires = ["hatchling"]\n' + 'build-backend = "hatchling.build"\n', + ) + assert m.detect_import_support(recipe) == "installable" + + +def test_pythonpath_ini_covering_root(tmp_path): + recipe = _recipe( + tmp_path, + '[project]\nname = "x"\n' + '[tool.pytest.ini_options]\npythonpath = ["."]\n', + ) + assert m.detect_import_support(recipe) == "pythonpath-ini" + + +def test_pythonpath_ini_not_covering_root(tmp_path): + recipe = _recipe( + tmp_path, + '[project]\nname = "x"\n' + '[tool.pytest.ini_options]\npythonpath = ["src"]\n', + ) + assert m.detect_import_support(recipe) == "unresolved" + + +def test_root_conftest_is_sufficient_even_when_empty(tmp_path): + # Verified against real pytest: under prepend import mode a conftest's own + # directory goes on sys.path, and for a ROOT conftest that is the recipe + # root — so its contents are irrelevant. + recipe = _recipe(tmp_path, '[project]\nname = "x"\n') + _write(recipe / "conftest.py", "") + assert m.detect_import_support(recipe) == "existing-conftest" + + +def test_tests_conftest_counts_only_when_it_touches_syspath(tmp_path): + recipe = _recipe(tmp_path, '[project]\nname = "x"\n') + _write(recipe / "tests" / "conftest.py", "# just fixtures\n") + # tests/conftest.py only adds /tests, which does not help. + assert m.detect_import_support(recipe) == "unresolved" + + # A mere MENTION of sys.path in prose must not count as import support. + _write( + recipe / "tests" / "conftest.py", + '"""Fixtures. Deliberately does not touch sys.path."""\n', + ) + assert m.detect_import_support(recipe) == "unresolved" + + _write( + recipe / "tests" / "conftest.py", + "import sys\nsys.path.insert(0, '..')\n", + ) + assert m.detect_import_support(recipe) == "existing-conftest" + + +def test_missing_pyproject_is_unresolved(tmp_path): + _write(tmp_path / "app" / "agent.py", "root_agent = object()\n") + assert m.detect_import_support(tmp_path) == "unresolved" + + +def test_unparseable_pyproject_falls_back_to_text_scan(tmp_path): + # Malformed TOML must not be read as "no build system" — the text scan + # is the safety net. + recipe = _recipe( + tmp_path, + '[project\nname = "x"\n[build-system]\nrequires = ["hatchling"]\n', + ) + assert m.detect_import_support(recipe) == "installable" + + +# --------------------------------------------------------------------------- +# conftest generation +# --------------------------------------------------------------------------- + + +def test_generates_conftest_when_import_would_fail(tmp_path): + recipe = _recipe(tmp_path, '[project]\nname = "x"\n', pkg="scripts") + + report = m.run(recipe, None, dry_run=False, overwrite=False) + + assert report.action == "wrote" + assert report.import_support == "generated-conftest" + assert report.conftest_action == "wrote" + conftest = recipe / "tests" / "conftest.py" + assert conftest.is_file() + content = conftest.read_text(encoding="utf-8") + assert "sys.path.insert" in content + ast.parse(content) # must be valid Python + assert any("build-system" in w for w in report.warnings) + + +def test_no_conftest_generated_when_installable(tmp_path): + recipe = _recipe( + tmp_path, + '[project]\nname = "x"\n' + '[build-system]\nrequires = ["hatchling"]\n' + 'build-backend = "hatchling.build"\n', + ) + + report = m.run(recipe, None, dry_run=False, overwrite=False) + + assert report.import_support == "installable" + assert report.conftest_action is None + assert not (recipe / "tests" / "conftest.py").exists() + assert report.warnings == [] + + +def test_existing_conftest_is_never_clobbered(tmp_path): + recipe = _recipe(tmp_path, '[project]\nname = "x"\n') + original = "# hand-written, does not touch sys.path\n" + _write(recipe / "tests" / "conftest.py", original) + + report = m.run(recipe, None, dry_run=False, overwrite=False) + + assert report.conftest_action == "skipped" + assert (recipe / "tests" / "conftest.py").read_text() == original + assert any("left alone" in w for w in report.warnings) + + +def test_dry_run_writes_nothing(tmp_path): + recipe = _recipe(tmp_path, '[project]\nname = "x"\n') + + report = m.run(recipe, None, dry_run=True, overwrite=False) + + assert report.action == "would_write" + assert report.conftest_action == "would_write" + assert not (recipe / "tests" / "conftest.py").exists() + assert not (recipe / "tests" / "test_runnability.py").exists() + + +def test_report_json_includes_new_fields(tmp_path): + import json + + recipe = _recipe(tmp_path, '[project]\nname = "x"\n') + report = m.run(recipe, None, dry_run=True, overwrite=False) + + payload = json.loads(report.to_json()) + assert payload["import_support"] == "generated-conftest" + assert payload["conftest_action"] == "would_write" + assert isinstance(payload["warnings"], list) diff --git a/.agents/skills/prepare-python-recipe/SKILL.md b/.agents/skills/prepare-python-recipe/SKILL.md index c60dbbf304..a2446d15e5 100644 --- a/.agents/skills/prepare-python-recipe/SKILL.md +++ b/.agents/skills/prepare-python-recipe/SKILL.md @@ -4,11 +4,13 @@ description: > End-to-end orchestration to prepare or update a Python recipe under core/python/, contrib/python/, or skills/// so it passes every check in .github/workflows/python-validate-recipe.yml. - Runs seven phases in + Runs eight phases in order on an already-in-place recipe: manifest.yaml generation, environment-variable extraction, pyproject.toml alignment, ruff - format+check, per-recipe `uv lock`, runnability-test generation, and a - final `py_compile` verification of the generated test file. Assumes + format+check, per-recipe `uv lock`, runnability-test generation, + compile-and-run verification of the generated test file, and a final + pass through the repo's own `validate manifest` / `validate structure` + validators. Assumes the user has already done the manual prep (deactivated any venv, `git pull` and `uv sync` from the repo root, placed the recipe at its target path, @@ -25,7 +27,7 @@ description: > metadata: author: Google license: Apache-2.0 - version: 1.0.0 + version: 1.1.0 --- # Prepare Python Recipe @@ -65,25 +67,28 @@ If the user has NOT done these and asks you to run the skill anyway, tell them t ## What This Skill Does -Runs seven ordered phases against a target recipe. Each phase either invokes an existing sub-skill (or its underlying script) or runs a repo-standard command: +Runs eight ordered phases against a target recipe. Each phase either invokes an existing sub-skill (or its underlying script) or runs a repo-standard command: 1. **Manifest** — generate `manifest.yaml` if missing. Ownership placeholders (`ownership.team`, `ownership.poc`) are LEFT AS-IS — never replaced mid-pipeline. See "Canonical placeholder strings" above. 2. **Environment variables** — extract env vars used by the recipe into `.env.example`; ensure `load_dotenv()` is bootstrapped and `python-dotenv` is a dep. -3. **Align pyproject.toml** — remove `[tool.ruff*]`, raise `requires-python` floor, ensure `[project].name` matches folder, reconcile description with manifest, and ensure `[[tool.uv.index]]` declares public PyPI as default (needed to bypass corp Airlock). +3. **Align pyproject.toml** — remove `[tool.ruff*]`, raise `requires-python` floor, ensure `[project].name` matches folder, reconcile description with manifest, ensure `[[tool.uv.index]]` declares public PyPI as default (needed to bypass corp Airlock), and report stale sub-3.11 version references plus a `testpaths` that would exclude the runnability test. 4. **Lint** — `ruff format` + `ruff check --fix` on the recipe (from the repo root, so the root ruff config wins). **Must run AFTER Phase 3** — align removes any recipe-local `[tool.ruff*]` block, and that removal is what makes the root config the effective one. Running lint before align would check against the recipe's (often more permissive) local config and miss violations that CI will later catch. 5. **Recipe `uv lock`** — regenerate `uv.lock` so it reflects the post-align `pyproject.toml`. Does NOT install into `.venv/` — that's a heavier step the user runs after they've reviewed the diff. `uv lock` just resolves and records; `uv sync` would download and install every wheel, which is scope-creep for a "prepare" pipeline. -6. **Runnability test** — generate `tests/test_runnability.py` if missing (or ask before overwriting). -7. **Verify (compile-check)** — `uv run --no-project python3 -m py_compile /tests/test_runnability.py` — a lightweight sanity check that the generated (or existing) test file is syntactically valid Python. Deterministic; does NOT execute the test, resolve imports, or require `.env` to exist. If it fails, the master reports the error verbatim and moves on (the summary marks Phase 7 as failed). The master does NOT attempt to diagnose or fix — that's a human review task. +6. **Runnability test** — generate `tests/test_runnability.py` if missing (or ask before overwriting), plus a `tests/conftest.py` path shim when the recipe isn't installable. +7. **Verify (compile + run)** — `py_compile` the runnability test, then run it with pytest. The compile step is a syntax check; running it is what proves the test's `import` can actually resolve (`--collect-only` would not — the guarded test shape puts the import inside the test function). The test is side-effect-free by construction. +8. **Validate (repo validators)** — run `uv run validate manifest` and `uv run validate structure` on the recipe. This is the phase that catches everything the seven build phases don't model: required files, required directories (`tests/unit/` for vertical skills), size limits, naming. At the end, print a summary table and remind the user to `git diff` and commit — the skill never commits. +**Why Phase 8 exists.** The pipeline used to end at Phase 7 and report a clean run while `uv run validate structure` failed — the recipe passed every phase the skill modelled and was still rejected by CI. Reimplementing policy checks inside this skill would guarantee drift, so the pipeline defers to the repo's own validators as the last word. + --- ## Rules for the Agent -1. **Ask for `--recipe-dir` up front** if the user hasn't given one. All seven phases operate on the same recipe. +1. **Ask for `--recipe-dir` up front** if the user hasn't given one. All eight phases operate on the same recipe. -2. **Confirm before starting**. The pipeline touches many files. Show the user the plan (the seven phases + the target recipe path) and ask for a single "yes, go ahead" before Phase 1. Do NOT prompt again for each phase unless a decision is required (see rules 5 and 6). +2. **Confirm before starting**. The pipeline touches many files. Show the user the plan (the eight phases + the target recipe path) and ask for a single "yes, go ahead" before Phase 1. Do NOT prompt again for each phase unless a decision is required (see rules 5 and 6). 3. **Invoke sub-skill SCRIPTS directly** (not the sub-skills' own agent-facing SKILL.md). Reason: sub-skills each have their own "want me to apply?" prompt. In master-orchestration mode the user has already opted into apply for the whole pipeline; individual prompts would be noise. Command lines for each sub-script are given in each phase below. @@ -143,7 +148,7 @@ If the user has not specified the recipe directory, ask for it before proceeding If it isn't a directory, stop immediately with that message — do NOT show the plan or prompt. -**Step 0b — Verify the recipe folder name matches CI's naming rules.** `python-validate-recipe.yml`'s Check 1 (folder-name regex + max length) rejects folders that don't match `^[a-z][a-z-]*$` or exceed `.github/policy.yml` `recipe_naming.max_folder_name_length`. Historically the pipeline was BLIND to this — it would run all 7 phases against a folder named `data_science` or `MyBadName`, report success, and let CI reject the PR later (or worse: Phase 3's `project-name-matches-folder` would propagate the bad name into `[project].name`). This check catches it up front. +**Step 0b — Verify the recipe folder name matches CI's naming rules.** `python-validate-recipe.yml`'s Check 1 (folder-name regex + max length) rejects folders that don't match `^[a-z][a-z-]*$` or exceed `.github/policy.yml` `recipe_naming.max_folder_name_length`. Historically the pipeline was BLIND to this — it would run every phase against a folder named `data_science` or `MyBadName`, report success, and let CI reject the PR later (or worse: Phase 3's `project-name-matches-folder` would propagate the bad name into `[project].name`). This check catches it up front. ```bash MAX_LEN=$(uv run --no-project --with pyyaml python3 .github/scripts/load_policy.py recipe_naming.max_folder_name_length) @@ -157,7 +162,30 @@ The check exits 0 silently on a compliant name; on violation it exits 1 with the **Only proceed past this step if the folder-name check passed.** -**Step 0c — Show the plan and get confirmation.** Before composing the plan, glance at the recipe for anything non-standard (package not called `app/`, `.env.example` outside root, missing `tests/`, extra Python source dirs, deprecated model literals per `AGENTS.md`). If any will affect what the pipeline does, flag them briefly in the plan message so the user isn't surprised mid-pipeline. Skip the flags entirely for a standard recipe. +**Step 0c — For a recipe under `skills/`, check the required directories.** `.github/policy.yml` `required_dirs.by_root.skills` mandates a fixed shape for every vertical skill — `scripts/`, `assets/`, `references/`, and `tests/unit/`. None of the eight phases creates these, so a missing one survives the whole pipeline and fails `validate structure` in Phase 8 (and CI). Surfacing it here means the user can create the directory before anything else runs, rather than reading about it in the final summary. + +Skip this step entirely for `core/` and `contrib/` recipes — `required_dirs.by_root` is empty for both. + +```bash +uv run --no-project --with pyyaml python3 -c " +import pathlib, sys, yaml +recipe = pathlib.Path('') +policy = yaml.safe_load(open('.github/policy.yml')) +needed = policy.get('required_dirs', {}).get('by_root', {}).get('skills', []) or [] +missing = [d for d in needed if not (recipe / d).is_dir()] +print('MISSING_DIRS: ' + (', '.join(missing) if missing else '(none)')) +" +``` + +This is INFORMATIONAL, not a halt. An empty directory satisfies the check, and git cannot commit an empty directory, so the fix is a `.gitkeep`: + +```bash +mkdir -p /tests/unit && touch /tests/unit/.gitkeep +``` + +Mention any missing directories in the Step 0d plan message and offer to create them with `.gitkeep` files as part of the run. If the user agrees, create them right after they confirm the plan and before Phase 1; record them in the summary's "Files created" list. If they decline, carry the item into the final TODO list. Do NOT create them unasked — an empty scaffold directory the user didn't want is still clutter. + +**Step 0d — Show the plan and get confirmation.** Before composing the plan, glance at the recipe for anything non-standard (package not called `app/`, `.env.example` outside root, missing `tests/`, extra Python source dirs, deprecated model literals per `AGENTS.md`). If any will affect what the pipeline does, flag them briefly in the plan message so the user isn't surprised mid-pipeline. Skip the flags entirely for a standard recipe. Then flag the assumptions the pipeline is making and show the user the plan. Do NOT frame these as "prerequisites" — they're a heads-up so the user can push back if any assumption is wrong, not a preflight checklist for the user to tick off: @@ -166,17 +194,23 @@ Then flag the assumptions the pipeline is making and show the user the plan. Do > - You've run `git pull` and `uv sync` at the repo root. > - `` is already at its target path (and renamed to its final basename). > -> I'll run the prepare-python-recipe pipeline on `` — 7 phases: +> I'll run the prepare-python-recipe pipeline on `` — 8 phases: > 1. Generate manifest.yaml (if missing) > 2. Extract env vars into .env.example > 3. Align pyproject.toml > 4. Ruff format + check --fix > 5. uv lock inside the recipe (regenerates uv.lock; does NOT install .venv/) > 6. Generate tests/test_runnability.py (if missing) -> 7. Compile-check the runnability test (`py_compile`; reports failure but does not debug) +> 7. Verify the runnability test compiles and runs +> 8. Run the repo validators (`validate manifest`, `validate structure`) > > Nothing gets committed — you'll `git diff` at the end. Proceed? +If Step 0c found missing required directories, add one line before "Nothing gets committed": + +> `` is a vertical skill and is missing `tests/unit/`, which +> `.github/policy.yml` requires. Want me to create it with a `.gitkeep`? + Get a yes-or-no. If no, stop. ### Phase 1 — manifest.yaml @@ -239,9 +273,11 @@ uv run --no-project --with tomlkit --with 'ruamel.yaml' --with packaging \ [--description-source=] ``` -**The align script exits `1` (non-zero) whenever any check is `report_only`** — that is expected, not a hard error, so do NOT apply rule 7's halt to it. Decide from the JSON, not the exit code: if the apply run's only non-clean checks are `report_only`, note them in the summary (the master does NOT auto-fix these) and continue; halt only if a check has status `error`. Two rules can produce `report_only`: - - `build-system-present` (missing `[build-system]` — backend choice is editorial) +**The align script exits `1` (non-zero) whenever any check is `report_only`** — that is expected, not a hard error, so do NOT apply rule 7's halt to it. Decide from the JSON, not the exit code: if the apply run's only non-clean checks are `report_only`, note them in the summary (the master does NOT auto-fix these) and continue; halt only if a check has status `error`. Four rules can produce `report_only`: + - `build-system-present` (missing `[build-system]` — backend choice is editorial). **Note the knock-on for Phase 6:** with no `[build-system]` the recipe is never installed, so the runnability test's `import` cannot resolve on its own and the generator will emit a `tests/conftest.py` path shim. Mention both together in the summary rather than as two unrelated items. - `default-pypi-index` (a default index is declared but points somewhere other than public PyPI — divergence may be intentional) + - `stale-python-version-refs` (files still referencing a Python below the 3.11 floor). Relay `details.files` in the summary and flag **executable** files first — a bootstrap script whose interpreter allowlist still accepts 3.10 will build a venv the recipe then refuses to install into. Prose files are lower priority. + - `runnability-test-in-testpaths` (a `testpaths` setting that excludes `tests/test_runnability.py`, so a bare `pytest` never collects it) Progress line: `Phase 3 (align): fix(es) applied; report-only issue(s) left.` @@ -306,32 +342,83 @@ uv run --no-project python3 .agents/skills/generate-python-runnability-test/scri If the script errors (no `agent.py` found), surface the message and offer to re-run with `--agent-file ` when the user tells you where the entry point is. -Progress line: `Phase 6 (runnability test): generated | kept existing | regenerated.` +**Read the report's `import_support`, `conftest_action`, and `warnings` fields.** The generated test does `import `, which only resolves if the recipe root is on `sys.path` — not automatic under pytest. When the recipe declares no `[build-system]` the generator also writes a `tests/conftest.py` path shim (`conftest_action: wrote`) and explains why in `warnings`. Relay every warning; do not silently drop them. If `conftest_action` is `skipped`, an existing `tests/conftest.py` was left untouched and may not provide the shim — carry that into the summary as a Manual TODO. -### Phase 7 — verify (compile-check the runnability test) +Progress line: `Phase 6 (runnability test): generated | kept existing | regenerated[; tests/conftest.py path shim written (recipe has no [build-system])].` -Runs LAST. Lightweight sanity check that the generated (or existing) `tests/test_runnability.py` is at least syntactically valid Python. Deliberately weaker than `uv run pytest`: it does NOT execute the test, resolve imports, or require `.env` to be populated. Its only purpose is to catch generator bugs (invalid Python emitted by Phase 6) and gross syntax errors in a hand-edited test file. +### Phase 7 — verify (compile + run the runnability test) -**7a. Check the test file exists.** If Phase 6 skipped generation (agent.py not found, so no test was written) or the user chose not to regenerate an existing broken test, there may be nothing to compile. Skip and record it. +Two escalating checks on the generated (or existing) `tests/test_runnability.py`. The test is designed to be side-effect-free (it patches `vertexai.init` and `google.auth.default`), so neither step needs `.env`, ADC, or network. + +**7a. Check the test file exists.** If Phase 6 skipped generation (agent.py not found, so no test was written) or the user chose not to regenerate an existing broken test, there may be nothing to check. Skip and record it. ```bash [ -f /tests/test_runnability.py ] && echo exists || echo missing ``` -**7b. Compile.** +**7b. Compile** — is it valid Python? ```bash uv run --no-project python3 -m py_compile /tests/test_runnability.py ``` -Use `uv run --no-project python3` here too — not a bare `python`/`python3`. Two reasons: `python` may not be on PATH at all on some systems, and (more importantly) a guarded test with multiple patches emits a parenthesized `with (...)` block, which is **Python 3.10+ syntax**. Compiling it under an older system interpreter would report a spurious `SyntaxError` on a file that is actually valid. uv's managed interpreter is 3.11+, so this is a true syntax check rather than a version artifact. +Use `uv run --no-project python3` here — not a bare `python`/`python3`. Two reasons: `python` may not be on PATH at all on some systems, and (more importantly) a guarded test with multiple patches emits a parenthesized `with (...)` block, which is **Python 3.10+ syntax**. Compiling it under an older system interpreter would report a spurious `SyntaxError` on a file that is actually valid. uv's managed interpreter is 3.11+, so this is a true syntax check rather than a version artifact. + +**7c. Run it** — does the import actually resolve? + +```bash +uv run --no-project --with pytest pytest tests/test_runnability.py -q +``` + +Run this WITH `workdir = ` so pytest's rootdir matches what a user would get running the test themselves. + +**Why run it rather than `--collect-only`.** `py_compile` only parses; it passes happily on a test whose `import app.agent` can never resolve. But `--collect-only` is no better here: the *guarded* test shape (the common one for ADK recipes) puts the import INSIDE the test function, under a `with patch(...)` block, so collection imports the test module without ever touching the recipe's module. Only actually running the test exercises the import. + +**A third-party `ModuleNotFoundError` is EXPECTED here, not a finding.** Phase 5 ran `uv lock`, not `uv sync`, so the recipe's dependencies are NOT installed. Classify by the module named in the error: + +- The name is a **dependency** (`vertexai`, `google.adk`, `pandas`) → expected. Report `deps not installed`. Note that this outcome is **inconclusive** about the import path: the dependency failure fires before the recipe's own import is reached, so the test proves nothing either way. Fall back to Phase 6's `import_support` field for that question (`installable` / `pythonpath-ini` / `existing-conftest` / `generated-conftest` all mean the path is provided; only `unresolved` is a problem). +- The name is the recipe's **own top-level module** (the first segment of Phase 6's `module_name` — e.g. `scripts` from `scripts.agent`) → REAL finding. The import path is broken and the test can never pass, whatever else is installed. Report per outcome: -- Exit 0 → **pass.** Progress line: `Phase 7 (verify): compile OK.` -- Exit non-zero → **fail.** Print the stderr verbatim in the summary as a Manual TODO. Do NOT attempt to diagnose, retry, or auto-fix. Do NOT halt the pipeline (Phase 7 is the last phase anyway; the summary still gets printed). Progress line: `Phase 7 (verify): compile FAILED — .` +- Compile 0, test passes → **pass.** Progress line: `Phase 7 (verify): compile OK, test passes.` +- Compile 0, fails on a dependency → **pass with note.** Progress line: `Phase 7 (verify): compile OK; test not run ( not installed — run uv sync). Import path: .` +- Compile 0, fails on the recipe's own module → **fail.** Progress line: `Phase 7 (verify): compile OK but is not importable.` Record as a Manual TODO; the usual cause is a missing `[build-system]` (Phase 3 report-only) with no conftest shim. +- Compile 0, fails an ASSERTION (`root_agent is None`) → **fail.** A genuine recipe defect, not an environment one. Report it verbatim. +- Compile non-zero → **fail.** Print the stderr verbatim in the summary as a Manual TODO and SKIP 7c (a file that doesn't parse cannot run). Do NOT attempt to diagnose, retry, or auto-fix. Progress line: `Phase 7 (verify): compile FAILED — .` - File missing → **skip.** Progress line: `Phase 7 (verify): skipped (no tests/test_runnability.py to check).` -Note: passing Phase 7 does NOT mean the recipe actually runs — it means the test file is valid Python. Actually running the test (which validates that `agent.py` imports and `root_agent` is non-None) is still a manual `uv run pytest` step listed under "Next steps" in the summary. +Never halt the pipeline on Phase 7 — Phase 8 still runs and the summary still prints. + +Note: a `deps not installed` result means Phase 7 has NOT proven the recipe runs. Say so plainly in the summary rather than implying a clean bill of health; the real confirmation is the manual `uv sync && uv run pytest` step under "Next steps". + +### Phase 8 — validate (the repo's own validators) + +Runs LAST. Phases 1–7 model what the pipeline knows how to *build*; this phase asks the repo whether the result is actually acceptable. It is deliberately a thin wrapper: the checks live in `tools/` and `.github/policy.yml`, and reimplementing any of them here would guarantee drift. + +**Historical gap this closes:** the pipeline ended at Phase 7 and printed a clean summary for a recipe that `validate structure` rejected for a missing `tests/unit/` directory. Every modelled phase passed; CI still failed the PR. + +**8a. Validate the manifest.** + +```bash +uv run validate manifest +``` + +**8b. Validate the structure.** + +```bash +uv run validate structure +``` + +Both run from the repo root with a repo-root-relative ``, never an absolute path. Both exit non-zero on failure — do NOT apply rule 7's halt (Phase 8 is last, and its failures are findings to report, not crashes). + +**Interpreting the output — two distinct kinds of failure:** + +- **Ownership placeholder failures are EXPECTED.** `ownership.team` / `ownership.poc` still hold the canonical placeholders, and the validator fails deliberately until a human replaces them. Report these as `expected` in the summary, not as a problem the pipeline caused. They are already item 1 of the TODO list. +- **Everything else is a REAL finding.** Missing required files or directories, size-limit violations, schema errors, naming violations. List each verbatim in Section 3 with the fix. + +Progress line: `Phase 8 (validate): manifest ; structure .` + +If the only failures across both validators are the two ownership placeholders, the recipe is in the expected end state — say so plainly rather than presenting it as a failure. --- @@ -340,9 +427,9 @@ Note: passing Phase 7 does NOT mean the recipe actually runs — it means the te While the pipeline runs, print a short progress line per phase (see above). Do NOT dump raw JSON. Do NOT re-render sub-skill tables. **Track three things as the pipeline runs** so you can report them at the end: -- Every file the pipeline created or modified (across all seven phases). Observe this from each sub-script's stdout plus your own knowledge of what each phase touches (Phase 1 → `manifest.yaml`; Phase 2 → `.env.example`, package `__init__.py`, `pyproject.toml`, any source files where a hardcoded model name was replaced; Phase 3 → `pyproject.toml`; Phase 4 → any `.py` under the recipe; Phase 5 → `uv.lock`; Phase 6 → `tests/test_runnability.py`; Phase 7 → nothing). -- Every action the pipeline **attempted but couldn't complete** (a phase halted by rule 7, Phase 4 unfixable ruff, Phase 7 compile fail). -- Every deferred item that needs human follow-up (Phase 3 `report_only`). +- Every file the pipeline created or modified (across all eight phases). Observe this from each sub-script's stdout plus your own knowledge of what each phase touches (Phase 0c → `tests/unit/.gitkeep` and any other required dir, if the user opted in; Phase 1 → `manifest.yaml`; Phase 2 → `.env.example`, package `__init__.py`, `pyproject.toml`, any source files where a hardcoded model name was replaced; Phase 3 → `pyproject.toml`; Phase 4 → any `.py` under the recipe; Phase 5 → `uv.lock`; Phase 6 → `tests/test_runnability.py` and possibly `tests/conftest.py`; Phases 7 and 8 → nothing). +- Every action the pipeline **attempted but couldn't complete** (a phase halted by rule 7, Phase 4 unfixable ruff, Phase 7 compile or run failure). +- Every deferred item that needs human follow-up (Phase 3 `report_only`, Phase 8 real validator findings). At the end, print the sections below in order. Section 3 is **conditional** — omit it entirely if nothing belongs in it. Sections 1, 2, and 4 always print. @@ -356,10 +443,13 @@ At the end, print the sections below in order. Section 3 is **conditional** — | 4. Lint | ok | 12 files formatted, 4 issues auto-fixed | | 5. Recipe lock | ok | done | | 6. Runnability test | ok | generated | -| 7. Verify (compile-check) | ok | tests/test_runnability.py compiles | +| 7. Verify (compile + run) | ok | compiles; test passes | +| 8. Validate (repo validators) | ok | manifest + structure clean except ownership placeholders | Use plain words in the Outcome column (`ok` / `skipped` / `failed`). No emoji unless the user asked for them. +Phase 8 is `ok` when the only failures are the two ownership placeholders — that is the expected end state, not a defect. Use `failed` only for real validator findings. + ### 2. Files created or modified Short bullet list, grouped **Created** and **Modified**, one line per file. Aggregate large groups (e.g. "12 `.py` files formatted (Phase 4)") rather than listing each individually. Omit files that were checked but untouched. If a group is empty, drop its heading. If nothing changed at all, print `Nothing changed — the recipe was already fully aligned.` @@ -369,13 +459,15 @@ Example: > **Created** > - `manifest.yaml` (Phase 1) > - `tests/test_runnability.py` (Phase 6) +> - `tests/conftest.py` (Phase 6 — sys.path shim; recipe has no `[build-system]`) +> - `tests/unit/.gitkeep` (Phase 0c — required for `skills/` recipes) > - `uv.lock` (Phase 5) > > **Modified** > - `pyproject.toml` (Phases 2, 3 — added `python-dotenv`; aligned rules) > - `/__init__.py` (Phase 2 — added `load_dotenv()`) > - `.env.example` (Phase 2 — 3 vars added) -> - `agent.py` (Phase 2 — replaced hardcoded model name with `os.getenv("MODEL_NAME")`) +> - `agent.py` (Phase 2 — replaced hardcoded model name with `os.getenv("EMBEDDING_MODEL")`) > - 12 `.py` files formatted, 4 auto-fixed (Phase 4) ### 3. What the skill tried but couldn't complete (conditional — omit section if empty) @@ -388,12 +480,16 @@ Cases that go here: - **Halted phase (rule 7 hard error)**: a phase's script exited non-zero and the pipeline stopped. Show the phase, the command that failed, and a one-line snippet of the stderr. Explicitly note which phases did NOT run as a result. - **Phase 4 unfixable ruff**: `ruff check --fix` ran but couldn't auto-fix some violations. Show `:` — `` for each. +- **Phase 6 conftest skipped**: an existing `tests/conftest.py` blocked the path shim the generator wanted to write. - **Phase 7 compile fail**: `py_compile` on `tests/test_runnability.py` returned an error. Show a one-line snippet of the stderr. +- **Phase 7 failure on the recipe's own module**: the test's import can't resolve. (A dependency-not-installed failure does NOT belong here — that's expected after `uv lock`.) +- **Phase 8 real validator findings**: anything `validate manifest` / `validate structure` reported other than the two ownership placeholders. One line each, verbatim. Example: > - **Phase 4 (ruff)** — 2 violations remain that `ruff check --fix` can't auto-fix: `app/deploy.py:276` (`C901`, `PLR0915`), `app/tools.py:52` (`C901`). Refactor or add `# noqa: ` at the def line. > - **Phase 7 (verify)** — `uv run --no-project python3 -m py_compile tests/test_runnability.py` failed: `SyntaxError: invalid syntax (line 14)`. Likely a generator bug or hand-edit; regenerate or fix before running pytest. +> - **Phase 8 (validate)** — `validate structure` reports `Required directory 'tests/unit/' is missing`. Create it with a `.gitkeep`. ### 4. What you still need to do @@ -407,8 +503,12 @@ A single short TODO list. Keep every entry to one line. Standard items come firs **Conditional — include ONLY if the phase raised it:** -- **Phase 3 report-only, `build-system`**: add a `[build-system]` block to `pyproject.toml` — see `.agents/skills/align-recipe-pyproject/SKILL.md` for hatchling / uv_build templates. +- **Phase 3 report-only, `build-system`**: add a `[build-system]` block to `pyproject.toml` — see `.agents/skills/align-recipe-pyproject/SKILL.md` for hatchling / uv_build templates. If Phase 6 also wrote a `tests/conftest.py` shim, note that adding the build-system makes the shim redundant and it can be deleted. - **Phase 3 report-only, `pypi-index`**: `[[tool.uv.index]]` default points somewhere other than public PyPI — verify this is intentional or fix per the align skill. +- **Phase 3 report-only, `stale-python-version-refs`**: list the files, executable ones first (a bootstrap script's interpreter allowlist is a live bug; prose is cosmetic). +- **Phase 3 report-only, `runnability-test-in-testpaths`**: add `"tests"` to `[tool.pytest.ini_options].testpaths`, or the runnability test never runs under a bare `pytest`. +- **Phase 0c / Phase 8, missing required directory**: create it with a `.gitkeep` (e.g. `mkdir -p /tests/unit && touch /tests/unit/.gitkeep`) if the user declined during Phase 0c. +- **Phase 8, other validator findings**: one line per finding with the fix. **Commands — always show:** @@ -419,6 +519,6 @@ uv run pytest tests/test_runnability.py -v # confirm the runnability test ac # commit when you're happy ``` -`uv sync` is what actually installs the recipe's dependencies into `.venv/`. The pipeline stopped at `uv lock` on purpose — installing is heavier and better done after you've reviewed the diff. +`uv sync` is what actually installs the recipe's dependencies into `.venv/`. The pipeline stopped at `uv lock` on purpose — installing is heavier and better done after you've reviewed the diff. It is also why Phase 7 usually cannot run the test to completion — it stops at the first missing dependency. Then stop. Do NOT commit. End your turn. diff --git a/.github/workflows/tools-tests.yml b/.github/workflows/tools-tests.yml index fce125a81d..8ba9ded09e 100644 --- a/.github/workflows/tools-tests.yml +++ b/.github/workflows/tools-tests.yml @@ -58,9 +58,10 @@ jobs: pytest: name: pytest (tools + skills) runs-on: ubuntu-latest - # Tooling + skills tests are lightweight (stdlib + pytest); 15 min - # leaves headroom for cold uv-sync while still protecting against a - # stuck job riding the default 6-hour timeout. + # Tooling + skills tests are lightweight (pytest plus a few pure-Python + # parsing libs in the dev group); 15 min leaves headroom for cold + # uv-sync while still protecting against a stuck job riding the default + # 6-hour timeout. timeout-minutes: 15 steps: diff --git a/pyproject.toml b/pyproject.toml index 649ebf25b1..11c14eb871 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -16,8 +16,21 @@ validate = "validate:main" [dependency-groups] # Test-only dependencies for the repo tooling (tools/) and skill scripts # (.agents/skills/*/scripts/). Install with `uv sync --dev`. +# +# tomlkit, ruamel.yaml and packaging are imported directly by +# .agents/skills/align-recipe-pyproject/scripts/align_pyproject.py, which +# pytest imports via that skill's tests. Without them, collection of the +# whole suite fails. They live here rather than in [project.dependencies] +# because nothing under tools/ needs them at runtime. +# +# packaging is also imported by .github/scripts/check_recipe_pyproject.py, +# but that gate injects its own deps via `uv run --with` and does not read +# this group -- so it is declared here for the test run, not for that gate. dev = [ "pytest>=8", + "tomlkit>=0.13", + "ruamel.yaml>=0.18", + "packaging>=24", ] [build-system] diff --git a/uv.lock b/uv.lock index 3d45c36765..4ec7638a12 100644 --- a/uv.lock +++ b/uv.lock @@ -13,7 +13,10 @@ dependencies = [ [package.dev-dependencies] dev = [ + { name = "packaging" }, { name = "pytest" }, + { name = "ruamel-yaml" }, + { name = "tomlkit" }, ] [package.metadata] @@ -23,7 +26,12 @@ requires-dist = [ ] [package.metadata.requires-dev] -dev = [{ name = "pytest", specifier = ">=8" }] +dev = [ + { name = "packaging", specifier = ">=24" }, + { name = "pytest", specifier = ">=8" }, + { name = "ruamel-yaml", specifier = ">=0.18" }, + { name = "tomlkit", specifier = ">=0.13" }, +] [[package]] name = "attrs" @@ -314,6 +322,24 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/10/85/48f0abdcef5cce4e034c7a5b0ceeceba0b01bf0d942824f4bb720afe2dec/rpds_py-2026.6.3-pp311-pypy311_pp73-musllinux_1_2_x86_64.whl", hash = "sha256:8e65860d238379ed982fd9ba690579b5e95af2f4840f99c772816dbe573cb826", size = 586486, upload-time = "2026-06-30T07:17:51.141Z" }, ] +[[package]] +name = "ruamel-yaml" +version = "0.19.1" +source = { registry = "https://pypi.org/simple/" } +sdist = { url = "https://files.pythonhosted.org/packages/c7/3b/ebda527b56beb90cb7652cb1c7e4f91f48649fbcd8d2eb2fb6e77cd3329b/ruamel_yaml-0.19.1.tar.gz", hash = "sha256:53eb66cd27849eff968ebf8f0bf61f46cdac2da1d1f3576dd4ccee9b25c31993", size = 142709, upload-time = "2026-01-02T16:50:31.84Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/b8/0c/51f6841f1d84f404f92463fc2b1ba0da357ca1e3db6b7fbda26956c3b82a/ruamel_yaml-0.19.1-py3-none-any.whl", hash = "sha256:27592957fedf6e0b62f281e96effd28043345e0e66001f97683aa9a40c667c93", size = 118102, upload-time = "2026-01-02T16:50:29.201Z" }, +] + +[[package]] +name = "tomlkit" +version = "0.15.1" +source = { registry = "https://pypi.org/simple/" } +sdist = { url = "https://files.pythonhosted.org/packages/94/96/e07752635b98536177fa1f37671c8f3cdde2e724c6bcf6034b2cfb571565/tomlkit-0.15.1.tar.gz", hash = "sha256:e25bbf38843005246210a12982776f27f99cb9be67160e14434d0c0d21ee1e97", size = 180129, upload-time = "2026-07-17T01:48:04.562Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/13/bc/8c13eb66537dce1d2bd3a57132902f38d0e7f5bb46fa9f4daed9fe9d76ee/tomlkit-0.15.1-py3-none-any.whl", hash = "sha256:177a05aece5a8ca5266fd3c448abb47b8d352f09d477d3ca8332db4d89b24304", size = 49449, upload-time = "2026-07-17T01:48:05.728Z" }, +] + [[package]] name = "typing-extensions" version = "4.16.0"