From d9e0a186dae818093a8c48fbdb3fb9c0a18f7a1c Mon Sep 17 00:00:00 2001 From: MrTango Date: Thu, 13 Aug 2026 18:28:12 +0000 Subject: [PATCH 1/3] Fix scaffolding evaluation failures --- .github/workflows/python-package.yml | 29 +- evals/scaffolding/.gitignore | 4 + evals/scaffolding/EVALUATION.md | 70 + evals/scaffolding/README.md | 92 ++ evals/scaffolding/run_evals.py | 1268 +++++++++++++++++++ evals/scaffolding/validators.py | 92 ++ plonecli/cli.py | 16 +- plonecli/git.py | 9 +- plonecli/project.py | 26 +- pyproject.toml | 1 - tests/test_git.py | 14 + tests/test_plonecli.py | 39 + tests/test_project.py | 25 +- tests/test_theme_barceloneta_integration.py | 11 +- uv.lock | 16 +- 15 files changed, 1676 insertions(+), 36 deletions(-) create mode 100644 evals/scaffolding/.gitignore create mode 100644 evals/scaffolding/EVALUATION.md create mode 100644 evals/scaffolding/README.md create mode 100644 evals/scaffolding/run_evals.py create mode 100644 evals/scaffolding/validators.py diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index c7ea994..4fedfbc 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -23,7 +23,7 @@ jobs: uses: actions/checkout@v2 with: repository: plone/copier-templates - path: copier-templates + path: develop/plone/src/copier-templates - name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v2 with: @@ -44,6 +44,31 @@ jobs: flake8 . --exclude=copier-templates --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics - name: Test with pytest env: - PLONECLI_TEMPLATES_DIR: ${{ github.workspace }}/copier-templates + PLONECLI_TEMPLATES_DIR: ${{ github.workspace }}/develop/plone/src/copier-templates run: | pytest + + scaffolding: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + - name: Checkout copier-templates + uses: actions/checkout@v4 + with: + repository: plone/copier-templates + path: develop/plone/src/copier-templates + - name: Set up uv + uses: astral-sh/setup-uv@v6 + with: + python-version: "3.12" + - name: Install dependencies + run: uv sync --extra test + - name: Validate generated scaffolding + run: uv run python evals/scaffolding/run_evals.py --ci-validation + - name: Upload scaffolding report + if: always() + uses: actions/upload-artifact@v4 + with: + name: scaffolding-evaluation + path: evals/scaffolding/results/ diff --git a/evals/scaffolding/.gitignore b/evals/scaffolding/.gitignore new file mode 100644 index 0000000..294788e --- /dev/null +++ b/evals/scaffolding/.gitignore @@ -0,0 +1,4 @@ +workspaces/ +results/ +__pycache__/ +*.py[cod] diff --git a/evals/scaffolding/EVALUATION.md b/evals/scaffolding/EVALUATION.md new file mode 100644 index 0000000..b825443 --- /dev/null +++ b/evals/scaffolding/EVALUATION.md @@ -0,0 +1,70 @@ +# Scaffolding evaluation findings + +> Historical baseline from before the fixes. The current full report passes +> 245/245 cases; see the ignored `results/report.md` generated on 2026-08-13. + +## Scope + +The full run executed 245 cases against the development templates checkout: + +- all 3 project templates; +- all 20 feature templates individually; +- explicit finite high-interaction matrices for backend/Svelte booleans, behavior booleans, REST booleans crossed with target mode, reachable content-type states, view choices/booleans, vocabulary types, all viewlet managers and template states, and Zope distribution/storage choices; +- combined, reversed-order, repeated-application, hostile-input, and command-chain cases; +- harmless root commands and the CLI command unit suite. + +Open-ended strings, integers, and environment-discovered choices cannot have a literal exhaustive Cartesian product. They are covered with defaults, non-default valid values, manual-choice paths, and hostile quote/newline/backslash partitions. Finite domains are exhaustive only in the explicitly named high-interaction matrices; other templates receive individual non-default cases plus their focused unit tests. + +Result: **237 passed, 8 failed**. See the ignored runtime report at `results/report.md` and per-case logs under `results/logs/`. + +## Problems + +### High: free text can generate invalid TOML + +`backend_addon` and `zope-setup` interpolate title, description, and author values directly into quoted TOML. Quotes, newlines, and backslashes can produce an invalid `pyproject.toml`. + +Evidence: + +- `hostile-backend-toml-strings`: generation returned success, but TOML validation failed. +- `hostile-zope-toml-strings`: the generated TOML was invalid and the post-copy hook aborted while parsing it. + +Use a TOML-safe Jinja filter or generate these values through `tomlkit` instead of interpolating raw strings. + +### High: `zope_instance` cannot be added through plonecli + +All four `zope_instance` CLI cases failed because `plonecli add` did not list `zope_instance` in a standalone `zope-setup` project. The generated project contains both `[tool.plone.project.settings]` and `[tool.plone.backend_addon.settings]`. Project detection checks backend settings first, classifies the project as `backend_addon`, and exposes the wrong subtemplate set. + +Either avoid writing backend-addon settings for standalone Zope projects or make project detection prefer the substantive project settings in this mixed layout. + +### High: chained `create` then `setup` fails + +The CLI declares `chain=True`, but `chain-create-then-setup` failed after successfully creating the backend add-on. The group retains the project context detected before `create`, so `setup` still reports that it is outside a package. + +Refresh project context after creation, or remove command chaining if cross-context chains are not supported. + +### Medium: theme variants conflict without non-interactive resolution + +The all-feature sequence failed when `theme_barceloneta` followed `theme`: both own `profiles/default/theme.xml` and related theme paths. Copier requested an interactive overwrite despite `--defaults`, then aborted in the non-TTY evaluation. + +Treat theme templates as explicit alternatives and reject a second theme with a clear message, or add a documented overwrite/replacement flow. + +### Medium: Barceloneta integration test uses a stale path + +The root integration suite generated the test at `src/collective/mythemetest/tests/test_theme_my_test_theme.py`, but `tests/test_theme_barceloneta_integration.py` expects it under top-level `tests/`. Result: 23 integration cases passed and 1 failed. + +Update the assertion and pytest target to the generated `src//tests/` layout. + +## Optimization opportunities + +- Copier template extensions emitted hundreds of deprecation warnings because `ContextHook.update` is deprecated. Migrate hooks to modify context in `hook`. +- `click_aliases` reads deprecated `click.__version__`; update or replace the dependency before Click 9.1. +- Feature generation inside these nested, `--no-git` workspaces reports the outer plonecli repository as dirty. Git cleanliness checks should be scoped to the detected generated project rather than walking into an unrelated parent repository. +- Keep the generated TOML/XML/Python validators as CI checks. They found failures that successful Copier exit codes did not detect. + +## Test receipts + +- Root unit suite: **209 passed, 16 skipped**. +- Copier-template unit suite: **386 passed, 2 integration tests deselected**. +- Copier-template integration suite: **2 passed**. +- Root integration suite: **23 passed, 1 failed** (stale Barceloneta test path above). +- Full scaffolding matrix: **199 passed, 8 failed** (the eight cases map to four product problems above). diff --git a/evals/scaffolding/README.md b/evals/scaffolding/README.md new file mode 100644 index 0000000..d0a0317 --- /dev/null +++ b/evals/scaffolding/README.md @@ -0,0 +1,92 @@ +# plonecli scaffolding evaluations + +This directory contains a reusable, real-CLI evaluation of plonecli commands and +all copier templates in the development checkout. It is intentionally separate +from product and template source. + +## Run + +From the repository root: + +```sh +uv run python evals/scaffolding/run_evals.py --quick +uv run python evals/scaffolding/run_evals.py --ci-validation +uv run python evals/scaffolding/run_evals.py +``` + +`--quick` runs a reduced smoke subset. `--ci-validation` runs every template +once plus hostile-input and command checks. With no flag, the runner executes +the explicit finite high-interaction matrices described below. It audits the +repository template inventory and fails when a new template has no lane. The runner itself invokes plonecli only as +`uv run --project /workspaces/plonecli plonecli`, so copied generated projects cannot shadow the checkout with their own environment. It sets: + +```text +PLONECLI_TEMPLATES_DIR=/workspaces/plonecli/develop/plone/src/copier-templates +``` + +Generated trees are disposable and always live beneath `workspaces/`. Reports +and per-case command logs are written beneath `results/`: + +- `results/report.json` — machine-readable case inventory, commands, coverage, + validation results, counts, and failures. +- `results/report.md` — human-readable coverage table and problem summary. +- `results/logs/*.log` — captured stdout/stderr for every case. + +Both output directories are ignored by git and replaced at the start of a run. + +## Coverage + +The full run covers: + +- harmless root commands: help, template list, versions, and bash/zsh/fish + completion output; +- real non-default creation of `backend_addon`, `zope-setup`, and the `addon` + composite, plus a real standalone `setup` application; +- every currently shipped backend subtemplate individually against a copied + clean parent, plus `zope_instance` against a copied Zope parent; +- both backend headless states and both Svelte custom-element states; +- the complete behavior boolean matrix (4 cases); +- all REST boolean states crossed with normal/manual registration targets + (64 cases); +- all reachable content-type gated boolean/choice states; +- every view base class × template × marker × normal/manual target state; +- both vocabulary implementation choices; +- all 26 viewlet managers with both template values (52 cases); +- both Zope distributions × all three storage modes, and all three + `zope_instance` storage modes; +- one all-backend-subtemplates project, reversed-order pairs, and representative + repeated-application/idempotency cases; +- TOML-hostile quote/newline/backslash partitions and a real chained + `create` → `setup` command. + +Names are unique per isolated project to make collisions deterministic. The +report records the full planned and actually executed counts by category, and +each matrix case records its parameter values. + +## Validation and safety + +Every generated project receives deterministic syntax and duplicate-registration +checks without installing Plone. The template unit suite supplies +feature-specific semantic assertions: + +- every TOML file is parsed with `tomllib`; +- every XML and ZCML file is parsed; +- every Python file is compiled; +- exact duplicate direct-child XML registrations are reported where practical; +- every subprocess exit code is checked; +- stdin is disabled and every command has a configurable timeout. + +The harness does **not** run `serve` or `debug`, and does not run a generated +project's `test` task because those branches can start services or resolve a +full Plone environment. Instead it runs the repository's root CLI command unit +test suite, which covers `serve`, `debug`, and `test` dispatch and error paths +with mocked subprocesses. Template hooks may still ask native `uv` to resolve +small hook-only tools (`tomlkit`, Copier extensions); use a warmed uv cache for +the most network-independent run. + +## Reading failures + +A nonzero runner exit means at least one case failed or was blocked. Start with +`results/report.md`, then inspect the referenced log. Failures are retained as +evaluation findings rather than hidden or retried with defaults. Reports include +repository commits, dirty state, Python, and uv provenance. diff --git a/evals/scaffolding/run_evals.py b/evals/scaffolding/run_evals.py new file mode 100644 index 0000000..d763642 --- /dev/null +++ b/evals/scaffolding/run_evals.py @@ -0,0 +1,1268 @@ +#!/usr/bin/env python3 +"""Run real-CLI, install-free plonecli scaffolding evaluations.""" + +from __future__ import annotations + +import argparse +import itertools +import json +import os +import shutil +import subprocess +import sys +import time +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +from validators import validate_project + +ROOT = Path(__file__).resolve().parents[2] +HERE = Path(__file__).resolve().parent +TEMPLATES = Path( + os.environ.get( + "PLONECLI_TEMPLATES_DIR", + ROOT / "develop/plone/src/copier-templates", + ) +).resolve() +WORKSPACES = HERE / "workspaces" +RESULTS = HERE / "results" +LOGS = RESULTS / "logs" +CLI = ["uv", "run", "--project", str(ROOT), "plonecli"] + +BACKEND_SUBTEMPLATES = ( + "behavior", + "content_type", + "controlpanel", + "form", + "indexer", + "language", + "mockup_pattern", + "portlet", + "restapi_service", + "site_initialization", + "subscriber", + "svelte_app", + "theme", + "theme_barceloneta", + "theme_basic", + "upgrade_step", + "view", + "viewlet", + "vocabulary", +) +VIEWLET_MANAGERS = ( + "plone.htmlhead", + "plone.htmlhead.links", + "plone.htmlhead.javascript", + "plone.portaltop", + "plone.portalheader", + "plone.portalfooter", + "plone.portalbottom", + "plone.header", + "plone.footer", + "plone.mainnavigation", + "plone.searchbox", + "plone.personal_bar", + "plone.site_actions", + "plone.globalstatusmessage", + "plone.abovecontent", + "plone.belowcontent", + "plone.abovecontenttitle", + "plone.belowcontenttitle", + "plone.abovecontentbody", + "plone.belowcontentbody", + "plone.abovecontentactions", + "plone.belowcontentactions", + "plone.contentactions", + "plone.contentviews", + "plone.portalleftcolumn", + "plone.portalrightcolumn", +) + + +@dataclass +class Step: + args: list[str] + cwd: Path + + +@dataclass +class Case: + case_id: str + category: str + steps: list[Step] + project: Path | None = None + copy_from: Path | None = None + coverage: dict[str, Any] = field(default_factory=dict) + quick: bool = False + + +def slug(value: str) -> str: + return "".join(ch if ch.isalnum() else "-" for ch in value.lower()).strip("-") + + +def audit_template_inventory() -> None: + """Fail when a template is added without an explicit evaluation lane.""" + discovered = {path.parent.name for path in TEMPLATES.glob("*/copier.yml")} + expected = { + "addon", + "backend_addon", + "zope-setup", + "zope_instance", + *BACKEND_SUBTEMPLATES, + } + if discovered != expected: + missing = sorted(discovered - expected) + stale = sorted(expected - discovered) + raise SystemExit( + f"template inventory drift; add evaluation cases: " + f"uncovered={missing}, missing={stale}" + ) + + +def data_args(data: dict[str, Any]) -> list[str]: + result: list[str] = [] + for key, value in data.items(): + if isinstance(value, bool): + value = str(value).lower() + result.extend(["-d", f"{key}={value}"]) + return result + + +def create_step(template: str, target: Path, data: dict[str, Any]) -> Step: + return Step( + [ + "create", + template, + str(target), + "--defaults", + "--allow-dirty", + "--no-git", + *data_args(data), + ], + ROOT, + ) + + +def add_step(template: str, target: Path, data: dict[str, Any]) -> Step: + # Options intentionally follow the positional template. This exercises the + # chain=True/interspersed-argument behavior used in documentation. + return Step( + [ + "add", + template, + "--defaults", + "--allow-dirty", + "--no-git", + *data_args(data), + ], + target, + ) + + +def backend_data(package: str) -> dict[str, Any]: + return { + "package_name": package, + "package_title": "Evaluation Backend", + "package_description": "Generated by the scaffolding evaluation", + "plone_version": "6.1", + "is_headless": True, + "author_name": "Evaluation Runner", + "author_email": "eval@example.invalid", + "github_organization": "plone-evals", + } + + +def zope_data( + name: str, distribution: str = "plone.classicui", storage: str = "zeo" +) -> dict[str, Any]: + data: dict[str, Any] = { + "project_name": name, + "project_title": "Evaluation Zope Project", + "project_description": "Generated without installing Plone", + "plone_version": "6.1.1", + "distribution": distribution, + "base_path": "runtime", + "db_storage": storage, + "author_name": "Evaluation Runner", + "author_email": "eval@example.invalid", + "initial_zope_username": "eval-admin", + "initial_user_password": "not-a-real-password", + } + if storage == "zeo": + data["zeo_address"] = "127.0.0.1:9100" + elif storage == "relstorage": + data.update( + pg_host="db.invalid", + pg_port=5544, + pg_dbname="eval_plone", + pg_user="eval_user", + pg_password="eval_password", + ) + return data + + +def individual_data(template: str, index: int) -> dict[str, Any]: + suffix = f"{index:02d}" + mapping: dict[str, dict[str, Any]] = { + "behavior": { + "behavior_name": f"IEvalBehavior{suffix}", + "behavior_description": "Non-default behavior", + "behavior_marker": False, + "behavior_factory": True, + }, + "content_type": { + "content_type_name": f"Eval Article {suffix}", + "content_type_description": "Non-default content type", + "content_type_base": "Item", + "content_type_icon": "newspaper", + "global_allow": False, + "parent_content_type": "", + "parent_content_type_manual": "Eval Container", + "activate_default_behaviors": False, + "enable_dublin_core": False, + "enable_navigation": True, + }, + "controlpanel": { + "controlpanel_name": f"EvalSettings{suffix}", + "controlpanel_title": f"Evaluation settings {suffix}", + "controlpanel_description": "Non-default control panel", + }, + "form": { + "form_name": f"eval-form-{suffix}", + "form_class_name": f"EvalForm{suffix}", + "form_for": "zope.interface.Interface", + "form_description": "Non-default form", + }, + "indexer": { + "indexer_name": f"eval_index_{suffix}", + "indexer_description": "Non-default indexer", + }, + "language": { + "language_code": f"x{suffix}", + "language_name": f"Evaluation Language {suffix}", + }, + "mockup_pattern": { + "pattern_name": f"eval-pattern-{suffix}", + "pattern_description": "Non-default pattern", + }, + "portlet": { + "portlet_name": f"EvalPortlet{suffix}", + "portlet_description": "Non-default portlet", + }, + "restapi_service": { + "service_name": f"eval-service-{suffix}", + "service_description": "Non-default REST service", + "expandable": True, + "http_get": False, + "http_post": True, + "http_patch": True, + "http_delete": True, + "service_for": "", + "service_for_manual": "zope.interface.Interface", + }, + "site_initialization": { + "site_name": f"Evaluation Site {suffix}", + "language": "de", + }, + "subscriber": { + "subscriber_handler_name": f"eval_handler_{suffix}", + "subscriber_event": "zope.lifecycleevent.interfaces.IObjectAddedEvent", + "subscriber_for": "zope.interface.Interface", + "subscriber_description": "Non-default subscriber", + }, + "svelte_app": { + "svelte_app_name": f"eval-app-{suffix}", + "svelte_app_description": "Non-default Svelte app", + "svelte_app_custom_element": False, + }, + "theme": { + "theme_name": f"Eval Theme {suffix}", + "theme_description": "Non-default theme", + }, + "theme_barceloneta": { + "theme_name": f"Eval Barceloneta {suffix}", + "theme_description": "Non-default Barceloneta theme", + }, + "theme_basic": { + "theme_name": f"Eval Basic {suffix}", + "theme_description": "Non-default basic theme", + }, + "upgrade_step": { + "upgrade_step_title": f"Evaluation upgrade {suffix}", + "upgrade_step_description": "Non-default upgrade", + "source_version": f"20{suffix}", + "destination_version": f"21{suffix}", + }, + "view": { + "view_name": f"eval-view-{suffix}", + "view_class_name": f"EvalView{suffix}", + "view_base_class": "CollectionView", + "view_template": False, + "view_for": "", + "view_for_manual": "zope.interface.Interface", + "view_marker": True, + "view_description": "Non-default view", + }, + "viewlet": { + "viewlet_name": f"evalviewlet{suffix}", + "viewlet_class_name": f"EvalViewlet{suffix}", + "viewlet_manager": "plone.portalfooter", + "viewlet_for": "zope.interface.Interface", + "viewlet_template": False, + "viewlet_description": "Non-default viewlet", + }, + "vocabulary": { + "vocabulary_name": f"EvalVocabulary{suffix}", + "vocabulary_description": "Non-default vocabulary", + "vocabulary_type": "catalog", + }, + } + return mapping[template] + + +def build_cases(quick: bool) -> tuple[list[Case], dict[str, int]]: + cases: list[Case] = [] + create_dir = WORKSPACES / "create" + backend_parent = create_dir / "backend-parent" + zope_parent = create_dir / "zope-parent" + addon_target = create_dir / "addon-composite" + + harmless = [ + ("help", ["--help"]), + ("list", ["--list-templates"]), + ("versions", ["--versions"]), + ("completion-bash", ["completion", "bash"]), + ("completion-zsh", ["completion", "zsh"]), + ("completion-fish", ["completion", "fish"]), + ] + for name, args in harmless: + cases.append( + Case(f"root-{name}", "root-harmless", [Step(args, ROOT)], quick=True) + ) + + command_tests = [ + "tests/test_plonecli.py", + "tests/test_completion_command.py", + "tests/test_setup_command.py", + "tests/test_config_command.py", + "tests/test_update_command.py", + "tests/test_skill.py", + ] + cases.append( + Case( + "root-command-unit-suite", + "root-command-tests", + [Step(["__uv__", "run", "pytest", "-q", *command_tests], ROOT)], + coverage={ + "side_effect_branches": ["serve", "debug", "test"], + "note": ( + "Covered with mocked invoke subprocesses; no server or full " + "Plone install is started." + ), + }, + quick=True, + ) + ) + cases.append( + Case( + "template-unit-suite", + "template-tests", + [ + Step( + ["__uv__", "run", "pytest", "-q"], + TEMPLATES, + ) + ], + coverage={"note": "Template-specific semantic assertions."}, + quick=False, + ) + ) + + cases.extend( + [ + Case( + "create-backend-addon", + "create", + [ + create_step( + "backend_addon", + backend_parent, + backend_data("collective.evalbackend"), + ) + ], + backend_parent, + quick=True, + ), + Case( + "create-zope-setup", + "create", + [create_step("zope-setup", zope_parent, zope_data("eval-zope"))], + zope_parent, + quick=True, + ), + Case( + "create-addon-composite", + "create", + [ + create_step( + "addon", + addon_target, + { + key: value + for key, value in { + **backend_data("collective.evalcomposite"), + **zope_data( + "collective.evalcomposite", + "plone.volto", + "relstorage", + ), + }.items() + if key != "plone_version" + }, + ) + ], + addon_target, + coverage={ + "note": ( + "plone_version is omitted because the composite's backend " + "minor-version and Zope full-version choices differ" + ) + }, + quick=True, + ), + ] + ) + + for headless in (False, True): + target = WORKSPACES / "matrix" / "backend" / f"headless-{int(headless)}" + cases.append( + Case( + f"backend-headless-{int(headless)}", + "matrix-backend", + [ + create_step( + "backend_addon", + target, + { + **backend_data(f"collective.headless{int(headless)}"), + "is_headless": headless, + }, + ) + ], + target, + coverage={"is_headless": headless}, + quick=False, + ) + ) + + hostile_backend = WORKSPACES / "hostile" / "backend-toml" + cases.append( + Case( + "hostile-backend-toml-strings", + "hostile-input", + [ + create_step( + "backend_addon", + hostile_backend, + { + **backend_data("collective.hostiletoml"), + "package_title": 'Quoted "title"', + "package_description": "Line one\nLine two \\ path", + "author_name": 'Eval "Runner"', + "author_email": "eval+quoted@example.invalid", + }, + ) + ], + hostile_backend, + coverage={"partition": "TOML quotes, newline, and backslash"}, + ) + ) + hostile_zope = WORKSPACES / "hostile" / "zope-toml" + cases.append( + Case( + "hostile-zope-toml-strings", + "hostile-input", + [ + create_step( + "zope-setup", + hostile_zope, + { + **zope_data("hostile-zope", "plone.volto", "instance"), + "project_title": 'Quoted "title"', + "project_description": "Line one\nLine two \\ path", + "author_name": 'Eval "Runner"', + }, + ) + ], + hostile_zope, + coverage={"partition": "TOML quotes, newline, and backslash"}, + ) + ) + + chained_target = WORKSPACES / "chains" / "create-then-setup" + chained_create = create_step( + "backend_addon", + chained_target, + backend_data("collective.chaincheck"), + ) + cases.append( + Case( + "chain-create-then-setup", + "command-chain", + [ + Step( + [ + *chained_create.args, + "setup", + "--defaults", + "--allow-dirty", + ], + ROOT, + ) + ], + chained_target, + coverage={"commands": ["create", "setup"]}, + ) + ) + + setup_target = WORKSPACES / "commands" / "setup-backend" + cases.append( + Case( + "command-setup-backend", + "command-real", + [ + Step( + ["setup", "--defaults", "--allow-dirty"], + setup_target, + ) + ], + setup_target, + backend_parent, + {"command": "setup"}, + quick=False, + ) + ) + + for index, template in enumerate(BACKEND_SUBTEMPLATES, 1): + target = WORKSPACES / "individual" / template + cases.append( + Case( + f"individual-{template}", + "individual-subtemplate", + [add_step(template, target, individual_data(template, index))], + target, + backend_parent, + {"template": template}, + quick=template + in {"behavior", "content_type", "restapi_service", "view"}, + ) + ) + zi_target = WORKSPACES / "individual" / "zope-instance" + cases.append( + Case( + "individual-zope-instance", + "individual-subtemplate", + [ + add_step( + "zope_instance", + zi_target, + { + "instance_name": "eval-extra", + "port": 8188, + "base_path": "runtime-extra", + "db_storage": "relstorage", + "pg_host": "db.invalid", + "pg_port": 5544, + "pg_dbname": "eval_extra", + "pg_user": "eval", + "pg_password": "secret", + "initial_zope_username": "runner", + "initial_user_password": "secret", + }, + ) + ], + zi_target, + zope_parent, + {"template": "zope_instance"}, + quick=True, + ) + ) + + # Hidden behavior booleans: the complete 2^2 interaction matrix. + for marker, factory in itertools.product((False, True), repeat=2): + bits = f"m{int(marker)}-f{int(factory)}" + target = WORKSPACES / "matrix" / "behavior" / bits + cases.append( + Case( + f"behavior-{bits}", + "matrix-behavior", + [ + add_step( + "behavior", + target, + { + "behavior_name": f"IBehavior{int(marker)}{int(factory)}", + "behavior_marker": marker, + "behavior_factory": factory, + }, + ) + ], + target, + backend_parent, + {"behavior_marker": marker, "behavior_factory": factory}, + quick=not marker and not factory, + ) + ) + + # Cross all five REST booleans with both deterministic target modes. + rest_fields = ("expandable", "http_get", "http_post", "http_patch", "http_delete") + rest_states = itertools.product( + itertools.product((False, True), repeat=5), ("normal", "manual") + ) + for rest_number, (values, target_mode) in enumerate(rest_states): + flags = dict(zip(rest_fields, values, strict=True)) + manual = target_mode == "manual" + data = { + "service_name": f"matrix-service-{rest_number:02d}", + **flags, + "service_for": "" + if manual + else "plone.dexterity.interfaces.IDexterityContainer", + } + if manual: + data["service_for_manual"] = "zope.interface.Interface" + target = WORKSPACES / "matrix" / "rest" / f"case-{rest_number:02d}" + cases.append( + Case( + f"rest-{rest_number:02d}", + "matrix-rest", + [add_step("restapi_service", target, data)], + target, + backend_parent, + {**flags, "target_mode": target_mode}, + quick=rest_number in {0, 63}, + ) + ) + + # Exhaustive reachable content-type states: gated filter flag, parent + # selection, and the 1 + 2^2 default-behavior states. + ct_number = 0 + behavior_states = [ + (True, None, None), + *[(False, dc, nav) for dc, nav in itertools.product((False, True), repeat=2)], + ] + for base in ("Container", "Item"): + filters = (False, True) if base == "Container" else (None,) + for global_allow in (False, True): + parents = ("normal", "manual") if not global_allow else (None,) + for filter_value, parent_mode, (activate, dc, nav) in itertools.product( + filters, parents, behavior_states + ): + data = { + "content_type_name": f"Matrix Type {ct_number:02d}", + "content_type_base": base, + "global_allow": global_allow, + "activate_default_behaviors": activate, + } + if filter_value is not None: + data["filter_content_types"] = filter_value + if parent_mode: + data["parent_content_type"] = ( + "Folder" if parent_mode == "normal" else "" + ) + if parent_mode == "manual": + data["parent_content_type_manual"] = "Matrix Parent" + if not activate: + data.update(enable_dublin_core=dc, enable_navigation=nav) + target = ( + WORKSPACES / "matrix" / "content-type" / f"case-{ct_number:02d}" + ) + coverage = { + "base": base, + "global_allow": global_allow, + "filter_content_types": filter_value, + "parent_mode": parent_mode, + "activate_default_behaviors": activate, + "enable_dublin_core": dc, + "enable_navigation": nav, + } + cases.append( + Case( + f"content-type-{ct_number:02d}", + "matrix-content-type", + [add_step("content_type", target, data)], + target, + backend_parent, + coverage, + quick=ct_number in {0, 44}, + ) + ) + ct_number += 1 + + view_combinations = itertools.product( + ("BrowserView", "DefaultView", "CollectionView"), + (False, True), + (False, True), + ("normal", "manual"), + ) + for view_number, (base, template_value, marker, target_mode) in enumerate( + view_combinations + ): + data = { + "view_name": f"matrix-view-{view_number:02d}", + "view_class_name": f"MatrixView{view_number:02d}", + "view_base_class": base, + "view_template": template_value, + "view_marker": marker, + "view_for": "*" if target_mode == "normal" else "", + } + if target_mode == "manual": + data["view_for_manual"] = "zope.interface.Interface" + target = WORKSPACES / "matrix" / "view" / f"case-{view_number:02d}" + cases.append( + Case( + f"view-{view_number:02d}", + "matrix-view", + [add_step("view", target, data)], + target, + backend_parent, + { + "base": base, + "template": template_value, + "marker": marker, + "target_mode": target_mode, + }, + quick=view_number in {0, 23}, + ) + ) + + for number, vocabulary_type in enumerate(("simple", "catalog")): + target = WORKSPACES / "matrix" / "vocabulary" / vocabulary_type + cases.append( + Case( + f"vocabulary-{vocabulary_type}", + "matrix-vocabulary", + [ + add_step( + "vocabulary", + target, + { + "vocabulary_name": f"MatrixVocabulary{number}", + "vocabulary_type": vocabulary_type, + }, + ) + ], + target, + backend_parent, + {"vocabulary_type": vocabulary_type}, + quick=True, + ) + ) + + for manager_number, manager in enumerate(VIEWLET_MANAGERS): + for template_value in (False, True): + target = ( + WORKSPACES + / "matrix" + / "viewlet" + / f"manager-{manager_number:02d}-template-{int(template_value)}" + ) + cases.append( + Case( + f"viewlet-{manager_number:02d}-t{int(template_value)}", + "matrix-viewlet", + [ + add_step( + "viewlet", + target, + { + "viewlet_name": ( + f"matrixviewlet{manager_number:02d}" + f"{int(template_value)}" + ), + "viewlet_class_name": ( + f"MatrixViewlet{manager_number:02d}" + f"{int(template_value)}" + ), + "viewlet_manager": manager, + "viewlet_template": template_value, + }, + ) + ], + target, + backend_parent, + {"manager": manager, "template": template_value}, + quick=manager_number in {0, 25} and not template_value, + ) + ) + + for custom_element in (False, True): + target = ( + WORKSPACES / "matrix" / "svelte" / f"custom-element-{int(custom_element)}" + ) + cases.append( + Case( + f"svelte-custom-element-{int(custom_element)}", + "matrix-svelte", + [ + add_step( + "svelte_app", + target, + { + "svelte_app_name": f"matrix-svelte-{int(custom_element)}", + "svelte_app_custom_element": custom_element, + }, + ) + ], + target, + backend_parent, + {"svelte_app_custom_element": custom_element}, + quick=False, + ) + ) + + for distribution, storage in itertools.product( + ("plone.volto", "plone.classicui"), ("instance", "relstorage", "zeo") + ): + label = f"{distribution.split('.')[-1]}-{storage}" + target = WORKSPACES / "matrix" / "zope-setup" / label + cases.append( + Case( + f"zope-setup-{label}", + "matrix-zope-setup", + [ + create_step( + "zope-setup", + target, + zope_data(f"eval-{label}", distribution, storage), + ) + ], + target, + coverage={"distribution": distribution, "db_storage": storage}, + quick=label == "classicui-instance", + ) + ) + + for number, storage in enumerate(("instance", "relstorage", "zeo")): + target = WORKSPACES / "matrix" / "zope-instance" / storage + data: dict[str, Any] = { + "instance_name": f"matrix-{storage}", + "port": 8280 + number, + "base_path": "matrix-runtime", + "db_storage": storage, + } + if storage == "zeo": + data["zeo_address"] = "127.0.0.1:9200" + elif storage == "relstorage": + data.update( + pg_host="db.invalid", + pg_port=5544, + pg_dbname="matrix", + pg_user="matrix", + pg_password="secret", + ) + cases.append( + Case( + f"zope-instance-{storage}", + "matrix-zope-instance", + [add_step("zope_instance", target, data)], + target, + zope_parent, + {"db_storage": storage}, + quick=storage == "instance", + ) + ) + + combination = WORKSPACES / "combinations" / "all-backend-subtemplates" + # Theme variants are mutually exclusive and are covered individually. Use + # the full theme in this valid all-features sequence rather than attempting + # to overlay three alternatives that own the same paths. + combinable_templates = tuple( + template + for template in BACKEND_SUBTEMPLATES + if template not in {"theme_barceloneta", "theme_basic"} + ) + combination_steps = [ + add_step(template, combination, individual_data(template, 100 + index)) + for index, template in enumerate(combinable_templates) + ] + cases.append( + Case( + "combination-all-backend-subtemplates", + "combination", + combination_steps, + combination, + backend_parent, + { + "templates": list(combinable_templates), + "excluded_alternatives": ["theme_barceloneta", "theme_basic"], + }, + quick=False, + ) + ) + + for name, order in ( + ("content-then-view", ("content_type", "view")), + ("view-then-content", ("view", "content_type")), + ): + target = WORKSPACES / "combinations" / name + steps = [ + add_step(template, target, individual_data(template, 210 + index)) + for index, template in enumerate(order) + ] + cases.append( + Case( + f"reversed-{name}", + "reversed-order", + steps, + target, + backend_parent, + {"order": list(order)}, + quick=False, + ) + ) + + for template in ("behavior", "viewlet", "vocabulary"): + target = WORKSPACES / "combinations" / f"idempotent-{template}" + data = individual_data(template, 240) + cases.append( + Case( + f"idempotency-{template}", + "idempotency", + [add_step(template, target, data), add_step(template, target, data)], + target, + backend_parent, + {"template": template, "applications": 2}, + quick=False, + ) + ) + + all_cases = cases + if quick: + cases = [case for case in cases if case.quick] + planned = {} + for case in all_cases: + planned[case.category] = planned.get(case.category, 0) + 1 + return cases, planned + + +def run_command( + step: Step, env: dict[str, str], timeout_seconds: int +) -> dict[str, Any]: + command = ( + ["uv", *step.args[1:]] + if step.args and step.args[0] == "__uv__" + else [*CLI, *step.args] + ) + started = time.monotonic() + try: + completed = subprocess.run( + command, + cwd=step.cwd, + env=env, + text=True, + capture_output=True, + stdin=subprocess.DEVNULL, + timeout=timeout_seconds, + ) + exit_code = completed.returncode + stdout = completed.stdout + stderr = completed.stderr + timed_out = False + except subprocess.TimeoutExpired as exc: + exit_code = 124 + stdout = exc.stdout or "" + stderr = (exc.stderr or "") + f"\nTimed out after {timeout_seconds}s\n" + timed_out = True + return { + "command": command, + "cwd": str(step.cwd), + "exit_code": exit_code, + "timed_out": timed_out, + "duration_seconds": round(time.monotonic() - started, 3), + "stdout": stdout, + "stderr": stderr, + } + + +def execute_case( + case: Case, env: dict[str, str], timeout_seconds: int +) -> dict[str, Any]: + if case.project and case.project.exists(): + shutil.rmtree(case.project) + if case.copy_from: + if not case.copy_from.exists(): + message = f"missing successful parent workspace: {case.copy_from}" + log_path = LOGS / f"{case.case_id}.log" + log_path.write_text(message + "\n", encoding="utf-8") + return { + "id": case.case_id, + "category": case.category, + "status": "blocked", + "coverage": case.coverage, + "errors": [message], + "observations": [], + "commands": [], + "validation": {}, + "log": str(log_path.relative_to(HERE)), + } + case.project.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree( + case.copy_from, + case.project, + ignore=shutil.ignore_patterns(".git", ".venv", "__pycache__"), + ) + # Keep generated-project git operations isolated from the repository + # that contains this evaluation harness. In particular, `setup` may + # commit by design and must never commit the outer working tree. + subprocess.run( + ["git", "init", "-q"], + cwd=case.project, + check=True, + stdin=subprocess.DEVNULL, + ) + subprocess.run( + ["git", "add", "-A"], + cwd=case.project, + check=True, + stdin=subprocess.DEVNULL, + ) + subprocess.run( + [ + "git", + "-c", + "user.name=Evaluation Runner", + "-c", + "user.email=eval@example.invalid", + "commit", + "-qm", + "Evaluation fixture", + ], + cwd=case.project, + check=True, + stdin=subprocess.DEVNULL, + ) + + commands = [] + errors = [] + observations: list[str] = [] + for step in case.steps: + step.cwd.mkdir(parents=True, exist_ok=True) + result = run_command(step, env, timeout_seconds) + commands.append(result) + output_lines = (result["stdout"] + "\n" + result["stderr"]).splitlines() + for line in output_lines: + lowered = line.lower() + if "warning" in lowered or "deprecated" in lowered: + observation = " ".join(line.split())[:300] + if observation and observation not in observations: + observations.append(observation) + if result["exit_code"] != 0: + errors.append(f"command {len(commands)} exited {result['exit_code']}") + break + + validation: dict[str, list[str]] = {} + if case.project and case.project.exists(): + validation = validate_project(case.project) + errors.extend(error for group in validation.values() for error in group) + + log_path = LOGS / f"{case.case_id}.log" + sections = [] + for number, command in enumerate(commands, 1): + sections.extend( + [ + f"## command {number}\n$ {' '.join(command['command'])}\n", + "### stdout\n", + command["stdout"], + "\n### stderr\n", + command["stderr"], + "\n", + ] + ) + log_path.write_text("".join(sections), encoding="utf-8") + for command in commands: + command.pop("stdout", None) + command.pop("stderr", None) + if errors and case.project: + # Never allow a partial prerequisite tree to feed later cases. + shutil.rmtree(case.project, ignore_errors=True) + return { + "id": case.case_id, + "category": case.category, + "status": "passed" if not errors else "failed", + "coverage": case.coverage, + "errors": errors, + "observations": observations, + "commands": commands, + "validation": validation, + "log": str(log_path.relative_to(HERE)), + } + + +def _git_value(repo: Path, *args: str) -> str: + completed = subprocess.run( + ["git", "-C", str(repo), *args], text=True, capture_output=True + ) + return completed.stdout.strip() if completed.returncode == 0 else "unknown" + + +def provenance() -> dict[str, Any]: + return { + "python": sys.version, + "uv": subprocess.run( + ["uv", "--version"], text=True, capture_output=True + ).stdout.strip(), + "plonecli_commit": _git_value(ROOT, "rev-parse", "HEAD"), + "plonecli_dirty": bool(_git_value(ROOT, "status", "--porcelain")), + "templates_commit": _git_value(TEMPLATES, "rev-parse", "HEAD"), + "templates_dirty": bool(_git_value(TEMPLATES, "status", "--porcelain")), + } + + +def write_reports( + mode: str, cases: list[dict[str, Any]], planned: dict[str, int], elapsed: float +) -> None: + status_counts: dict[str, int] = {} + executed: dict[str, int] = {} + for case in cases: + status_counts[case["status"]] = status_counts.get(case["status"], 0) + 1 + executed[case["category"]] = executed.get(case["category"], 0) + 1 + report = { + "schema_version": 2, + "provenance": provenance(), + "mode": mode, + "templates_dir": str(TEMPLATES), + "workspace_root": str(WORKSPACES), + "command_prefix": CLI, + "network_policy": ( + "No serve/debug and no generated-project test/install; template hook " + "dependency resolution may use uv caches/indexes." + ), + "planned_full_case_counts": planned, + "attempted_case_counts": executed, + "status_counts": status_counts, + "elapsed_seconds": round(elapsed, 3), + "cases": cases, + } + (RESULTS / "report.json").write_text( + json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) + + status_summary = ", ".join( + f"{key}: {value}" for key, value in sorted(status_counts.items()) + ) + lines = [ + "# plonecli scaffolding evaluation\n", + f"- Mode: **{mode}**", + f"- Cases: **{len(cases)}** ({status_summary})", + f"- Duration: **{elapsed:.1f}s**", + "- Generated projects were validated as TOML, XML/ZCML, and Python; " + "exact duplicate XML child registrations were checked.", + "- `serve`, `debug`, and generated-project `test` were not started. " + "Their branches are covered by the root pytest command suite.", + "\n## Mechanically auditable coverage\n", + "| Category | Full plan | Attempted |", + "|---|---:|---:|", + ] + for category in sorted(planned): + lines.append( + f"| `{category}` | {planned[category]} | {executed.get(category, 0)} |" + ) + lines.extend(["\n## Problems and opportunities\n"]) + failed = [case for case in cases if case["status"] != "passed"] + observed = [case for case in cases if case.get("observations")] + if not failed: + lines.append("No validation or command problems detected by this run.") + else: + for case in failed: + lines.append(f"### `{case['id']}` ({case['status']})") + for error in case["errors"]: + lines.append(f"- {error}") + lines.append(f"- Full command output: `{case.get('log', 'not available')}`") + if observed: + lines.append("\n### Non-failing warnings and optimization opportunities") + for case in observed: + for observation in case["observations"]: + lines.append(f"- `{case['id']}`: {observation}") + else: + lines.append("\nNo warning-derived optimization opportunities were observed.") + lines.extend( + [ + "\n## Case inventory\n", + "| Case | Category | Status | Coverage |", + "|---|---|---|---|", + ] + ) + for case in cases: + coverage = json.dumps(case["coverage"], sort_keys=True).replace("|", "\\|") + lines.append( + f"| `{case['id']}` | `{case['category']}` | " + f"{case['status']} | `{coverage}` |" + ) + (RESULTS / "report.md").write_text("\n".join(lines) + "\n", encoding="utf-8") + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--quick", + action="store_true", + help="Run a reduced smoke subset instead of the full finite matrix.", + ) + parser.add_argument( + "--ci-validation", + action="store_true", + help="Run every template once plus hostile-input and command checks.", + ) + parser.add_argument( + "--timeout", + type=int, + default=900, + help="Maximum seconds per command (default: 900).", + ) + args = parser.parse_args() + if args.quick and args.ci_validation: + parser.error("--quick and --ci-validation are mutually exclusive") + if not (TEMPLATES / ".git").exists(): + parser.error(f"template checkout is missing or is not a git clone: {TEMPLATES}") + audit_template_inventory() + + shutil.rmtree(WORKSPACES, ignore_errors=True) + shutil.rmtree(RESULTS, ignore_errors=True) + WORKSPACES.mkdir(parents=True) + LOGS.mkdir(parents=True) + env = dict(os.environ) + env["PLONECLI_TEMPLATES_DIR"] = str(TEMPLATES) + env["PYTHONUNBUFFERED"] = "1" + + cases, planned = build_cases(args.quick) + if args.ci_validation: + ci_categories = { + "command-chain", + "create", + "hostile-input", + "individual-subtemplate", + "root-command-tests", + "root-harmless", + } + cases = [case for case in cases if case.category in ci_categories] + started = time.monotonic() + results = [] + for number, case in enumerate(cases, 1): + print(f"[{number:03d}/{len(cases):03d}] {case.case_id}", flush=True) + results.append(execute_case(case, env, args.timeout)) + elapsed = time.monotonic() - started + mode = "ci-validation" if args.ci_validation else "quick" if args.quick else "full" + write_reports(mode, results, planned, elapsed) + failures = sum(result["status"] != "passed" for result in results) + print( + f"Wrote {RESULTS / 'report.json'} and {RESULTS / 'report.md'}; " + f"failures={failures}" + ) + return 1 if failures else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/evals/scaffolding/validators.py b/evals/scaffolding/validators.py new file mode 100644 index 0000000..3573e0c --- /dev/null +++ b/evals/scaffolding/validators.py @@ -0,0 +1,92 @@ +"""Static validation for projects generated by the scaffolding evaluation.""" + +from __future__ import annotations + +import tomllib +import xml.etree.ElementTree as ET +from collections import Counter +from pathlib import Path + +IGNORED_PARTS = {".git", ".venv", "node_modules", "__pycache__"} +XML_SUFFIXES = {".xml", ".zcml"} + + +def _files(root: Path): + for path in root.rglob("*"): + if path.is_file() and not (set(path.parts) & IGNORED_PARTS): + yield path + + +def validate_toml(root: Path) -> list[str]: + errors: list[str] = [] + for path in _files(root): + if path.suffix != ".toml": + continue + try: + with path.open("rb") as stream: + tomllib.load(stream) + except (OSError, tomllib.TOMLDecodeError) as exc: + errors.append(f"invalid TOML {path.relative_to(root)}: {exc}") + return errors + + +def validate_xml(root: Path) -> list[str]: + errors: list[str] = [] + for path in _files(root): + if path.suffix not in XML_SUFFIXES: + continue + try: + ET.parse(path) + except (OSError, ET.ParseError) as exc: + errors.append(f"invalid XML {path.relative_to(root)}: {exc}") + return errors + + +def validate_python(root: Path) -> list[str]: + errors: list[str] = [] + for path in _files(root): + if path.suffix != ".py": + continue + try: + source = path.read_text(encoding="utf-8") + compile(source, str(path), "exec") + except (OSError, UnicodeError, SyntaxError) as exc: + errors.append(f"invalid Python {path.relative_to(root)}: {exc}") + return errors + + +def detect_duplicate_xml_registrations(root: Path) -> list[str]: + """Find exact repeated direct-child registrations in generated XML. + + Exact element identity is deliberately conservative: it catches hooks that + append the same registration twice without treating similar, valid + registrations as duplicates. + """ + errors: list[str] = [] + for path in _files(root): + if path.suffix not in XML_SUFFIXES: + continue + try: + tree = ET.parse(path) + except (OSError, ET.ParseError): + continue + for parent in tree.iter(): + serialized = [ET.tostring(child, encoding="unicode") for child in parent] + for element, count in Counter(serialized).items(): + if count > 1: + preview = " ".join(element.split())[:160] + errors.append( + f"duplicate XML registration ({count}x) " + f"{path.relative_to(root)}: {preview}" + ) + return errors + + +def validate_project(root: Path) -> dict[str, list[str]]: + """Run all deterministic, install-free generated-project checks.""" + return { + "toml": validate_toml(root), + "xml": validate_xml(root), + "python": validate_python(root), + "duplicate_xml": detect_duplicate_xml_registrations(root), + } diff --git a/plonecli/cli.py b/plonecli/cli.py index 3ef604f..bb53390 100644 --- a/plonecli/cli.py +++ b/plonecli/cli.py @@ -11,7 +11,6 @@ from pathlib import Path import click -from click_aliases import ClickAliasedGroup from plonecli.config import load_config, save_config from plonecli.exceptions import NoSuchValue, NotInPackageError @@ -186,7 +185,7 @@ def get_templates(ctx, args, incomplete): return [k for k in templates if incomplete in k] -class ClickFilteredAliasedGroup(ClickAliasedGroup): +class ClickFilteredGroup(click.Group): def list_commands(self, ctx): existing_cmds = super().list_commands(ctx) project = find_project_root() @@ -200,7 +199,7 @@ def list_commands(self, ctx): @click.group( - cls=ClickFilteredAliasedGroup, + cls=ClickFilteredGroup, chain=True, context_settings={"help_option_names": ["-h", "--help"]}, invoke_without_command=True, @@ -351,6 +350,11 @@ def create(context, template, name, data, data_file, defaults, no_git, allow_dir if committed: echo(f" Committed: {committed}", fg="green") context.obj["target_dir"] = name + context.obj["chain_defaults"] = defaults + context.obj["chain_allow_dirty"] = allow_dirty + # Chained commands share the group context created before generation. Refresh + # it now so ``create ... setup`` and similar cross-context chains work. + context.obj["project"] = find_project_root(Path(name)) @cli.command(cls=InterspersedCommand) @@ -400,11 +404,15 @@ def add(context, template, data, data_file, defaults, no_git, allow_dirty): echo(f" Committed: {committed}", fg="green") -@cli.command() +@cli.command(cls=InterspersedCommand) @template_run_options @click.pass_context def setup(context, data, data_file, defaults, allow_dirty): """Run zope-setup inside an existing backend_addon""" + # Click's chained parser may bind shared trailing flags to the preceding + # create command. Carry those execution flags across the chain. + defaults = defaults or context.obj.get("chain_defaults", False) + allow_dirty = allow_dirty or context.obj.get("chain_allow_dirty", False) project = context.obj.get("project") if project is None: raise NotInPackageError(context.command.name) diff --git a/plonecli/git.py b/plonecli/git.py index 73ee67e..11acb66 100644 --- a/plonecli/git.py +++ b/plonecli/git.py @@ -17,17 +17,18 @@ def is_git_repo(path: Path) -> bool: - """Return True if ``path`` is inside a git working tree.""" + """Return True if ``path`` is the root of a git working tree.""" try: - subprocess.run( - ["git", "rev-parse", "--git-dir"], + result = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], cwd=str(path), check=True, capture_output=True, + text=True, ) - return True except (subprocess.CalledProcessError, FileNotFoundError): return False + return Path(result.stdout.strip()).resolve() == path.resolve() def dirty_files(path: str | Path) -> tuple[list[str], list[str]]: diff --git a/plonecli/project.py b/plonecli/project.py index 5d9de05..afae598 100644 --- a/plonecli/project.py +++ b/plonecli/project.py @@ -113,10 +113,13 @@ def find_project_root(start_dir: Path | None = None) -> ProjectContext | None: """Walk up directories looking for a Plone project. Detection order (first match wins): - 1. pyproject.toml with [tool.plone.backend_addon.settings] -> backend_addon - 2. pyproject.toml with [tool.plone.project.settings] -> zope-setup + 1. substantive backend-add-on settings -> backend_addon + 2. project settings -> zope-setup 3. bobtemplate.cfg with [main] template -> mapped project type (legacy) + A zope-setup project may contain a legacy marker-only backend settings + table. It is treated as zope-setup unless package identity is present. + Returns the first match found walking upward, or None. """ current = (start_dir or Path.cwd()).resolve() @@ -124,9 +127,13 @@ def find_project_root(start_dir: Path | None = None) -> ProjectContext | None: while True: pyproject_path = current / "pyproject.toml" if pyproject_path.exists(): - # Check for backend_addon first addon_settings = _read_backend_addon_settings(pyproject_path) - if addon_settings: + project_settings = _read_project_settings(pyproject_path) + is_addon = addon_settings and ( + addon_settings.get("package_name") + or addon_settings.get("package_folder") + ) + if is_addon: return ProjectContext( root_folder=current, project_type="backend_addon", @@ -135,8 +142,6 @@ def find_project_root(start_dir: Path | None = None) -> ProjectContext | None: package_folder=addon_settings.get("package_folder"), ) - # Check for zope-setup project - project_settings = _read_project_settings(pyproject_path) if project_settings: return ProjectContext( root_folder=current, @@ -144,6 +149,15 @@ def find_project_root(start_dir: Path | None = None) -> ProjectContext | None: settings=project_settings, ) + if addon_settings: + return ProjectContext( + root_folder=current, + project_type="backend_addon", + settings=addon_settings, + package_name=addon_settings.get("package_name"), + package_folder=addon_settings.get("package_folder"), + ) + # Check for legacy bobtemplate.cfg bobtemplate_path = current / "bobtemplate.cfg" if bobtemplate_path.exists(): diff --git a/pyproject.toml b/pyproject.toml index 1a0c2dd..6123502 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,7 +26,6 @@ classifiers = [ dependencies = [ "Click>=8.0", - "click-aliases", "copier>=9.0.0", "copier-templates-extensions", "packaging", diff --git a/tests/test_git.py b/tests/test_git.py index 4239388..e84a0bd 100644 --- a/tests/test_git.py +++ b/tests/test_git.py @@ -129,6 +129,20 @@ def test_is_git_repo_false_for_plain_dir(tmp_path): assert is_git_repo(tmp_path) is False +def test_parent_repository_is_not_used_for_generated_project(tmp_path): + """Git checks stay scoped to the generated project root.""" + config = PlonecliConfig() + (tmp_path / "outer.py").write_text("outer\n") + commit_template_changes(tmp_path, "outer", config, is_subtemplate=False) + (tmp_path / "outer.py").write_text("dirty outer\n") + project = tmp_path / "generated" + project.mkdir() + (project / "pyproject.toml").write_text("[project]\nname = \"generated\"\n") + + assert is_git_repo(project) is False + assert dirty_files(project) == ([], []) + + def test_dirty_files_clean_or_non_repo(tmp_path): # Not a repo at all. assert dirty_files(tmp_path) == ([], []) diff --git a/tests/test_plonecli.py b/tests/test_plonecli.py index 7be445f..102f362 100644 --- a/tests/test_plonecli.py +++ b/tests/test_plonecli.py @@ -165,6 +165,45 @@ def test_create_composite_via_alias( assert mock_run_create.call_count == 2 +@patch("plonecli.cli.find_project_root") +@patch("plonecli.cli.load_config") +@patch("plonecli.cli.run_create") +@patch("plonecli.cli.ensure_templates_cloned") +def test_create_then_setup_chain_refreshes_project( + mock_ensure, + mock_run_create, + mock_config, + mock_project, + runner, + tmp_path, +): + _make_template(tmp_path, "backend_addon", {"type": "main"}) + target = tmp_path / "my.addon" + mock_config.return_value = MagicMock(templates_dir=str(tmp_path), auto_commit=False) + mock_project.side_effect = lambda start=None: ( + project_at(target) if start is not None else None + ) + + result = runner.invoke( + cli, + [ + "create", + "backend_addon", + str(target), + "--defaults", + "setup", + "--defaults", + ], + ) + + assert result.exit_code == 0, result.output + assert [call.args[0] for call in mock_run_create.call_args_list] == [ + "backend_addon", + "zope-setup", + ] + assert mock_run_create.call_args_list[1].args[1] == str(target) + + @patch("plonecli.cli.find_project_root", return_value=None) @patch("plonecli.cli.load_config") def test_create_unknown_template(mock_config, mock_project, runner, tmp_path): diff --git a/tests/test_project.py b/tests/test_project.py index efed1c1..494d446 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -78,8 +78,8 @@ def test_find_project_ignores_plain_pyproject(tmp_path): assert ctx is None -def test_backend_addon_takes_priority(tmp_path): - """If both sections exist, backend_addon is detected first.""" +def test_substantive_backend_addon_takes_priority(tmp_path): + """A mixed add-on project keeps backend feature templates available.""" pyproject = tmp_path / "pyproject.toml" pyproject.write_text("""\ [project] @@ -96,6 +96,27 @@ def test_backend_addon_takes_priority(tmp_path): assert ctx.project_type == "backend_addon" +def test_project_settings_beat_marker_only_backend_settings(tmp_path): + """Standalone Zope projects ignore a stale zope_setup marker table.""" + pyproject = tmp_path / "pyproject.toml" + pyproject.write_text("""\ +[project] +name = "zope-project" + +[tool.plone.backend_addon.settings] +zope_setup = true + +[tool.plone.project.settings] +plone_version = "6.1.1" +""") + + ctx = find_project_root(tmp_path) + + assert ctx is not None + assert ctx.project_type == "zope-setup" + assert ctx.settings["project_name"] == "zope-project" + + def _make_bobtemplate_cfg(path, template="plone_addon"): """Create a legacy bobtemplate.cfg.""" cfg = path / "bobtemplate.cfg" diff --git a/tests/test_theme_barceloneta_integration.py b/tests/test_theme_barceloneta_integration.py index 859ecde..593701d 100644 --- a/tests/test_theme_barceloneta_integration.py +++ b/tests/test_theme_barceloneta_integration.py @@ -87,7 +87,14 @@ def test_theme_barceloneta_generates_and_tests_pass(tmp_path: Path) -> None: ) # The template ships a theme test keyed on theme_id. - theme_test = project_dir / "tests" / "test_theme_my_test_theme.py" + theme_test = ( + project_dir + / "src" + / "collective" + / "mythemetest" + / "tests" + / "test_theme_my_test_theme.py" + ) assert theme_test.exists(), f"theme test not generated: {theme_test}" # 3. Build the project with uv sync + run its pytest suite. This is the @@ -103,7 +110,7 @@ def test_theme_barceloneta_generates_and_tests_pass(tmp_path: Path) -> None: ) result = subprocess.run( - ["uv", "run", "--extra", "test", "pytest", "tests/", "-x", "-q"], + ["uv", "run", "--extra", "test", "pytest", "-x", "-q"], cwd=project_dir, env=env, capture_output=True, diff --git a/uv.lock b/uv.lock index ecdfc0e..7ed6ecc 100644 --- a/uv.lock +++ b/uv.lock @@ -152,18 +152,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/e4/20/71885d8b97d4f3dde17b1fdb92dbd4908b00541c5a3379787137285f602e/click-8.3.2-py3-none-any.whl", hash = "sha256:1924d2c27c5653561cd2cae4548d1406039cb79b858b747cfea24924bbc1616d", size = 108379, upload-time = "2026-04-03T19:14:43.505Z" }, ] -[[package]] -name = "click-aliases" -version = "1.0.5" -source = { registry = "https://pypi.org/simple" } -dependencies = [ - { name = "click" }, -] -sdist = { url = "https://files.pythonhosted.org/packages/40/d8/adbaeadc13c9686b9bda8b4c50e5a3983f504faae2ffbea5165d5beb1cdb/click_aliases-1.0.5.tar.gz", hash = "sha256:e37d4cabbaad68e1c48ec0f063a59dfa15f0e7450ec901bd1ce4f4b954bc881d", size = 3105, upload-time = "2024-10-17T15:44:19.056Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/3d/1a/d5e29a6f896293e32ab3e63201df5d396599e57a726575adaafbcd9d70a6/click_aliases-1.0.5-py3-none-any.whl", hash = "sha256:cbb83a348acc00809fe18b6da13a7f6307bc71b3c5f69cc730e012dfb4bbfdc3", size = 3524, upload-time = "2024-10-17T15:44:17.389Z" }, -] - [[package]] name = "colorama" version = "0.4.6" @@ -579,11 +567,10 @@ wheels = [ [[package]] name = "plonecli" -version = "7.0.0b14.dev0" +version = "7.0.0b15.dev0" source = { editable = "." } dependencies = [ { name = "click" }, - { name = "click-aliases" }, { name = "copier" }, { name = "copier-templates-extensions" }, { name = "packaging" }, @@ -609,7 +596,6 @@ test = [ [package.metadata] requires-dist = [ { name = "click", specifier = ">=8.0" }, - { name = "click-aliases" }, { name = "copier", specifier = ">=9.0.0" }, { name = "copier-templates-extensions" }, { name = "myst-parser", marker = "extra == 'docs'" }, From 447280eb112f4ccdb5a56266758740c627b5cd6e Mon Sep 17 00:00:00 2001 From: MrTango Date: Thu, 13 Aug 2026 20:22:57 +0000 Subject: [PATCH 2/3] Allow explicitly requested dev servers --- plonecli/skills/plonecli/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plonecli/skills/plonecli/SKILL.md b/plonecli/skills/plonecli/SKILL.md index 07fa710..34d54ff 100644 --- a/plonecli/skills/plonecli/SKILL.md +++ b/plonecli/skills/plonecli/SKILL.md @@ -46,7 +46,7 @@ On first run, plonecli clones the copier-templates to `~/.copier-templates/plone - **Scaffold Plone features with plonecli — don't hand-write them.** The moment a task or plan calls for a behavior, content type, view, viewlet, portlet, vocabulary, indexer, subscriber, control panel, form, REST API service, theme, upgrade step, or any other Plone artifact, the first step is `plonecli add ` (inside the addon), not creating `.py`/`.zcml`/`.xml` files by hand. The templates wire up registration, profiles, and tests correctly; hand-rolled files miss those. Only fall back to manual edits for the legacy-package case below, and even then only the minimal hooks plonecli needs. - **`create`/`add` are interactive by default — always run them non-interactively here.** copier opens prompts you cannot answer in Claude Code / CI (they hang or fail). Pass `--defaults` (use template defaults for unasked questions) plus `-d/--data KEY=VALUE` (repeatable) for each answer the user specified — or `--data-file PATH` for a YAML/JSON file of answers (`-d` overrides matching keys). Required answers without a default (`content_type_name`, `behavior_name`, `service_name`, `upgrade_step_title`) **must** be given via `-d`. **The full catalogue of every template's `-d` keys, defaults, choices, conditional/computed answers and which are required is [reference/templates.md](reference/templates.md)** — consult it to build the invocation instead of guessing or triggering a prompt. Example: `plonecli add upgrade_step --defaults -d upgrade_step_title="Reimport viewlets"`. **Never give up on a prompt and hand-write the files the subtemplate would generate** — drive plonecli non-interactively instead. Don't invoke `copier` directly. See [reference/add.md](reference/add.md). -- **Never start the dev server yourself.** Do not run `plonecli serve` / `plonecli debug` / `invoke start`. Assume the instance is already running; if it is not, ask the user to start it. (`plonecli test` is fine to run.) +- **Do not start the dev server by default.** Assume the instance is already running. Only run `plonecli serve`, `plonecli debug`, or `invoke start` when the user explicitly asks you to start it or clearly authorizes you to do so. Otherwise, if no instance is running, ask the user to start it. (`plonecli test` is fine to run.) - **Use native `uv`.** Run things as `uv run `; never `uv pip` or `pip` unless explicitly told. - **Tests must pass — never skip them.** After scaffolding or adding a feature, run `plonecli test` and report real results. - **Profile XML changes need an upgrade step — scaffold it automatically.** Whenever you edit GenericSetup profile XML under `profiles/default/` (e.g. `catalog.xml`, `types/*.xml`, `types.xml`, `workflows.xml`, `registry.xml`, `rolemap.xml`) in a way that must propagate to already-installed sites, run `plonecli add upgrade_step --defaults -d upgrade_step_title=""` as part of the same change — don't leave it to the user to remember. It bumps `profiles/default/metadata.xml` and registers a GS upgrade handler; then fill that handler so existing sites actually get the change (reapply the relevant import step or migrate data). Never hand-edit `metadata.xml`'s version to "do an upgrade" — that bumps the number without a registered step. Details and what does/doesn't need a step: [reference/add.md](reference/add.md). From bb03d3e6d5a5dde96e2e9a9885664e4011027f5a Mon Sep 17 00:00:00 2001 From: MrTango Date: Fri, 14 Aug 2026 16:37:02 +0000 Subject: [PATCH 3/3] Harden evaluation harnesses - isolate nested uv template invocations from the root virtualenv - checkpoint eval steps so chained commands start from a clean tree - handle byte output from timed-out subprocesses - ignore XML formatting tails in duplicate-registration detection - prune dependency dirs while walking generated projects - fail skill eval runs on process errors and baseline skill leaks - add fast unit tests for both harnesses --- CHANGES.md | 7 ++- evals/scaffolding/EVALUATION.md | 25 ++++++++-- evals/scaffolding/run_evals.py | 69 ++++++++++++++++++++++---- evals/scaffolding/validators.py | 61 ++++++++++++++++------- evals/skill/README.md | 6 +-- evals/skill/run_evals.py | 47 +++++++++++++----- tests/test_eval_harness.py | 86 +++++++++++++++++++++++++++++++++ tests/test_skill_evals.py | 7 +-- 8 files changed, 259 insertions(+), 49 deletions(-) create mode 100644 tests/test_eval_harness.py diff --git a/CHANGES.md b/CHANGES.md index d1e6404..154bf1c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,7 +3,12 @@ ## 7.0.0b15 (unreleased) -- Nothing changed yet. +- Allow the plonecli skill to start the development server when the user + explicitly requests it. + [MrTango] + +- Harden scaffolding evaluation and remove false template Git warnings. + [MrTango] ## 7.0.0b14 (2026-08-13) diff --git a/evals/scaffolding/EVALUATION.md b/evals/scaffolding/EVALUATION.md index b825443..1830ff8 100644 --- a/evals/scaffolding/EVALUATION.md +++ b/evals/scaffolding/EVALUATION.md @@ -1,7 +1,9 @@ # Scaffolding evaluation findings -> Historical baseline from before the fixes. The current full report passes -> 245/245 cases; see the ignored `results/report.md` generated on 2026-08-13. +> Historical baseline from before the fixes. A full verification run passed +> 245/245 cases with no warnings on 2026-08-13. The ignored +> `results/report.md` is mutable and may instead contain the latest quick or +> CI-validation run. ## Scope @@ -61,7 +63,24 @@ Update the assertion and pytest target to the generated `src//tests/` l - Feature generation inside these nested, `--no-git` workspaces reports the outer plonecli repository as dirty. Git cleanliness checks should be scoped to the detected generated project rather than walking into an unrelated parent repository. - Keep the generated TOML/XML/Python validators as CI checks. They found failures that successful Copier exit codes did not detect. -## Test receipts +## Resolution + +All findings above have been addressed: + +- free-text TOML values use serialization filters; +- standalone Zope projects are detected correctly; +- chained `create` → `setup` refreshes project context; +- theme variants reject conflicting overlays; +- the Barceloneta integration test uses the generated package test path; +- context hooks use the current in-place API; +- the deprecated command-alias dependency was removed; +- Git checks are scoped to the generated project; +- subtemplate validation tasks use Copier's `_copier_operation` value and no + longer report files generated earlier in the same copy as pre-existing + changes; +- generated TOML/XML/Python validation runs in CI. + +## Baseline test receipts - Root unit suite: **209 passed, 16 skipped**. - Copier-template unit suite: **386 passed, 2 integration tests deselected**. diff --git a/evals/scaffolding/run_evals.py b/evals/scaffolding/run_evals.py index d763642..d4e2c69 100644 --- a/evals/scaffolding/run_evals.py +++ b/evals/scaffolding/run_evals.py @@ -948,20 +948,32 @@ def build_cases(quick: bool) -> tuple[list[Case], dict[str, int]]: return cases, planned +def _subprocess_text(value: str | bytes | None) -> str: + """Normalize subprocess output, including TimeoutExpired byte payloads.""" + if value is None: + return "" + if isinstance(value, bytes): + return value.decode(errors="replace") + return value + + def run_command( step: Step, env: dict[str, str], timeout_seconds: int ) -> dict[str, Any]: - command = ( - ["uv", *step.args[1:]] - if step.args and step.args[0] == "__uv__" - else [*CLI, *step.args] - ) + is_direct_uv = bool(step.args and step.args[0] == "__uv__") + command = ["uv", *step.args[1:]] if is_direct_uv else [*CLI, *step.args] + command_env = env + if is_direct_uv: + # The template checkout has its own uv project. Do not leak the root + # project's active environment into that nested invocation. + command_env = dict(env) + command_env.pop("VIRTUAL_ENV", None) started = time.monotonic() try: completed = subprocess.run( command, cwd=step.cwd, - env=env, + env=command_env, text=True, capture_output=True, stdin=subprocess.DEVNULL, @@ -973,8 +985,10 @@ def run_command( timed_out = False except subprocess.TimeoutExpired as exc: exit_code = 124 - stdout = exc.stdout or "" - stderr = (exc.stderr or "") + f"\nTimed out after {timeout_seconds}s\n" + stdout = _subprocess_text(exc.stdout) + stderr = ( + _subprocess_text(exc.stderr) + f"\nTimed out after {timeout_seconds}s\n" + ) timed_out = True return { "command": command, @@ -987,6 +1001,41 @@ def run_command( } +def _checkpoint_project(project: Path, step_number: int) -> None: + """Commit an intermediate eval step so the next command starts clean.""" + status = subprocess.run( + ["git", "status", "--porcelain"], + cwd=project, + check=True, + capture_output=True, + text=True, + stdin=subprocess.DEVNULL, + ) + if not status.stdout.strip(): + return + subprocess.run( + ["git", "add", "-A"], + cwd=project, + check=True, + stdin=subprocess.DEVNULL, + ) + subprocess.run( + [ + "git", + "-c", + "user.name=Evaluation Runner", + "-c", + "user.email=eval@example.invalid", + "commit", + "-qm", + f"Evaluation step {step_number}", + ], + cwd=project, + check=True, + stdin=subprocess.DEVNULL, + ) + + def execute_case( case: Case, env: dict[str, str], timeout_seconds: int ) -> dict[str, Any]: @@ -1048,7 +1097,7 @@ def execute_case( commands = [] errors = [] observations: list[str] = [] - for step in case.steps: + for step_number, step in enumerate(case.steps, 1): step.cwd.mkdir(parents=True, exist_ok=True) result = run_command(step, env, timeout_seconds) commands.append(result) @@ -1062,6 +1111,8 @@ def execute_case( if result["exit_code"] != 0: errors.append(f"command {len(commands)} exited {result['exit_code']}") break + if case.project and step_number < len(case.steps): + _checkpoint_project(case.project, step_number) validation: dict[str, list[str]] = {} if case.project and case.project.exists(): diff --git a/evals/scaffolding/validators.py b/evals/scaffolding/validators.py index 3573e0c..2a3c8c5 100644 --- a/evals/scaffolding/validators.py +++ b/evals/scaffolding/validators.py @@ -2,9 +2,11 @@ from __future__ import annotations +import os import tomllib import xml.etree.ElementTree as ET from collections import Counter +from collections.abc import Iterable from pathlib import Path IGNORED_PARTS = {".git", ".venv", "node_modules", "__pycache__"} @@ -12,14 +14,16 @@ def _files(root: Path): - for path in root.rglob("*"): - if path.is_file() and not (set(path.parts) & IGNORED_PARTS): - yield path + """Yield project files while pruning generated dependency directories.""" + for directory, dirnames, filenames in os.walk(root): + dirnames[:] = [name for name in dirnames if name not in IGNORED_PARTS] + base = Path(directory) + yield from (base / name for name in filenames) -def validate_toml(root: Path) -> list[str]: +def validate_toml(root: Path, files: Iterable[Path] | None = None) -> list[str]: errors: list[str] = [] - for path in _files(root): + for path in files if files is not None else _files(root): if path.suffix != ".toml": continue try: @@ -30,9 +34,9 @@ def validate_toml(root: Path) -> list[str]: return errors -def validate_xml(root: Path) -> list[str]: +def validate_xml(root: Path, files: Iterable[Path] | None = None) -> list[str]: errors: list[str] = [] - for path in _files(root): + for path in files if files is not None else _files(root): if path.suffix not in XML_SUFFIXES: continue try: @@ -42,9 +46,9 @@ def validate_xml(root: Path) -> list[str]: return errors -def validate_python(root: Path) -> list[str]: +def validate_python(root: Path, files: Iterable[Path] | None = None) -> list[str]: errors: list[str] = [] - for path in _files(root): + for path in files if files is not None else _files(root): if path.suffix != ".py": continue try: @@ -55,7 +59,22 @@ def validate_python(root: Path) -> list[str]: return errors -def detect_duplicate_xml_registrations(root: Path) -> list[str]: +def _element_identity(element: ET.Element, cache: dict[int, tuple]) -> tuple: + """Build a hashable subtree identity once, excluding formatting tails.""" + key = id(element) + if key not in cache: + cache[key] = ( + element.tag, + tuple(sorted(element.attrib.items())), + element.text, + tuple(_element_identity(child, cache) for child in element), + ) + return cache[key] + + +def detect_duplicate_xml_registrations( + root: Path, files: Iterable[Path] | None = None +) -> list[str]: """Find exact repeated direct-child registrations in generated XML. Exact element identity is deliberately conservative: it catches hooks that @@ -63,17 +82,24 @@ def detect_duplicate_xml_registrations(root: Path) -> list[str]: registrations as duplicates. """ errors: list[str] = [] - for path in _files(root): + for path in files if files is not None else _files(root): if path.suffix not in XML_SUFFIXES: continue try: tree = ET.parse(path) except (OSError, ET.ParseError): continue + cache: dict[int, tuple] = {} for parent in tree.iter(): - serialized = [ET.tostring(child, encoding="unicode") for child in parent] - for element, count in Counter(serialized).items(): + children_by_identity = { + _element_identity(child, cache): child for child in parent + } + counts = Counter(_element_identity(child, cache) for child in parent) + for identity, count in counts.items(): if count > 1: + element = ET.tostring( + children_by_identity[identity], encoding="unicode" + ) preview = " ".join(element.split())[:160] errors.append( f"duplicate XML registration ({count}x) " @@ -84,9 +110,10 @@ def detect_duplicate_xml_registrations(root: Path) -> list[str]: def validate_project(root: Path) -> dict[str, list[str]]: """Run all deterministic, install-free generated-project checks.""" + files = tuple(_files(root)) return { - "toml": validate_toml(root), - "xml": validate_xml(root), - "python": validate_python(root), - "duplicate_xml": detect_duplicate_xml_registrations(root), + "toml": validate_toml(root, files), + "xml": validate_xml(root, files), + "python": validate_python(root, files), + "duplicate_xml": detect_duplicate_xml_registrations(root, files), } diff --git a/evals/skill/README.md b/evals/skill/README.md index 8cae1fc..b7f231b 100644 --- a/evals/skill/README.md +++ b/evals/skill/README.md @@ -41,11 +41,11 @@ python evals/skill/run_evals.py --mode both --cases restapi-implicit,upgrade-ste python evals/skill/run_evals.py --model haiku # cheaper smoke run ``` -Requires the `claude` CLI logged in. Runs bill real model usage — a full -`--mode both` sweep is ~16 agent runs. Sandboxes and transcripts land in +Requires the `claude` CLI logged in. Runs bill real model usage. Sandboxes and transcripts land in `/plonecli-skill-evals//` (outside the repo on purpose: a sandbox inside this repo lets the baseline agent *find* the skill by -searching the project); `results.json` there summarizes. Each run prints +searching the project); `results.json` there summarizes. A full `--mode both` +sweep is 24 agent runs. Each run prints `skills fired: [...]` — in `noskill` mode it must be `none`, anything else means a skill leaked into the baseline. diff --git a/evals/skill/run_evals.py b/evals/skill/run_evals.py index 1f083e7..8124241 100644 --- a/evals/skill/run_evals.py +++ b/evals/skill/run_evals.py @@ -275,8 +275,8 @@ class Case: ), file_has( "collective.demo/src/collective/demo/profiles/uninstall/catalog.xml", - r'is_featured(?s).*remove="True"|remove="True"(?s).*is_featured', - "uninstall catalog.xml removes the index (remove=\"True\")", + r'(?s)]*\bname=["\']is_featured["\'])(?=[^>]*\bremove=["\']True["\'])[^>]*>', + 'uninstall catalog.xml removes the index (remove="True")', ), ], notes=( @@ -384,12 +384,21 @@ def prepare_sandbox(case, mode, root, skill_src): (sandbox / ".eval").mkdir() # Own git repo: pins the nested agent's project root to the sandbox # (no searching upward/sideways) and lets the shim's auto-commit work. - subprocess.run(["git", "init", "-q"], cwd=sandbox, check=False) - subprocess.run(["git", "add", "-A"], cwd=sandbox, check=False) + subprocess.run(["git", "init", "-q"], cwd=sandbox, check=True) + subprocess.run(["git", "add", "-A"], cwd=sandbox, check=True) subprocess.run( - ["git", "commit", "-qm", "fixture"], + [ + "git", + "-c", + "user.name=Evaluation Runner", + "-c", + "user.email=eval@example.invalid", + "commit", + "-qm", + "fixture", + ], cwd=sandbox, - check=False, + check=True, capture_output=True, ) return sandbox @@ -440,8 +449,10 @@ def run_case(case, mode, root, model, skill_src, config_dir): ) result.error = f"timeout after {RUN_TIMEOUT}s" - (sandbox / ".eval" / "transcript.jsonl").write_text(result.transcript) - result.log = log_file.read_text() + (sandbox / ".eval" / "transcript.jsonl").write_text( + result.transcript, encoding="utf-8" + ) + result.log = log_file.read_text(encoding="utf-8") result.skills_fired = tuple( sorted( set( @@ -473,6 +484,13 @@ def grade(case, result): return rows +def run_passed(mode, result, rows): + """Require a clean process exit, passing checks, and an isolated baseline.""" + checks_passed = all(ok for _, ok, _ in rows) + baseline_clean = mode != "noskill" or not result.skills_fired + return not result.error and checks_passed and baseline_clean + + def main(): ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) ap.add_argument("--cases", help="comma-separated case ids (default: all)") @@ -518,6 +536,9 @@ def main(): if args.runs_dir else Path(tempfile.gettempdir()) / "plonecli-skill-evals" / stamp ) + root = root.resolve() + if root == REPO.resolve() or REPO.resolve() in root.parents: + ap.error("--runs-dir must be outside the plonecli repository") root.mkdir(parents=True, exist_ok=True) config_dir = make_eval_config(root) @@ -538,7 +559,7 @@ def main(): case, mode, root, args.model or None, skill_src, config_dir ) rows = grade(case, result) - passed = all(ok for _, ok, _ in rows) + passed = run_passed(mode, result, rows) for desc, ok, extra in rows: mark = "PASS" if ok else "FAIL" print(f" [{mark}] {desc}{' ' + extra if extra else ''}") @@ -560,7 +581,9 @@ def main(): } ) - (root / "results.json").write_text(json.dumps(summary, indent=2)) + (root / "results.json").write_text( + json.dumps(summary, indent=2) + "\n", encoding="utf-8" + ) print(f"\nResults: {root / 'results.json'}") n_skill = [s for s in summary if s["mode"] == "skill"] if n_skill: @@ -571,7 +594,9 @@ def main(): print( f"baseline : {sum(s['passed'] for s in n_base)}/{len(n_base)} cases passed" ) - return 0 if all(s["passed"] for s in n_skill) else 1 + baseline_clean = all(not s["skills_fired"] for s in n_base) + evaluated = n_skill or n_base + return 0 if all(s["passed"] for s in evaluated) and baseline_clean else 1 if __name__ == "__main__": diff --git a/tests/test_eval_harness.py b/tests/test_eval_harness.py new file mode 100644 index 0000000..992a8c1 --- /dev/null +++ b/tests/test_eval_harness.py @@ -0,0 +1,86 @@ +"""Fast correctness tests for the evaluation harnesses.""" + +import importlib.util +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent + + +def _load_module(name, path): + spec = importlib.util.spec_from_file_location(name, path) + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + assert spec.loader is not None + spec.loader.exec_module(module) + return module + + +def test_scaffolding_validator_ignores_xml_tail_whitespace(tmp_path): + validators = _load_module( + "scaffolding_validators", + ROOT / "evals" / "scaffolding" / "validators.py", + ) + (tmp_path / "configure.zcml").write_text( + '\n ' + '', + encoding="utf-8", + ) + + errors = validators.detect_duplicate_xml_registrations(tmp_path) + + assert len(errors) == 1 + assert "duplicate XML registration (2x)" in errors[0] + + +def test_scaffolding_timeout_output_accepts_bytes(monkeypatch): + scaffolding_dir = ROOT / "evals" / "scaffolding" + monkeypatch.syspath_prepend(str(scaffolding_dir)) + runner = _load_module( + "scaffolding_run_evals", + scaffolding_dir / "run_evals.py", + ) + + assert runner._subprocess_text(b"partial \xff") == "partial �" + assert runner._subprocess_text(None) == "" + + +def test_skill_uninstall_check_requires_name_and_remove_on_same_index(tmp_path): + runner = _load_module( + "skill_run_evals", + ROOT / "evals" / "skill" / "run_evals.py", + ) + package = tmp_path / "collective.demo" / "src" / "collective" / "demo" + default = package / "profiles" / "default" + uninstall = package / "profiles" / "uninstall" + default.mkdir(parents=True) + uninstall.mkdir(parents=True) + (default / "catalog.xml").write_text("") + uninstall_xml = uninstall / "catalog.xml" + case = next(case for case in runner.CASES if case.id == "uninstall-mirror") + result = runner.RunResult(sandbox=tmp_path) + + uninstall_xml.write_text('') + rows = runner.grade(case, result) + assert all(ok for _, ok, _ in rows), rows + + uninstall_xml.write_text( + '' + '' + ) + rows = runner.grade(case, result) + assert rows[1][1] is False + + +def test_skill_run_errors_and_baseline_leaks_fail_the_run(tmp_path): + runner = _load_module( + "skill_run_evals_pass", + ROOT / "evals" / "skill" / "run_evals.py", + ) + passing_rows = [("check", True, "")] + + crashed = runner.RunResult(sandbox=tmp_path, error="timeout") + leaked = runner.RunResult(sandbox=tmp_path, skills_fired=("plonecli",)) + + assert runner.run_passed("skill", crashed, passing_rows) is False + assert runner.run_passed("noskill", leaked, passing_rows) is False diff --git a/tests/test_skill_evals.py b/tests/test_skill_evals.py index 95a31f0..c2b6714 100644 --- a/tests/test_skill_evals.py +++ b/tests/test_skill_evals.py @@ -70,8 +70,5 @@ def test_skill_case(case, eval_env): eval_env["config"], ) failed = [desc for desc, ok, extra in run_evals.grade(case, result) if not ok] - assert not failed, ( - f"failed checks: {failed}" - f"{'; run error: ' + result.error if result.error else ''}" - f"; sandbox: {result.sandbox}" - ) + assert not result.error, f"run error: {result.error}; sandbox: {result.sandbox}" + assert not failed, f"failed checks: {failed}; sandbox: {result.sandbox}"