diff --git a/.agents/skills/align-recipe-pyproject/SKILL.md b/.agents/skills/align-recipe-pyproject/SKILL.md index f77febf71a..6bf6b26bf8 100644 --- a/.agents/skills/align-recipe-pyproject/SKILL.md +++ b/.agents/skills/align-recipe-pyproject/SKILL.md @@ -33,7 +33,7 @@ Runs `scripts/align_pyproject.py` against a recipe directory. Six rules: | Rule ID | What it checks | Auto-fix | |---|---|---| | `no-local-ruff-config` | Recipe `pyproject.toml` must not declare any `[tool.ruff*]` table. Ruff config is centralized in the root `pyproject.toml`. | Yes — removes the tables. | -| `python-version-floor` | `[project].requires-python` must not permit any Python version below 3.11 (per `AGENTS.md`). A recipe that requires Python 3.12+ is the author's choice and is left alone. | Yes — raises the lower bound to `>=3.11` while preserving every upper bound, exclusion, compatible-release (`~=`) ceiling, and pin (only pure `>=`/`>` are dropped). If the result would be self-contradictory because the recipe's own ceiling/pin/exclusion excludes 3.11 (e.g. `>=3.10,!=3.11` or `==3.10.*`), refuses to apply and returns `needs_input`. | +| `python-version-floor` | `[project].requires-python` must **accept Python 3.11 exactly** — it must neither permit anything below (loose floors like `>=3.10`) nor exclude 3.11 by requiring higher (`>=3.12`, `~=3.12`, etc.). Per `AGENTS.md` "Minimum python version: 3.11" and CI in `.github/workflows/python-dependency-policy.yml`, which pins Python 3.11 and would otherwise emit a misleading "lockfile is out of date" error whose real cause is the interpreter mismatch. | Yes — rewrites the specifier so its lower bound is `>=3.11` while preserving every upper bound, exclusion, compatible-release (`~=`) ceiling, and pin (only pure `>=`/`>` are dropped or replaced). Applies to BOTH failure modes (loose floors AND higher-than-min floors). If the result would still exclude 3.11 (e.g. `>=3.10,!=3.11` → `>=3.11,!=3.11`, or `~=3.12` → `>=3.11,~=3.12` == `>=3.12,<4`), refuses to apply and returns `needs_input` for a human to resolve (typically: relax the ceiling, or raise the recipe with the maintainers to update CI's pinned interpreter). | | `project-name-matches-folder` | `[project].name` must equal the recipe folder basename. | Yes — sets it. | | `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. | diff --git a/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py b/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py index c7d2d74c71..7bc1f4ceb8 100644 --- a/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py +++ b/.agents/skills/align-recipe-pyproject/scripts/align_pyproject.py @@ -21,15 +21,23 @@ (Standalone ruff.toml / .ruff.toml files are also forbidden but are outside this skill's scope — see the workflow's Check 7.) - python-version-floor - [project].requires-python must not permit any Python version below - 3.11 (per AGENTS.md "Minimum python version: 3.11"). Recipes that - require Python 3.12+ are the author's choice and are left alone. - Auto-fix: raise the lower bound to >=3.11 while preserving every - upper bound, exclusion, compatible-release ceiling, and pin (only the - pure lower-bound operators >= and > are dropped). If the rewrite - would produce a self-contradictory result — because the recipe's own - ceiling/pin/exclusion excludes 3.11 (e.g. `>=3.10,!=3.11` or - `==3.10.*`) — refuse to apply and return NEEDS_INPUT. + [project].requires-python must ACCEPT Python 3.11 exactly — it must + neither permit anything below (loose floor like >=3.10) nor exclude + 3.11 by requiring higher (>=3.12 etc.). Per AGENTS.md "Minimum python + version: 3.11" and CI in .github/workflows/python-dependency-policy.yml, + which pins Python 3.11 and would otherwise emit a confusing "lockfile + is out of date" error whose real cause is the interpreter mismatch. + Auto-fix: rewrite the specifier so its lower bound is >=3.11 while + preserving every upper bound, exclusion, compatible-release ceiling, + and pin (only the pure lower-bound operators >= and > are dropped + or replaced). Applies to BOTH failure modes — loose floors (>=3.10) + and higher-than-min floors (>=3.12). If the rewrite would produce a + self-contradictory result — because the recipe's own ceiling/pin/ + exclusion still excludes 3.11 after lowering (e.g. `>=3.10,!=3.11`, + `==3.10.*`, or `~=3.12` where the compatible-release ceiling shuts + out 3.11) — refuse to apply and return NEEDS_INPUT for a human to + resolve (typically: relax the ceiling, or raise the recipe with + the maintainers to update CI's pinned interpreter). - project-name-matches-folder [project].name must equal the recipe folder basename. Auto-fix: set it. @@ -211,7 +219,7 @@ def _enumerate_ruff_subtables( return names -# ---------- python-version-floor: requires-python floor must be >= 3.11 ---- +# ---------- python-version-floor: requires-python must accept exactly 3.11 - def _add_missing_python_floor( @@ -239,14 +247,22 @@ def _validate_and_apply_python_floor_rewrite( spec: SpecifierSet, permits_older: list[Version], apply: bool, + excludes_min: bool = False, ) -> Check: - """Rewrite requires-python, validating the result is not degenerate.""" + """Rewrite requires-python, validating the result is not degenerate. + + Two failure modes route here: + * ``permits_older``: the spec accepts a Python below MIN_PYTHON. + * ``excludes_min``: the spec rejects MIN_PYTHON by requiring higher + (e.g. ``>=3.12``). The rewrite lowers the floor to MIN_PYTHON. + """ target = _rewrite_requires_python(spec) # If the mechanical result no longer admits MIN_PYTHON (e.g. - # `>=3.10,!=3.11` -> `>=3.11,!=3.11`), the recipe author's exclusion - # collides with our required floor. Refuse to apply rather than emit a - # self-contradictory specifier. + # `>=3.10,!=3.11` -> `>=3.11,!=3.11`, or `~=3.12` -> `>=3.11,~=3.12` + # which normalizes back to `>=3.12,<4`), the recipe author's exclusion + # or ceiling collides with our required floor. Refuse to apply rather + # than emit a self-contradictory or ineffective specifier. try: new_spec = SpecifierSet(target) except InvalidSpecifier as e: @@ -258,26 +274,41 @@ def _validate_and_apply_python_floor_rewrite( {"current": current, "attempted_rewrite": target}, ) if Version(MIN_PYTHON_STR) not in new_spec: + # Build a message that names the specific reason we're stuck. + if excludes_min and not permits_older: + explain = ( + f"the recipe's compatible-release ceiling or pin " + f"(e.g. ~=, ==) still shuts out {MIN_PYTHON_STR} after " + "lowering the pure lower bound" + ) + else: + explain = ( + "the recipe's own upper bound, pin, or exclusion " + "contradicts the required floor" + ) return Check( "python-version-floor", NEEDS_INPUT, - f"[project].requires-python = '{current}' permits Python " - f"versions below {MIN_PYTHON_STR}, but a mechanical rewrite " - f"would produce '{target}' which excludes {MIN_PYTHON_STR} " - f"itself (the recipe's own upper bound, pin, or exclusion " - f"contradicts the required floor). Fix by hand.", + f"[project].requires-python = '{current}' is incompatible " + f"with Python {MIN_PYTHON_STR}, but a mechanical rewrite " + f"would produce '{target}' which still excludes " + f"{MIN_PYTHON_STR} itself ({explain}). Fix by hand.", {"current": current, "attempted_rewrite": target}, ) - # Prefer a real witness version for the message; fall back to a generic - # phrase rather than surfacing a synthetic `.9999` probe (which would only - # appear for a micro-version floor like `>=3.10.5`). - real = [v for v in permits_older if v.micro != _PROBE_MICRO] - reason = ( - f"permits Python {real[0]}" - if real - else f"permits Python below {MIN_PYTHON_STR}" - ) + # Build the "reason this violated" phrase for the log message. + if excludes_min and not permits_older: + reason = f"excludes Python {MIN_PYTHON_STR}" + else: + # Prefer a real witness version; fall back to a generic phrase rather + # than surfacing a synthetic `.9999` probe (which would only appear + # for a micro-version floor like `>=3.10.5`). + real = [v for v in permits_older if v.micro != _PROBE_MICRO] + reason = ( + f"permits Python {real[0]}" + if real + else f"permits Python below {MIN_PYTHON_STR}" + ) verb = "Rewrote" if apply else "Would rewrite" if apply: project["requires-python"] = target @@ -293,14 +324,31 @@ def _validate_and_apply_python_floor_rewrite( def check_python_version_floor( pyproject_path: Path, doc: tomlkit.TOMLDocument, apply: bool ) -> Check: - """Ensure [project].requires-python does not permit versions < MIN_PYTHON. - - Interpretation A (per AGENTS.md discussion): the repo standard is a FLOOR. - A recipe that requires Python 3.12+ is fine and is left untouched. - A recipe that permits Python < 3.11 is rewritten so its lower bound - becomes >=3.11 (upper bounds and exclusions preserved). If the rewrite - would produce a self-contradictory result (e.g. `>=3.10,!=3.11`), the - script refuses to apply and returns NEEDS_INPUT. + """Ensure [project].requires-python is compatible with MIN_PYTHON. + + Interpretation B (aligned with CI in .github/workflows/ + python-dependency-policy.yml, which pins Python 3.11 and runs + `uv lock --check` against every recipe): every recipe MUST accept + MIN_PYTHON (3.11) as a valid interpreter. Two failure modes are rewritten: + + 1. **Permits versions below MIN_PYTHON** (e.g. `>=3.10`, `~=3.10`, + unpinned): floor raised to MIN_PYTHON while preserving upper + bounds/exclusions. + 2. **Excludes MIN_PYTHON by requiring higher** (e.g. `>=3.12`, + `>=3.12,<3.14`, `~=3.12`): floor lowered to MIN_PYTHON while + preserving upper bounds. Previously (Interpretation A) this was + treated as "author's choice, leave alone" — that stance conflicts + with CI, which hardcodes Python 3.11 and produces a confusing + "lockfile is out of date" error whose real cause is the interpreter + version mismatch. + + In either case, if the rewrite would produce a self-contradictory + specifier that excludes MIN_PYTHON itself (e.g. `>=3.10,!=3.11` or + `~=3.12` where the compatible-release ceiling still shuts out 3.11), + the script refuses to apply and returns NEEDS_INPUT for the human to + resolve. Recipes that genuinely need Python 3.12+ features must be + reported to the repo maintainer so CI can be updated in tandem — the + align skill will not silently allow a floor above MIN_PYTHON. """ project = doc.get("project") current = None if project is None else project.get("requires-python") @@ -324,20 +372,20 @@ def check_python_version_floor( ) permits_older = [v for v in BELOW_MIN if v in spec] + excludes_min = Version(MIN_PYTHON_STR) not in spec - # Interpretation A: only rewrite when the spec permits versions below - # MIN_PYTHON. Higher lower bounds (e.g. `>=3.12`) are the recipe author's - # deliberate choice and are left alone. - if not permits_older: + # OK only when both invariants hold: 3.11 is accepted AND nothing below + # 3.11 is accepted. + if not permits_older and not excludes_min: return Check( "python-version-floor", OK, - f"[project].requires-python lower bound is >= {MIN_PYTHON_STR} " - f"('{current}').", + f"[project].requires-python admits Python {MIN_PYTHON_STR} " + f"and rejects everything below it ('{current}').", ) return _validate_and_apply_python_floor_rewrite( - project, current, spec, permits_older, apply + project, current, spec, permits_older, apply, excludes_min ) @@ -720,9 +768,30 @@ def _find_pypi_entry(doc: tomlkit.TOMLDocument) -> Any: def _append_default_pypi_index(doc: tomlkit.TOMLDocument) -> None: """Append a `[[tool.uv.index]]` block declaring public PyPI as default. - Placed under `[tool.uv]` (created if absent). Existing non-default - `[[tool.uv.index]]` entries are preserved and remain higher-priority - (per uv's index ordering). + Two code paths depending on whether any `[[tool.uv.index]]` AoT already + exists: + + 1. AoT already present (recipe has index entries but none marked + `default=true`): append a new entry into the existing AoT via + tomlkit. Placement is unambiguous — the new entry sits with its + siblings, which is exactly what the user wants. + + 2. AoT NOT present (nothing under `[tool.uv.index]` at all): DO NOT let + tomlkit place the new block. tomlkit places a newly-created nested + AoT immediately after its parent's last child table, and that + position can fall INSIDE a trailing comment block that visually + introduces the NEXT top-level table. In real recipes with a + `[tool.uv.build-backend]` sub-table followed by comments introducing + `[tool.agent-starter-pack]`, tomlkit will wedge the new + `[[tool.uv.index]]` between those comments and their target table, + silently reassigning comment ownership. To avoid this, defer the + block to a raw string append after `tomlkit.dumps(doc)` — see + `_persist_changes`. The string always lands at the end of the file, + which is unambiguous and preserves every comment's semantic + attribution. + + Existing non-default `[[tool.uv.index]]` entries are preserved either + way and remain higher-priority (per uv's index ordering). No leading comment is emitted — tomlkit attaches comments passed to a table INSIDE the table (after its header), which reads awkwardly. @@ -732,19 +801,26 @@ def _append_default_pypi_index(doc: tomlkit.TOMLDocument) -> None: this auto-fix path just get the bare block. Both are equivalent functionally. """ - if "tool" not in doc: - doc["tool"] = tomlkit.table() - tool = doc["tool"] - if "uv" not in tool: - tool["uv"] = tomlkit.table() - uv = tool["uv"] - if "index" not in uv: - uv["index"] = tomlkit.aot() + tool = doc.get("tool") + uv = tool.get("uv") if tool is not None else None + existing_aot = uv.get("index") if uv is not None else None + + if existing_aot is not None: + # Path 1: append into the existing AoT (safe placement). + entry = tomlkit.table() + entry["url"] = "https://pypi.org/simple/" + entry["default"] = True + existing_aot.append(entry) + return - entry = tomlkit.table() - entry["url"] = "https://pypi.org/simple/" - entry["default"] = True - uv["index"].append(entry) + # Path 2: no existing AoT. Defer the append to post-serialization so + # tomlkit's placement heuristic can't wedge the block inside a trailing + # comment group. `_persist_changes` will pick this up. + doc._pending_pypi_index_append = ( + "\n[[tool.uv.index]]\n" + 'url = "https://pypi.org/simple/"\n' + "default = true\n" + ) def check_default_pypi_index(doc: tomlkit.TOMLDocument, apply: bool) -> Check: @@ -870,7 +946,18 @@ def _persist_changes( """ try: with open(pyproject_path, "w") as f: - f.write(tomlkit.dumps(doc)) + rendered = tomlkit.dumps(doc) + # `_append_default_pypi_index` stashes a raw block on the doc + # when it needs the [[tool.uv.index]] declaration to land at + # end-of-file rather than wherever tomlkit's placement heuristic + # would put a newly-created nested AoT — see the docstring + # there for the comment-adjacency bug this avoids. + pending = getattr(doc, "_pending_pypi_index_append", None) + if pending: + if not rendered.endswith("\n"): + rendered += "\n" + rendered += pending + f.write(rendered) except OSError as e: # Nothing has touched disk yet — the manifest write was deferred. report.add( diff --git a/.agents/skills/extract-python-environment-variables/SKILL.md b/.agents/skills/extract-python-environment-variables/SKILL.md index 2bf8b71ef1..0e342a7892 100644 --- a/.agents/skills/extract-python-environment-variables/SKILL.md +++ b/.agents/skills/extract-python-environment-variables/SKILL.md @@ -17,7 +17,9 @@ description: > bootstrap lines. Pre-existing `os.environ.setdefault(...)` or `os.getenv("VAR", "default")` calls that the recipe author wrote by hand are LEFT UNTOUCHED — the skill is additive-only for Python files - (adds `load_dotenv()` bootstrap; replaces hardcoded model literals). + (adds `load_dotenv()` bootstrap; replaces hardcoded model literals; + appends `# noqa: E402` to trailing relative imports in `__init__.py` + when they'd otherwise trip Ruff after an env-bootstrap block). Use when the user wants to "extract env vars", "update .env.example", "add load_dotenv", "replace hardcoded model names", or "fix environment variables" in a Python recipe. @@ -70,7 +72,28 @@ Runs `scripts/extract_env_vars.py` against a recipe directory. The script: load_dotenv() ``` - If `load_dotenv` is already present the file is left unchanged. + If `load_dotenv` is already present the injection is skipped. + + **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 + statement. Two cases this covers: + + - **Fresh injection.** The injected `load_dotenv()` call pushes + pre-existing trailing relative imports below a non-import statement, so + they'd trigger Ruff `E402` ("module-level import not at top of file") + when Phase 4 (ruff) of `prepare-python-recipe` runs. + + - **Author-written bootstrap.** The recipe author already wrote + `load_dotenv()` + `os.environ.setdefault(...)` calls followed by a + trailing `from . import agent`, but never marked the trailing import. + The skill did NOT inject anything (load_dotenv was already present) but + still adds the noqa suffix so the file is lint-clean on the pipeline's + next ruff pass. + + The suppression pass is precise — a relative import at the very TOP of + the file (before any non-import statement) is fine and left untouched. + 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("MODEL_NAME")`** — no default argument. 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 662ec80d69..0c49aad032 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 @@ -408,23 +408,70 @@ def _imports_os(tree: ast.AST) -> bool: return False -_RELATIVE_IMPORT_RE = re.compile(r"^\s*from\s+\.") _NOQA_E402_SUFFIX = " # noqa: E402 -- must come after load_dotenv()" -def _maybe_suppress_e402(line: str) -> str: - """Append a `# noqa: E402` suffix to a relative-import line if absent. +def _suppress_e402_on_late_relative_imports( # noqa: C901 + tree: ast.Module | None, lines: list[str] +) -> tuple[list[str], int]: + """Append `# noqa: E402` to top-level `from .x import y` statements that + sit AFTER a non-import module-level statement — those are the only ones + Ruff actually flags as E402. A relative import at the top of the module + (before any non-import) is fine and left alone. - A no-op for anything that isn't a relative import (``from .x import ...``) - or that already carries an E402 noqa comment. + Returns ``(new_lines, count_of_lines_newly_marked)``. Idempotent: a line + that already carries an E402 noqa comment is not touched again. """ - if not _RELATIVE_IMPORT_RE.match(line): - return line - if "noqa" in line and "E402" in line: - return line - stripped = line.rstrip("\n") - newline = line[len(stripped) :] # preserve original line ending - return stripped + _NOQA_E402_SUFFIX + newline + if tree is None: + return lines, 0 + + # 1. Find the line of the first non-import module-level statement. + # A module docstring (bare string expression at the top) is allowed + # to precede imports and does NOT count as a non-import. + first_non_import_line: int | None = None + for stmt in tree.body: + if isinstance(stmt, (ast.Import, ast.ImportFrom)): + continue + if ( + isinstance(stmt, ast.Expr) + and isinstance(stmt.value, ast.Constant) + and isinstance(stmt.value.value, str) + ): + continue + first_non_import_line = stmt.lineno + break + + if first_non_import_line is None: + return lines, 0 # No non-imports → no E402 possible. + + # 2. Collect line numbers of trailing relative imports (level > 0) that + # fall past that first-non-import boundary. + late_lines: set[int] = set() + for stmt in tree.body: + if ( + isinstance(stmt, ast.ImportFrom) + and stmt.level > 0 + and stmt.lineno > first_non_import_line + ): + for lineno in range( + stmt.lineno, (stmt.end_lineno or stmt.lineno) + 1 + ): + late_lines.add(lineno) + + if not late_lines: + return lines, 0 + + new_lines: list[str] = [] + marked = 0 + for i, line in enumerate(lines, start=1): + if i in late_lines and not ("noqa" in line and "E402" in line): + stripped = line.rstrip("\n") + newline = line[len(stripped) :] + new_lines.append(stripped + _NOQA_E402_SUFFIX + newline) + marked += 1 + else: + new_lines.append(line) + return new_lines, marked def _has_load_dotenv(tree: ast.AST) -> bool: @@ -493,13 +540,36 @@ def _last_top_level_absolute_import_line(tree: ast.Module) -> int: return last -def inject_load_dotenv(init_py: Path, dry_run: bool = False) -> bool: +def inject_load_dotenv( + init_py: Path, dry_run: bool = False +) -> tuple[bool, int]: """ - Ensure load_dotenv import + bootstrap snippet exist in __init__.py. - Returns True if the file was (or would be) modified. - - When dry_run is True, no file is written; the return value still reports - whether the snippet would be injected. + Ensure ``__init__.py`` bootstraps env-var loading correctly. + + Does two things, in order: + + 1. Injects the ``from dotenv import load_dotenv`` + ``load_dotenv()`` + bootstrap block if not already present. Insertion goes AFTER the + last top-level absolute import so env vars are populated before any + downstream package code runs. + + 2. Appends ``# noqa: E402`` to any trailing relative import + (``from .x import y``) that comes AFTER a non-import statement. + Runs BOTH when we just injected AND when the recipe author already + wrote a bootstrap (``load_dotenv()`` + ``os.environ.setdefault(...)``) + with trailing relative imports that were never marked — Ruff would + otherwise flag those as E402. The ordering is deliberate (env must + be populated before importing agent submodules), so suppression is + the correct fix, not reordering. + + Returns ``(injected, noqa_added)``: + * ``injected`` — True if the bootstrap block was added. + * ``noqa_added`` — Number of trailing relative import lines newly + marked with ``# noqa: E402``. + + Both operations are idempotent — calling again on the same file returns + ``(False, 0)``. When ``dry_run`` is True, the file is NOT written but + the return value still reports what would happen. """ content = init_py.read_text(encoding="utf-8") @@ -513,40 +583,53 @@ def inject_load_dotenv(init_py: Path, dry_run: bool = False) -> bool: else: # Fall back to a conservative substring check on unparseable files. already_present = "load_dotenv" in content - if already_present: - return False # Already present — nothing to do - - if dry_run: - return True # Would inject the bootstrap snippet. lines = content.splitlines(keepends=True) + injected = False + + if not already_present: + # Build the block to inject (import + blank line + snippet + blank line) + inject_block = f"\n{LOAD_DOTENV_IMPORT}\n\n{LOAD_DOTENV_SNIPPET}\n" + + # Insert AFTER the last top-level absolute import (AST-based, so imports + # inside docstrings/functions/conditionals are never mistaken for one). + # Relative imports (from .something) must come AFTER load_dotenv() so + # the env is populated before any package module-level code runs. + last_import_line = ( + _last_top_level_absolute_import_line(tree) + if tree is not None + else 0 + ) - # Build the block to inject (import + blank line + snippet + blank line) - inject_block = f"\n{LOAD_DOTENV_IMPORT}\n\n{LOAD_DOTENV_SNIPPET}\n" + if last_import_line > 0: + # end_lineno is 1-based; index == end_lineno inserts on the next + # line. + lines.insert(last_import_line, inject_block) + else: + # No top-level imports — insert after license header + docstring. + lines.insert(_post_header_index(lines), inject_block) + injected = True - # Insert AFTER the last top-level absolute import (AST-based, so imports - # inside docstrings/functions/conditionals are never mistaken for one). - # Relative imports (from .something) must come AFTER load_dotenv() so the - # env is populated before any package module-level code runs. - last_import_line = ( - _last_top_level_absolute_import_line(tree) if tree is not None else 0 - ) + # Re-split so each list element is exactly one physical line — the - if last_import_line > 0: - # end_lineno is 1-based; index == end_lineno inserts on the next line. - lines.insert(last_import_line, inject_block) - else: - # No top-level imports — insert after license header + docstring. - lines.insert(_post_header_index(lines), inject_block) + # spans multiple lines, so leaving it as one element would misalign + # indices with AST line numbers. AND re-parse so the AST reflects the + # post-injection state (line numbers of trailing imports shift). + joined = "".join(lines) + lines = joined.splitlines(keepends=True) + try: + tree = ast.parse(joined) + except SyntaxError: + tree = None - # Any relative imports (`from .x import ...`) now sit AFTER the injected - # load_dotenv() call, which would trigger Ruff E402 ("module-level import - # not at top of file"). Suppress that warning per-line, since the ordering - # is intentional — env must be populated before agent module-level code. - lines = [_maybe_suppress_e402(ln) for ln in lines] + # Suppress E402 on any trailing relative import that comes after a + # non-import statement — see docstring for the two cases this covers. + lines, noqa_added = _suppress_e402_on_late_relative_imports(tree, lines) - init_py.write_text("".join(lines), encoding="utf-8") - return True + if (injected or noqa_added > 0) and not dry_run: + init_py.write_text("".join(lines), encoding="utf-8") + + return injected, noqa_added # --------------------------------------------------------------------------- @@ -989,7 +1072,10 @@ def run_step_env_vars( def run_step_load_dotenv(recipe_dir: Path, dry_run: bool = False) -> None: - """Step 4: inject load_dotenv() bootstrap into the package __init__.py.""" + """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).""" init_py = find_package_init(recipe_dir) if not init_py: print( @@ -998,11 +1084,21 @@ def run_step_load_dotenv(recipe_dir: Path, dry_run: bool = False) -> None: ) return rel = init_py.relative_to(recipe_dir) - if inject_load_dotenv(init_py, dry_run=dry_run): + injected, noqa_added = inject_load_dotenv(init_py, dry_run=dry_run) + if injected: verb = "Would inject" if dry_run else "Injected" print(f"[{_tag(dry_run)}] {verb} load_dotenv() bootstrap into {rel}") else: print(f"[PASS] load_dotenv() already present in {rel} — skipped.") + if noqa_added > 0: + verb = "Would add" if dry_run else "Added" + print( + f"[{_tag(dry_run)}] {verb} '# noqa: E402' to {noqa_added} " + f"trailing relative import(s) in {rel} — they come after " + "load_dotenv()/env-bootstrap and would otherwise trigger Ruff " + "E402 in Phase 4 (ordering is intentional: env must be " + "populated before importing agent submodules)." + ) def run_step_pyproject(recipe_dir: Path, dry_run: bool = False) -> None: 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 723993e2ef..79f7f5a1fb 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 @@ -320,7 +320,8 @@ def test_inject_load_dotenv_noop_if_present(tmp_path): ) before = init.read_text(encoding="utf-8") - assert m.inject_load_dotenv(init) is False + # Nothing to inject and no trailing relative imports → (False, 0). + assert m.inject_load_dotenv(init) == (False, 0) assert init.read_text(encoding="utf-8") == before @@ -330,7 +331,11 @@ def test_inject_load_dotenv_after_absolute_import_before_relative(tmp_path): '"""Package."""\n\nimport os\n\nfrom .agent import root_agent\n', ) - assert m.inject_load_dotenv(init) is True + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is True + # The trailing `from .agent` shifts below the injected load_dotenv() call, + # so it also picks up a noqa: E402 marker. + assert noqa_added == 1 content = init.read_text(encoding="utf-8") ast.parse(content) # result must be valid Python @@ -344,7 +349,9 @@ def test_inject_load_dotenv_after_absolute_import_before_relative(tmp_path): def test_inject_load_dotenv_no_imports_goes_after_docstring(tmp_path): init = _write(tmp_path / "__init__.py", '"""Package docstring."""\n') - assert m.inject_load_dotenv(init) is True + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is True + assert noqa_added == 0 # no trailing relative imports at all content = init.read_text(encoding="utf-8") ast.parse(content) # result must be valid Python @@ -362,7 +369,9 @@ def test_inject_load_dotenv_adds_noqa_to_trailing_relative_imports(tmp_path): "import os\nfrom .agent import root_agent\n", ) - assert m.inject_load_dotenv(init) is True + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is True + assert noqa_added == 1 content = init.read_text(encoding="utf-8") ast.parse(content) # result must be valid Python @@ -371,7 +380,7 @@ def test_inject_load_dotenv_adds_noqa_to_trailing_relative_imports(tmp_path): ) assert "noqa: E402" in rel_line # Idempotent: a second run must not duplicate the suffix. - m.inject_load_dotenv(init) + assert m.inject_load_dotenv(init) == (False, 0) assert init.read_text(encoding="utf-8").count("noqa: E402") == 1 @@ -383,7 +392,9 @@ def test_inject_load_dotenv_ignores_docstring_mention(tmp_path): '"""We will load_dotenv somewhere."""\nimport os\n', ) - assert m.inject_load_dotenv(init) is True + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is True + assert noqa_added == 0 # no trailing relative imports content = init.read_text(encoding="utf-8") ast.parse(content) # result must be valid Python @@ -395,7 +406,9 @@ def test_inject_load_dotenv_dry_run_reports_but_does_not_write(tmp_path): init = _write(tmp_path / "__init__.py", original) # Reports that it would inject ... - assert m.inject_load_dotenv(init, dry_run=True) is True + injected, noqa_added = m.inject_load_dotenv(init, dry_run=True) + assert injected is True + assert noqa_added == 0 # ... but leaves the file untouched. assert init.read_text(encoding="utf-8") == original @@ -415,7 +428,11 @@ def test_inject_load_dotenv_not_placed_inside_docstring(tmp_path): "from .agent import root_agent\n", ) - assert m.inject_load_dotenv(init) is True + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is True + # The pre-existing `from .agent import root_agent` shifts below the + # injected load_dotenv() call, so it picks up a noqa: E402 marker. + assert noqa_added == 1 content = init.read_text(encoding="utf-8") tree = ast.parse(content) # (b) result is valid Python @@ -443,7 +460,9 @@ def test_inject_load_dotenv_ignores_import_inside_conditional(tmp_path): "import os\nif True:\n import pdb\n x = 1\n", ) - assert m.inject_load_dotenv(init) is True + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is True + assert noqa_added == 0 # no trailing relative imports content = init.read_text(encoding="utf-8") ast.parse(content) # must remain valid Python (no IndentationError) @@ -452,6 +471,97 @@ def test_inject_load_dotenv_ignores_import_inside_conditional(tmp_path): assert content.index("load_dotenv") < content.index("if True:") +def test_inject_load_dotenv_marks_late_relative_import_when_already_bootstrapped( + tmp_path, +): + # Regression: some recipe authors hand-write the env-bootstrap pattern + # (load_dotenv + os.environ.setdefault + trailing `from .agent`) without + # a noqa marker on the relative import. When we run against such a file + # we must NOT inject anything (load_dotenv is already there) but MUST + # still add `# noqa: E402` to the trailing relative import — otherwise + # ruff (Phase 4 in prepare-python-recipe) flags E402 for a pattern the + # skill is aware of and could have suppressed. + init = _write( + tmp_path / "__init__.py", + "import os\n" + "from dotenv import load_dotenv\n" + "\n" + "load_dotenv()\n" + "os.environ.setdefault('FOO', 'bar')\n" + "\n" + "from . import agent\n", + ) + + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is False # load_dotenv was already present + assert noqa_added == 1 # ... but the trailing relative import was marked + + content = init.read_text(encoding="utf-8") + rel_line = next( + ln for ln in content.splitlines() if ln.startswith("from . import") + ) + assert "noqa: E402" in rel_line + + # Idempotent on a second run — nothing more to do. + assert m.inject_load_dotenv(init) == (False, 0) + assert init.read_text(encoding="utf-8").count("noqa: E402") == 1 + + +def test_inject_load_dotenv_leaves_early_relative_import_alone(tmp_path): + # Precision check: a relative import at the TOP of __init__.py (before + # any non-import statement) does NOT trigger Ruff E402. The suppression + # pass must be precise — the previous implementation blindly marked + # every relative import, which produced meaningless noqa comments on + # perfectly-fine lines. + init = _write( + tmp_path / "__init__.py", + "from . import agent\n" + "from dotenv import load_dotenv\n" + "\n" + "load_dotenv()\n", + ) + + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is False + assert noqa_added == 0 + + content = init.read_text(encoding="utf-8") + rel_line = next( + ln for ln in content.splitlines() if ln.startswith("from . import") + ) + assert "noqa" not in rel_line + + +def test_inject_load_dotenv_ignores_docstring_before_late_relative_import( + tmp_path, +): + # A module docstring at the top of the file must NOT count as "the first + # non-import statement" — otherwise every relative import in a module with + # a docstring would be treated as late and get spuriously marked. + init = _write( + tmp_path / "__init__.py", + '"""My package."""\n' + "\n" + "from dotenv import load_dotenv\n" + "\n" + "load_dotenv()\n" + "\n" + "from . import agent\n", + ) + + injected, noqa_added = m.inject_load_dotenv(init) + assert injected is False # already bootstrapped + # The trailing `from . import agent` DOES come after load_dotenv() (not + # after the docstring), so it IS late and should be marked. + assert noqa_added == 1 + + content = init.read_text(encoding="utf-8") + rel_line = next( + ln for ln in content.splitlines() if ln.startswith("from . import") + ) + assert "noqa: E402" in rel_line + + # --------------------------------------------------------------------------- # ensure_python_dotenv_dependency # --------------------------------------------------------------------------- diff --git a/.agents/skills/prepare-python-recipe/SKILL.md b/.agents/skills/prepare-python-recipe/SKILL.md index 43484d4a82..9bfab6fbb6 100644 --- a/.agents/skills/prepare-python-recipe/SKILL.md +++ b/.agents/skills/prepare-python-recipe/SKILL.md @@ -14,9 +14,9 @@ description: > renamed if needed). Delegates to the existing sub-skills (generate-manifest, extract-python-environment-variables, align-recipe-pyproject, generate-python-runnability-test) so the master - never duplicates their logic. Pauses at fixed decision points (manifest - team/POC verification, description mismatch, existing test regeneration) - AND is free to interrupt for clarification any time a phase's output + never duplicates their logic. Pauses at fixed decision points + (description mismatch, existing test regeneration) AND is free to + interrupt for clarification any time a phase's output looks ambiguous, unexpected, or would benefit from a human judgment call — this is an interactive skill by design. Use when the user wants to "prepare a recipe", "update a recipe end to end", "run all the @@ -31,7 +31,20 @@ metadata: Master orchestration skill. Runs the other Python-recipe skills in the right order, with the right inputs, in a single pipeline. Use when the user wants a recipe brought fully up to standard in one go. -**This is an interactive skill.** It's expected to pause and ask questions when doing so genuinely improves the outcome — not just at the four fixed checkpoints below, but any time a phase's output is ambiguous, surprising, or would benefit from a judgment call. See rule 5 (fixed checkpoints) and rule 6 (judgment-based interruptions) for the difference. +**This is an interactive skill.** It's expected to pause and ask questions when doing so genuinely improves the outcome — not just at the fixed checkpoints below, but any time a phase's output is ambiguous, surprising, or would benefit from a judgment call. See rule 5 (fixed checkpoints) and rule 6 (judgment-based interruptions) for the difference. + +--- + +## Canonical placeholder strings + +Two ownership placeholders are written by `generate-manifest` and enforced by `tools/validate_manifest.py`. They must be the EXACT strings below — never invent, translate, or rephrase them: + +``` +OWNERSHIP_TEAM_PLACEHOLDER = "TODO: Replace with your team name" +OWNERSHIP_POC_PLACEHOLDER = "TODO: Replace with your GitHub user ID" +``` + +`generate-manifest` is the single source of truth for these values. This skill NEVER replaces them mid-pipeline — they are intentionally left in place so CI validation fails until a human fills them in. Replacing them lives in the user's post-pipeline TODO list (see the summary's "What you still need to do" section). --- @@ -53,7 +66,7 @@ If the user has NOT done these and asks you to run the skill anyway, tell them t 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: -1. **Manifest** — generate `manifest.yaml` if missing; ask the user to verify `ownership.team` and `ownership.poc`. +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). 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. @@ -76,7 +89,6 @@ At the end, print a summary table and remind the user to `git diff` and commit 4. **Exception for pure-instructions skills**: `generate-manifest` has no script — it's a pure-instructions skill. For that one only, load its SKILL.md (via the `skill` tool) and follow it inline. 5. **Fixed checkpoints — always pause here**: - - **Manifest team/POC verification** — after Phase 1, `manifest.yaml` will contain the placeholders `"TODO: Replace with your team name"` and `"TODO: Replace with your GitHub user ID"`. Show them and ask for real values. (These exact strings are the canonical placeholders that `generate-manifest` writes AND that `tools/validate_manifest.py` enforces in CI — the three must stay in sync; if the validator's strings ever change, update this checkpoint, Phase 1c, and generate-manifest together.) - **Description mismatch** — if Phase 3 returns `needs_input` for `description-matches-manifest`, show both sides and ask the user to pick `pyproject`, `manifest`, or `delete`. - **Test file exists** — before Phase 6, if `tests/test_runnability.py` already exists, ask whether to regenerate (default: keep existing). Regeneration uses `--overwrite`. - **Entry point not found (Phase 6)** — if the runnability-test generator errors because no `agent.py` was found, surface the message and offer to re-run with `--agent-file ` once the user says where the entry point is. This is the one `error` case with a defined recovery instead of a hard stop. @@ -122,7 +134,7 @@ If the user has not specified the recipe directory, ask for it before proceeding ### Phase 0 — plan + confirm (do this first, always) -**First, verify the recipe directory actually exists** — a path typo shouldn't cost the user the whole plan-confirmation round-trip only to fail in Phase 1: +**Step 0a — Verify the recipe directory actually exists.** A path typo shouldn't cost the user the whole plan-confirmation round-trip only to fail in Phase 1: ```bash [ -d ] || { echo "Recipe directory not found: "; exit 1; } @@ -130,15 +142,29 @@ 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. -Then, before running anything, quickly confirm the prerequisites and show the user the plan: +**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. + +```bash +MAX_LEN=$(uv run --no-project --with pyyaml python3 .github/scripts/load_policy.py recipe_naming.max_folder_name_length) +uv run --no-project python3 .agents/skills/prepare-python-recipe/scripts/check_folder_name.py \ + --recipe-dir --max-length "$MAX_LEN" +``` + +The check exits 0 silently on a compliant name; on violation it exits 1 with the specific offending characters, the length overrun (if any), and a suggested compliant name derived from the current one (lowercase, `_` → `-`, drop disallowed characters, truncate on a hyphen boundary). The suggestion is ADVISORY — the script never renames anything. + +**If it fails, HALT the pipeline before Phase 1.** Print the script's stderr verbatim (it already includes the suggestion and the manual `git mv` command). Do NOT show the plan, do NOT prompt to proceed, do NOT ask "want me to rename?" — renaming a recipe directory is the user's decision, not the skill's. They rename by hand and re-invoke the skill. + +**Only proceed past this step if the folder-name check passed.** -> Prerequisites (I'll assume these are done — tell me if not): +**Step 0c — Show the plan and get confirmation.** Before running anything, briefly 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: + +> A few things I'm assuming — say so if any aren't true: > - You've deactivated any active venv. > - 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: -> 1. Generate manifest.yaml (if missing; verify team + POC) +> 1. Generate manifest.yaml (if missing) > 2. Extract env vars into .env.example > 3. Align pyproject.toml > 4. Ruff format + check --fix @@ -158,13 +184,11 @@ Get a yes-or-no. If no, stop. [ -f /manifest.yaml ] && echo exists || echo missing ``` -**1b. If missing** — load the `generate-manifest` skill (via the `skill` tool with `name="generate-manifest"`) and follow its instructions for this recipe. That skill writes `manifest.yaml` with placeholders for team/POC. - -**1b. If exists** — skip generation. Note it in the summary. +**1b. If missing** — load the `generate-manifest` skill (via the `skill` tool with `name="generate-manifest"`) and follow its instructions for this recipe. That skill writes `manifest.yaml` with the canonical ownership placeholders — LEAVE THEM AS-IS. -**1c. Verify team/POC** — regardless of whether we generated fresh or the file already existed, read `manifest.yaml` and locate `ownership.team` and `ownership.poc`. If either equals a placeholder value (`"TODO: Replace with your team name"` or `"TODO: Replace with your GitHub user ID"`), pause and ask the user for real values. When they answer, edit `manifest.yaml` in place (use the `edit` tool). If both are already filled in, do not ask. +**1b. If exists** — skip generation. Do NOT read `ownership.team` / `ownership.poc` and do NOT prompt about them. Whatever they are (real values or the canonical placeholders), leave them untouched — the user handles ownership post-pipeline. -Progress line: `Phase 1 (manifest): generated | pre-existing; team=, poc=.` +Progress line: `Phase 1 (manifest): generated | pre-existing.` ### Phase 2 — extract env vars @@ -230,6 +254,8 @@ uv run ruff check --fix - **Exit 1** — genuine violations ruff can't auto-fix (typically `C901` complex-structure, `PLR0912/0915` too-many-branches/statements). Do NOT stop the pipeline — note them in the summary as a Manual TODO so the user can refactor or add per-file `# noqa` markers after review. - **Exit 2** — ruff itself errored (invalid config, a file-system problem, or a bug in ruff), NOT a violation count. Phase 4 effectively did not run, so treat this as a hard error under rule 7: stop the pipeline and surface the message. Do not mistake it for "violations remain" and continue. +Note on `E402` and `__init__.py`: Phase 2 (env-var extraction) already suppresses `E402` on any trailing relative import (`from . import agent`) in the recipe's package `__init__.py` — the canonical ADK-recipe pattern where env-bootstrap side effects (`load_dotenv()`, `os.environ.setdefault(...)`) intentionally precede a `from . import ...` line so env vars are populated before agent submodules load. If you see `E402` on an `__init__.py` in Phase 4's output, something went wrong upstream (Phase 2 didn't detect the pattern, or a new file appeared between Phases 2 and 4). Note that in the progress line so it isn't quietly buried. + Progress line: `Phase 4 (lint): file(s) formatted, issue(s) auto-fixed, unfixable issue(s) left.` ### Phase 5 — recipe `uv lock` @@ -237,11 +263,13 @@ Progress line: `Phase 4 (lint): file(s) formatted, issue(s) auto-fixed, Now that `pyproject.toml` is stable (Phase 3 aligned it, Phase 4 didn't touch it), regenerate the lockfile so it matches: ```bash -uv lock +uv lock --python 3.11 ``` Run this WITH `workdir = ` (do not `cd` — pass the working directory via the tool call). +**Why `--python 3.11` explicitly?** CI's `.github/workflows/python-dependency-policy.yml` pins Python 3.11 when it runs `uv lock --check` on every recipe. If we lock here with whatever interpreter the user happens to have installed (typically 3.12 or newer on modern machines), the recipe locks cleanly locally but the CI check fails on the PR with a confusing `The requested interpreter resolved to Python 3.11.15, which is incompatible with the project's Python requirement: >=3.12` — mis-reported by the workflow as "lockfile is out of date". Forcing 3.11 here surfaces the same incompatibility at pipeline time, when the user can fix it or push back, rather than at PR time. Phase 3's `python-version-floor` check should have already caught this and rewritten `requires-python`, so the lock should succeed — but pinning defends against edge cases where the check was too permissive or the recipe had a compatible-release ceiling the rewrite couldn't lower. + **Why `uv lock` and not `uv sync`?** The pipeline's job is to prepare the recipe, not to install and validate its runtime environment. `uv lock` resolves dependencies against the aligned `pyproject.toml` and writes `uv.lock` — that's the artefact CI and downstream consumers need. `uv sync` would additionally download every wheel into `.venv/`, which: - Is slow (minutes of network I/O). - Sets up a dev environment the user may or may not want. @@ -249,9 +277,9 @@ Run this WITH `workdir = ` (do not `cd` — pass the working directo The user gets a real `.venv/` by running `uv sync` themselves after reviewing the diff — see the "Next steps" block at the end of the summary. -If `uv lock` fails (dependency conflict, unresolvable version, invalid `pyproject.toml`), stop and surface the error. +If `uv lock --python 3.11` fails, halt and surface the error verbatim (rule 7). The most common cause is a `requires-python` specifier that excludes 3.11 (`>=3.12`, `~=3.12`, etc.) that Phase 3 could not rewrite — the fix is to either lower the recipe's floor to `>=3.11` or, if the recipe genuinely needs newer features, raise the issue with the repo maintainers to update CI's pinned interpreter. -Progress line: `Phase 5 (recipe lock): uv lock completed.` +Progress line: `Phase 5 (recipe lock): uv lock --python 3.11 completed.` ### Phase 6 — runnability test @@ -318,7 +346,7 @@ At the end, print the sections below in order. Section 3 is **conditional** — | Phase | Outcome | Notes | |---|---|---| -| 1. Manifest | ok | generated; team=, poc= | +| 1. Manifest | ok | generated (ownership placeholders left for user to fill in) | | 2. Env vars | ok | 3 added, load_dotenv injected, python-dotenv added | | 3. Align | ok | 2 fixes applied | | 4. Lint | ok | 12 files formatted, 4 issues auto-fixed | @@ -369,7 +397,7 @@ A single short TODO list. Keep every entry to one line. Standard items come firs **Standard — always include (skip only if genuinely N/A):** -1. **Verify manifest** — open `/manifest.yaml` and confirm `ownership.team` and `ownership.poc` are correct. Omit this item if the user supplied real values at the Phase 1c prompt during this run — they just confirmed them, so re-listing it is noise. Keep it when the manifest pre-existed with values the pipeline never asked the user about. +1. **Fill in ownership** — open `/manifest.yaml` and replace `ownership.team` (`"TODO: Replace with your team name"`) and `ownership.poc` (`"TODO: Replace with your GitHub user ID"`) with real values. CI validation intentionally fails until you do. If the manifest pre-existed and already had real values, this is a no-op — glance to confirm. 2. **Fill in `.env.example`** — replace each `` placeholder with the real value (or delete the line if the variable isn't used). 3. **Review the diff** — `cd && git diff` — inspect every change the pipeline made before committing. diff --git a/.agents/skills/prepare-python-recipe/scripts/check_folder_name.py b/.agents/skills/prepare-python-recipe/scripts/check_folder_name.py new file mode 100644 index 0000000000..99052474c6 --- /dev/null +++ b/.agents/skills/prepare-python-recipe/scripts/check_folder_name.py @@ -0,0 +1,199 @@ +# 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. +""" +Guardrail for prepare-python-recipe Phase 0: verify the recipe's folder +name matches the two rules that .github/workflows/python-validate-recipe.yml +enforces so the pipeline halts LOCALLY on a bad name instead of running to +completion and shipping files under a path CI will reject. + +Rules enforced (mirrored from python-validate-recipe.yml, Check 1): + 1. Regex: ^[a-z][a-z-]*$ — lowercase letters and hyphens only, must + start with a letter. Rejects underscores, uppercase, digits, and + symbols. + 2. Length: <= --max-length characters (default 30, source of truth: + .github/policy.yml `recipe_naming.max_folder_name_length`). The + caller is expected to look up the live value and pass it in; the + default is a safety net for standalone use only. + +On violation, prints one line per broken rule PLUS a suggested compliant +name derived from the current one (lowercase, `_` → `-`, drop other +characters, collapse `--`, strip trailing `-`, truncate at hyphen +boundary when possible so we don't cut mid-word). The suggestion is +ADVISORY — the script never renames anything on disk. Exits 1. + +On pass, prints one PASS line and exits 0. + +Usage: + python check_folder_name.py --recipe-dir [--max-length N] + +Stdlib-only so it can be invoked with `uv run --no-project python3` and +never pulls in transitive deps. If the caller wants to source the max +length from `.github/policy.yml` at runtime, they should call +`.github/scripts/load_policy.py recipe_naming.max_folder_name_length` +and pass the result via --max-length — that keeps this script decoupled +from the policy file's format. +""" + +from __future__ import annotations + +import argparse +import re +import sys +from pathlib import Path + +VALID_NAME_RE = re.compile(r"^[a-z][a-z-]*$") +DEFAULT_MAX_LENGTH = 30 + + +def suggest_compliant_name(name: str, max_len: int) -> str: + """Derive a name that satisfies both rules from a possibly-bad input. + + Transformations, in order: + 1. Lowercase everything. + 2. Replace ``_`` with ``-`` (the most common ADK-recipe author habit). + 3. Drop any character that isn't in ``[a-z-]`` (digits, punctuation, + accented letters, whitespace, etc.). + 4. Collapse runs of ``-`` into a single ``-``. + 5. Strip leading/trailing ``-``. + 6. If longer than ``max_len``, truncate — preferring to cut at a + hyphen boundary within the second half of the string so the + suggestion doesn't end mid-word ("airflow-version-upg" is + uglier than "airflow-version" for the same violation). + 7. Strip any trailing ``-`` left over from truncation. + + Returns the suggested name, or "" when no salvageable suggestion exists + (empty after transformations, or starts with a non-letter and there is + no letter anywhere to promote to the front). Callers should render the + empty-string case as "no automatic suggestion; please choose a name + manually." + """ + normalized = name.lower().replace("_", "-") + filtered = "".join(c for c in normalized if c == "-" or "a" <= c <= "z") + while "--" in filtered: + filtered = filtered.replace("--", "-") + filtered = filtered.strip("-") + + if len(filtered) > max_len: + truncated = filtered[:max_len] + last_hyphen = truncated.rfind("-") + # Only cut at hyphen if it doesn't discard more than half of what + # fit — otherwise the mid-word cut is closer to the user's intent. + if last_hyphen >= max_len // 2: + truncated = truncated[:last_hyphen] + filtered = truncated.rstrip("-") + + if not filtered or not ("a" <= filtered[0] <= "z"): + return "" + return filtered + + +def check(folder_name: str, max_len: int) -> list[str]: + """Return a list of violation descriptions; empty when the name is OK.""" + violations: list[str] = [] + if not VALID_NAME_RE.match(folder_name): + # Enumerate the concrete offending characters/positions so the + # user sees exactly what to fix, not just "bad regex". + offenders = sorted( + {c for c in folder_name if not (c == "-" or "a" <= c <= "z")} + ) + detail_bits: list[str] = [] + if folder_name and not ("a" <= folder_name[0] <= "z"): + detail_bits.append( + f"starts with '{folder_name[0]}' (must start with a lowercase letter)" + ) + if offenders: + offender_str = ", ".join(f"'{c}'" for c in offenders) + detail_bits.append(f"disallowed characters: {offender_str}") + detail = ( + "; ".join(detail_bits) + if detail_bits + else "does not match ^[a-z][a-z-]*$" + ) + violations.append( + f"Folder name '{folder_name}' fails regex ^[a-z][a-z-]*$ — {detail}." + ) + if len(folder_name) > max_len: + violations.append( + f"Folder name '{folder_name}' is {len(folder_name)} characters, " + f"exceeds max {max_len} (source: .github/policy.yml " + "recipe_naming.max_folder_name_length)." + ) + return violations + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Verify a recipe folder name matches CI's rules." + ) + parser.add_argument( + "--recipe-dir", + required=True, + help="Path to the recipe root; the folder basename is what gets checked.", + ) + parser.add_argument( + "--max-length", + type=int, + default=DEFAULT_MAX_LENGTH, + help=( + f"Max allowed name length (default {DEFAULT_MAX_LENGTH}). Live " + "value lives in .github/policy.yml under " + "recipe_naming.max_folder_name_length — pass it explicitly to " + "stay in sync with CI." + ), + ) + args = parser.parse_args(argv) + + recipe_path = Path(args.recipe_dir) + folder_name = recipe_path.name + + violations = check(folder_name, args.max_length) + + if not violations: + print( + f"[PASS] Folder name '{folder_name}' is compliant " + f"(matches ^[a-z][a-z-]*$, length {len(folder_name)} <= " + f"{args.max_length})." + ) + return 0 + + print("[FAIL] Recipe folder name violates the CI naming rules.\n") + for v in violations: + print(f" - {v}") + + suggestion = suggest_compliant_name(folder_name, args.max_length) + print() + if suggestion: + print( + f"Suggested compliant name: '{suggestion}'\n" + f" (derived from '{folder_name}' by lowercasing, replacing '_' " + "with '-', dropping disallowed characters, and truncating on a " + "hyphen boundary — advisory only, tweak as you like.)" + ) + else: + print( + "No automatic suggestion — please choose a name that matches " + f"^[a-z][a-z-]*$ and is at most {args.max_length} chars long." + ) + + print( + "\nRename the directory manually and re-run the pipeline. The " + "prepare-python-recipe skill NEVER renames recipe directories on " + "its own — that's your call. Example:\n" + f" git mv '{recipe_path}' '{recipe_path.parent / (suggestion or 'YOUR-CHOSEN-NAME')}'" + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/check_recipe_pyproject.py b/.github/scripts/check_recipe_pyproject.py index be462a55f1..37cbc313d3 100644 --- a/.github/scripts/check_recipe_pyproject.py +++ b/.github/scripts/check_recipe_pyproject.py @@ -5,8 +5,10 @@ - project-name-matches-folder: [project].name must equal the recipe folder basename. - - python-version-floor: [project].requires-python must not permit any - Python version below 3.11 (per AGENTS.md). + - python-version-floor: [project].requires-python must ACCEPT Python + 3.11 exactly — it must neither permit anything below (loose floor) nor + exclude 3.11 by requiring higher (e.g. `>=3.12`). CI pins Python 3.11 + and any recipe that can't lock under 3.11 breaks the lockfile check. - description-matches-manifest: if [project].description is set, it must equal manifest.description from the recipe's manifest.yaml (after .strip(), exact match). Optional; skipped when absent. @@ -113,12 +115,23 @@ def check_name(project: dict, pyproject_path: Path, folder: str) -> None: def check_requires_python(project: dict, pyproject_path: Path) -> None: - """B1: [project].requires-python must not permit Python < MIN_PYTHON. - - Interpretation A: the repo standard is a FLOOR. A recipe that requires - Python 3.12+ (e.g. `>=3.12`) is fine — the recipe author has legitimately - chosen a stricter minimum. A recipe that PERMITS versions below 3.11 - (e.g. `>=3.10`, `~=3.10`, `!=3.11`, `<=3.12`, unpinned) is a violation. + """B1: [project].requires-python must ACCEPT Python == MIN_PYTHON. + + Interpretation B: every recipe must be lockable/runnable under the + version CI pins in .github/workflows/python-dependency-policy.yml (3.11). + Two failure modes: + + * ``permits_older`` — spec accepts a Python below MIN_PYTHON (e.g. + ``>=3.10``, ``~=3.10``, ``!=3.11``, ``<=3.12``, unpinned). + * ``excludes_min`` — spec rejects MIN_PYTHON by requiring higher + (e.g. ``>=3.12``, ``>=3.12,<3.14``, ``~=3.12``). This used to be + permitted under a "floor is 3.11 but higher is fine" reading, but + it makes CI fail with a confusing "lockfile is out of date" error + whose real cause is that uv can't resolve a 3.11 interpreter + against a >=3.12 requirement. Recipes that genuinely need 3.12+ + features must instead update CI's pinned interpreter (or add a + per-recipe override) — silently allowing them here just moves the + failure to PR-time. Uses packaging.specifiers.SpecifierSet (the PEP 440 reference implementation) so every legal operator (>=, >, ~=, ==, !=, <, <=, and @@ -130,7 +143,8 @@ def check_requires_python(project: dict, pyproject_path: Path) -> None: "FAIL", pyproject_path, f"[project].requires-python is missing; it must declare a " - f"lower bound of >= {MIN_PYTHON_STR} (per AGENTS.md).", + f"specifier that accepts Python {MIN_PYTHON_STR} and rejects " + "anything below (per AGENTS.md).", ) return @@ -145,9 +159,9 @@ def check_requires_python(project: dict, pyproject_path: Path) -> None: ) return - # If any pre-MIN_PYTHON version satisfies the spec, the lower bound is - # too loose (e.g. '>=3.10', '~=3.10', '!=3.11', '<=3.12', unpinned). permits_older = [v for v in BELOW_MIN if v in spec] + excludes_min = Version(MIN_PYTHON_STR) not in spec + if permits_older: # Only surface a real witness version, never a synthetic `.9999` # probe (which is the only match for a micro floor like `>=3.10.5`). @@ -162,10 +176,23 @@ def check_requires_python(project: dict, pyproject_path: Path) -> None: ) return + if excludes_min: + emit( + "FAIL", + pyproject_path, + f"[project].requires-python = '{requires_python}' excludes Python " + f"{MIN_PYTHON_STR}; the specifier must accept {MIN_PYTHON_STR} so " + "CI (which pins Python 3.11) can lock and run this recipe. Lower " + f"the floor to '>={MIN_PYTHON_STR}' (preserving any upper bound), " + "or if the recipe genuinely needs newer features, raise the " + "issue with the repo maintainers to update CI's pinned interpreter.", + ) + return + emit( "PASS", pyproject_path, - f"[project].requires-python lower bound is >= {MIN_PYTHON_STR} " + f"[project].requires-python admits Python {MIN_PYTHON_STR} " f"('{requires_python}').", ) diff --git a/.github/workflows/_ai-pr-review-core.yml b/.github/workflows/_ai-pr-review-core.yml index ee55b39004..00af9ab0fa 100644 --- a/.github/workflows/_ai-pr-review-core.yml +++ b/.github/workflows/_ai-pr-review-core.yml @@ -154,14 +154,22 @@ jobs: echo "pr_number=${PR_NUMBER}" >> "${GITHUB_OUTPUT}" echo "additional_instructions=" >> "${GITHUB_OUTPUT}" - PR_DATA="$(gh pr view "${PR_NUMBER}" --json title,additions,deletions,changedFiles,baseRefName,headRefName)" + for attempt in 1 2 3; do + PR_DATA="$(gh pr view "${PR_NUMBER}" --json title,additions,deletions,changedFiles,baseRefName,headRefName)" && break + echo "::warning::gh pr view failed (attempt ${attempt}/3); retrying in 5s..." + sleep 5 + done { echo "pr_data<> "${GITHUB_OUTPUT}" - CHANGED_FILES="$(gh pr diff "${PR_NUMBER}" --name-only)" + for attempt in 1 2 3; do + CHANGED_FILES="$(gh pr diff "${PR_NUMBER}" --name-only)" && break + echo "::warning::gh pr diff failed (attempt ${attempt}/3); retrying in 5s..." + sleep 5 + done { echo "changed_files<> "${GITHUB_OUTPUT}" - PR_DATA="$(gh pr view "${PR_NUMBER}" --json title,additions,deletions,changedFiles,baseRefName,headRefName)" + for attempt in 1 2 3; do + PR_DATA="$(gh pr view "${PR_NUMBER}" --json title,additions,deletions,changedFiles,baseRefName,headRefName)" && break + echo "::warning::gh pr view failed (attempt ${attempt}/3); retrying in 5s..." + sleep 5 + done { echo "pr_data<> "${GITHUB_OUTPUT}" - CHANGED_FILES="$(gh pr diff "${PR_NUMBER}" --name-only)" + for attempt in 1 2 3; do + CHANGED_FILES="$(gh pr diff "${PR_NUMBER}" --name-only)" && break + echo "::warning::gh pr diff failed (attempt ${attempt}/3); retrying in 5s..." + sleep 5 + done { echo "changed_files<> "${GITHUB_OUTPUT}" - PR_DATA="$(gh pr view "${PR_NUMBER}" --json title,additions,deletions,changedFiles,baseRefName,headRefName)" + for attempt in 1 2 3; do + PR_DATA="$(gh pr view "${PR_NUMBER}" --json title,additions,deletions,changedFiles,baseRefName,headRefName)" && break + echo "::warning::gh pr view failed (attempt ${attempt}/3); retrying in 5s..." + sleep 5 + done { echo "pr_data<> "${GITHUB_OUTPUT}" - CHANGED_FILES="$(gh pr diff "${PR_NUMBER}" --name-only)" + for attempt in 1 2 3; do + CHANGED_FILES="$(gh pr diff "${PR_NUMBER}" --name-only)" && break + echo "::warning::gh pr diff failed (attempt ${attempt}/3); retrying in 5s..." + sleep 5 + done { echo "changed_files</, OR - # (b) Its manifest.yaml contains: language: "python" + # (b) It lives under contrib/python//, OR + # (c) It lives under contrib// AND manifest.yaml + # contains: language: "python" # - # Layout: - # core/python//... — always Python (namespace dir) - # contrib//... — Python only if manifest says so + # Layouts recognised: + # core/python//... — always Python (namespace dir) + # contrib/python//... — always Python (namespace dir) + # contrib//... — Python only if manifest says so + # + # Order matters in the case statement below: the more-specific + # `contrib/python/*/*` pattern MUST come before the generic + # `contrib/*/*`, or the generic branch will match first and resolve + # to `contrib/python` (a namespace dir with no manifest.yaml), which + # then silently skips the real recipe at `contrib/python//`. + # This was a real bug — recipes at `contrib/python//` were + # never validated (folder-name check, size limits, model-literal + # notice, structural pyproject.toml rules — all bypassed). # --------------------------------------------------------------------------- CHANGED_RECIPES="" @@ -82,12 +94,32 @@ jobs: recipe_name="${without_root%%/*}" # recipe_dir="core/python/${recipe_name}" ;; + contrib/python/*/*) + # contrib/python//anything → recipe is contrib/python/. + # MUST match before the generic `contrib/*/*` case below, otherwise + # the generic one wins and resolves to `contrib/python` (namespace + # dir, no manifest) → silently skipped. + without_root="${file#contrib/python/}" + recipe_name="${without_root%%/*}" + recipe_dir="contrib/python/${recipe_name}" + ;; contrib/*/*) # contrib//anything → candidate, check manifest later without_root="${file#contrib/}" recipe_name="${without_root%%/*}" recipe_dir="contrib/${recipe_name}" ;; + core/*/*) + # core//anything (flat, non-namespaced) → candidate, + # check manifest for language: python later. + without_root="${file#core/}" + recipe_name="${without_root%%/*}" + # Skip the language namespace dir itself (core/python, core/go…) + case "$recipe_name" in + python|java|go|typescript|kotlin) continue ;; + esac + recipe_dir="core/${recipe_name}" + ;; *) continue ;; @@ -96,8 +128,12 @@ jobs: [ -z "$recipe_dir" ] && continue [ -d "$recipe_dir" ] || continue # skip deleted dirs - # For contrib recipes, only include if manifest.yaml declares language: python - if [[ "$recipe_dir" == contrib/* ]]; then + # For recipes NOT under a language namespace dir (flat core// + # or contrib//), only include if manifest.yaml declares + # language: python. Namespaced dirs (core/python/*, contrib/python/*) + # are Python by convention and don't need the manifest check. + if [[ "$recipe_dir" == contrib/* && "$recipe_dir" != contrib/python/* ]] || \ + [[ "$recipe_dir" == core/* && "$recipe_dir" != core/python/* ]]; then manifest="$recipe_dir/manifest.yaml" if [ ! -f "$manifest" ]; then continue @@ -139,7 +175,32 @@ jobs: [ -d "$d" ] && ALL="${ALL}${d%/}"$'\n' done - # contrib/* — only if manifest declares python + # core// (flat, non-namespaced) — only if manifest declares python. + # Skips language namespace dirs (core/python/, core/go/, etc.). + for d in core/*/; do + [ -d "$d" ] || continue + recipe_name="$(basename "${d%/}")" + case "$recipe_name" in + python|java|go|typescript|kotlin) continue ;; + esac + manifest="${d}manifest.yaml" + [ -f "$manifest" ] || continue + if grep -qE '^\s*language\s*:\s*["'"'"']?python["'"'"']?\s*(#.*)?$' "$manifest"; then + ALL="${ALL}${d%/}"$'\n' + fi + done + + # contrib/python/* — always Python (namespace dir, like core/python/). + # Kept as a separate loop from `contrib/*/` below so the language + # dir itself (contrib/python/) is never scanned for a manifest.yaml + # it doesn't have. + for d in contrib/python/*/; do + [ -d "$d" ] && ALL="${ALL}${d%/}"$'\n' + done + + # contrib// — only if manifest declares python. Skips + # language namespace dirs (contrib/python/, contrib/go/, etc.) + # since those have no top-level manifest.yaml. for d in contrib/*/; do [ -d "$d" ] || continue manifest="${d}manifest.yaml" @@ -376,20 +437,22 @@ jobs: # ------------------------------------------------------------------ # Check 5: Required keys in .env.example # ------------------------------------------------------------------ - REQUIRED_ENV_KEYS=( + # Keys that emit a ::notice if missing but do not fail the check. + # These are commonly set by GCP runtimes or ADK internals and + # their absence from .env.example is not necessarily an error. + NOTICE_ENV_KEYS=( "GOOGLE_CLOUD_PROJECT" "GOOGLE_CLOUD_LOCATION" "MODEL_NAME" ) if [ -f "$recipe/.env.example" ]; then - for key in "${REQUIRED_ENV_KEYS[@]}"; do + for key in "${NOTICE_ENV_KEYS[@]}"; do if ! grep -qE "^(export\s+)?${key}(_[A-Z0-9_]*)?\s*=" "$recipe/.env.example"; then - echo "::error file=$recipe/.env.example::No environment variable starting with '${key}' found in .env.example. All recipes must declare at least one variable prefixed with GOOGLE_CLOUD_PROJECT, GOOGLE_CLOUD_LOCATION, and MODEL_NAME in their .env.example file." - echo "[FAIL] No env key with prefix '$key' found in .env.example." - RECIPE_FAILED=1 + echo "::notice file=$recipe/.env.example::No environment variable starting with '${key}' found in .env.example. Consider adding one if your recipe uses it." + echo "[NOTICE] No env key with prefix '$key' in .env.example (not required)." else - echo "[PASS] Found required env key in .env.example: $key" + echo "[PASS] Found env key in .env.example: $key" fi done fi diff --git a/pyproject.toml b/pyproject.toml index 89e44e6054..55eb97d574 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,6 +24,11 @@ dev = [ requires = ["hatchling"] build-backend = "hatchling.build" +[tool.uv] +# Pin the minimum uv version so astral-sh/setup-uv reads it from here +# instead of falling back to the GitHub API (which fails during outages). +required-version = ">=0.5" + # Use public PyPI as the default index for this project. [[tool.uv.index]] url = "https://pypi.org/simple/" diff --git a/tools/validate_manifest.py b/tools/validate_manifest.py index 0a1c86ccd3..3bfbe0c5dc 100644 --- a/tools/validate_manifest.py +++ b/tools/validate_manifest.py @@ -56,8 +56,18 @@ def is_recipe_dir(path: Path) -> bool: # they are containers whose children are the actual recipes. if path.name in LANGUAGE_NAMESPACE_DIRS: return False - children = [p for p in path.iterdir() if p.name != "README.md"] - return len(children) > 0 + children = [ + p + for p in path.iterdir() + if not p.name.startswith(".") and p.name != "README.md" + ] + if not children: + return False + # A directory whose non-hidden children are exclusively language namespace + # dirs is itself a container (e.g. core/harnesses/), not a recipe. + if all(p.is_dir() and p.name in LANGUAGE_NAMESPACE_DIRS for p in children): + return False + return True def load_schema() -> dict: