From 30892017f70c48b227fcdb9eeee011720d85f21e Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 1 Aug 2026 05:36:38 +0200 Subject: [PATCH] ci(skills): lint, type-check and test the Python under skills/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1053. Nothing checked the Python in `skills/`. The workspace checks iterate over `[tool.uv.workspace] members` and every member lived under `tools/`, so ruff, mypy, and pytest never saw `skills/**`. Eight helper scripts and guards sat there unchecked, and #1049 added the first test file under the tree — nine tests that CI would never have run. Reverting `pr_link.py` would have left the checks green. Declares `skills/` as one workspace member whose `pyproject.toml` is a config carrier rather than a package (`package = false`). The existing machinery does the rest: `run-workspace-check.sh` discovers which checks apply from the sections present, and `tests.yml` derives its matrix from the members list, so this adds a `pytest (skills)` job with no workflow edit. Per-skill packaging was the alternative and is worse: skills are symlinked into adopter repos one directory at a time, so build metadata inside each would leak into every adopter. `skills/pyproject.toml` is not symlinked — the relays point at `skills/`, never at `skills/` itself. Turning the checks on found seven real things, all fixed here: - `scan_ci_runners.py` — `zip()` without `strict=`; two same-named `futures`/`future` bindings of different types in one function, which is why the second needed renaming before mypy could annotate it; an unannotated `rows`; and `step.get("with")` called twice so the isinstance narrowing did not stick. - `collect_status.py` — `l` as a variable name, and a mixed-value dict literal inferred as `dict[str, object]`, which made the later indexed assignment invalid. - `test_pr_link.py` — a stale `# noqa: E402` that no longer suppressed anything. None of these change behaviour; they are the annotations and renames the checks require, which is the point — they were invisible until now. `ruff format` is skipped for this member via `[tool.magpie.checks]`. It wants to reflow ~280 lines across six pre-existing scripts, none of them touched here, and that churn would bury the change behind a mechanical reformat. Lint, types and tests all run; formatting can land separately and the skip removed. Verified the coverage is real rather than nominal: breaking the OSC 8 emission in `pr_link.py` now fails three tests under the workspace runner, where before the same break produced no signal at all. Generated-by: Claude Code (Opus 5) --- pyproject.toml | 1 + .../scripts/scan_ci_runners.py | 18 +-- .../tests/test_pr_link.py | 2 +- skills/pyproject.toml | 111 ++++++++++++++++++ skills/setup-status/scripts/collect_status.py | 9 +- uv.lock | 22 ++++ 6 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 skills/pyproject.toml diff --git a/pyproject.toml b/pyproject.toml index 82fca5352..e2959a4bd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -95,6 +95,7 @@ package = false [tool.uv.workspace] members = [ "ai-tutors", + "skills", "tools/agent-guard", "tools/agent-isolation", "tools/bitbucket", diff --git a/skills/ci-runner-audit/scripts/scan_ci_runners.py b/skills/ci-runner-audit/scripts/scan_ci_runners.py index 45c9ad640..c83fe511a 100755 --- a/skills/ci-runner-audit/scripts/scan_ci_runners.py +++ b/skills/ci-runner-audit/scripts/scan_ci_runners.py @@ -26,8 +26,9 @@ import re import subprocess import sys -from concurrent.futures import ThreadPoolExecutor, as_completed +from concurrent.futures import Future, ThreadPoolExecutor, as_completed from pathlib import Path +from typing import Any from urllib.request import urlopen try: @@ -177,9 +178,11 @@ def load_workflows_for_repos(repo_names: list[str], workers: int) -> list[dict]: repos.append(repo) workflows: list[dict] = [] with ThreadPoolExecutor(max_workers=workers) as executor: - futures = [executor.submit(list_workflows_for_repo, repo) for repo in repos] - for future in as_completed(futures): - workflows.extend(future.result()) + workflow_futures: list[Future[list[dict]]] = [ + executor.submit(list_workflows_for_repo, repo) for repo in repos + ] + for workflow_future in as_completed(workflow_futures): + workflows.extend(workflow_future.result()) return sorted(workflows, key=lambda row: (row["repo"], row["path"])) @@ -199,8 +202,8 @@ def matrix_rows(matrix: object) -> list[dict]: continue keys.append(str(key)) values.append(value if isinstance(value, list) else [value]) - rows = [{}] - for key, vals in zip(keys, values): + rows: list[dict[str, Any]] = [{}] + for key, vals in zip(keys, values, strict=True): rows = [{**row, key: val} for row in rows for val in vals] excludes = matrix.get("exclude") @@ -303,7 +306,8 @@ def arch_hits(workflow: dict) -> list[dict]: continue name = str(step.get("name", "")) uses = str(step.get("uses", "")) - action_inputs = step.get("with") if isinstance(step.get("with"), dict) else {} + raw_with = step.get("with") + action_inputs = raw_with if isinstance(raw_with, dict) else {} for key, value in action_inputs.items(): key_text = str(key).lower() value_text = " ".join(lower_values(value)) diff --git a/skills/pr-management-triage/tests/test_pr_link.py b/skills/pr-management-triage/tests/test_pr_link.py index 7005643ef..5d9af69f6 100644 --- a/skills/pr-management-triage/tests/test_pr_link.py +++ b/skills/pr-management-triage/tests/test_pr_link.py @@ -23,7 +23,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "scripts")) -import pr_link # noqa: E402 +import pr_link class PrLinkTest(unittest.TestCase): diff --git a/skills/pyproject.toml b/skills/pyproject.toml new file mode 100644 index 000000000..d0fae2544 --- /dev/null +++ b/skills/pyproject.toml @@ -0,0 +1,111 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 +# +# http://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. + +# Config carrier, not a package. `skills/` is a tree of agent-facing +# SKILL.md files; a handful of them ship helper `scripts/` and `guards/` +# written in Python. Before this file existed nothing linted, type-checked, +# or ran them: the workspace checks iterate over `[tool.uv.workspace] +# members` and every member lived under `tools/`, so `skills/**` was +# invisible to ruff, mypy, and pytest alike. +# +# Declaring the tree as one workspace member is the lightest fix that reuses +# the existing machinery — `tools/dev/run-workspace-check.sh` auto-discovers +# which checks apply from the sections below, and `.github/workflows/tests.yml` +# emits a `pytest (skills)` job for any member with a +# `[tool.pytest.ini_options]` section. Per-skill packaging was the alternative +# and is far heavier: skills are symlinked into adopter repos one directory at +# a time, so build metadata inside each of them would leak into every adopter. +# This file is not symlinked — the relays point at `skills/`, never at +# `skills/` itself. + +[project] +name = "magpie-skills" +version = "0.1.0" +description = "Helper scripts and guards shipped alongside the agent-facing skill definitions in skills/." +requires-python = ">=3.11" +license = { text = "Apache-2.0" } +# stdlib-only — the helper scripts deliberately carry no runtime dependencies +# so an adopter can run them without installing anything. +dependencies = [] + +# Not an installable package: these are scripts invoked by path from a skill, +# not a library anything imports. +[tool.uv] +package = false + +# `ruff format` is deliberately skipped for now. The formatter wants to reflow +# roughly 280 lines across six pre-existing scripts — long set literals, long +# regex constants — none of which this change touches. Folding that churn in +# here would bury the point of the change (turning the checks on) behind a +# mechanical reformat nobody can review line by line. Lint, types, and tests +# all run; formatting should land as its own commit, after which this skip can +# be deleted. +[tool.magpie.checks] +skip = ["ruff-format"] + +[tool.ruff] +line-length = 110 +target-version = "py311" + +[tool.ruff.lint] +select = [ + "E", # pycodestyle errors + "W", # pycodestyle warnings + "F", # pyflakes + "I", # isort + "B", # flake8-bugbear + "UP", # pyupgrade + "SIM", # flake8-simplify + "C4", # flake8-comprehensions + "RUF", # ruff-specific +] +ignore = [ + "E501", # line-too-long — the 110-char limit above is already generous +] + +[tool.mypy] +python_version = "3.11" +files = ["."] +ignore_missing_imports = true +warn_unused_ignores = true +warn_redundant_casts = true +check_untyped_defs = true +no_implicit_optional = true +# Helper scripts and their tests are plain functions invoked by path, not a +# typed library surface, so the annotation requirements the `tools/` members +# apply would be noise here. `check_untyped_defs` above still type-checks the +# bodies, which is where the value is. +disallow_untyped_defs = false +disallow_incomplete_defs = false + +[tool.pytest.ini_options] +minversion = "8.0" +addopts = "-ra -q" +# Collected from the whole tree: tests live next to the skill they cover +# (`skills//tests/`) rather than in one central directory. +testpaths = ["."] + +[dependency-groups] +# Shared static-analysis + test toolchain, pinned to the same versions as the +# workspace root so every sub-project's own environment is self-contained. +# The workspace checks run each tool via `uv run --directory +# --project . python -m ` — see tools/dev/run-workspace-check.sh. +dev = [ + "mypy>=2.3.0", + "pytest>=9.1.1", + "ruff>=0.15.21", +] diff --git a/skills/setup-status/scripts/collect_status.py b/skills/setup-status/scripts/collect_status.py index 4e752e9f2..b6495f227 100644 --- a/skills/setup-status/scripts/collect_status.py +++ b/skills/setup-status/scripts/collect_status.py @@ -45,6 +45,7 @@ import subprocess import sys from pathlib import Path +from typing import Any # The agent-target registry is owned by skills/setup/agents.md # ("## The registry") — the single source of truth. At runtime the @@ -283,9 +284,9 @@ def compute_drift(committed: dict | None, local: dict | None) -> dict: ("ref", committed.get("ref"), local.get("source_ref")), ] mismatches = [ - {"field": f, "committed": c, "local": l} - for f, c, l in pairs - if c is not None and l is not None and c != l + {"field": field, "committed": committed, "local": local} + for field, committed, local in pairs + if committed is not None and local is not None and committed != local ] return { "checked": True, @@ -299,7 +300,7 @@ def gitignore_coverage(root: Path, targets: list[dict]) -> dict: gi = root / ".gitignore" text = gi.read_text(encoding="utf-8") if gi.is_file() else "" lines = {ln.strip() for ln in text.splitlines()} - cov = { + cov: dict[str, Any] = { "present": gi.is_file(), "snapshot_ignored": "/.apache-magpie/" in lines, "local_lock_ignored": "/.apache-magpie.local.lock" in lines, diff --git a/uv.lock b/uv.lock index 4700afc34..5f67f445b 100644 --- a/uv.lock +++ b/uv.lock @@ -28,6 +28,7 @@ members = [ "magpie-bitbucket", "magpie-fossil", "magpie-maildir", + "magpie-skills", "magpie-sourcehut", "magpie-vcs", "oauth-draft", @@ -824,6 +825,27 @@ dev = [ { name = "ruff", specifier = ">=0.15.22" }, ] +[[package]] +name = "magpie-skills" +version = "0.1.0" +source = { virtual = "skills" } + +[package.dev-dependencies] +dev = [ + { name = "mypy" }, + { name = "pytest" }, + { name = "ruff" }, +] + +[package.metadata] + +[package.metadata.requires-dev] +dev = [ + { name = "mypy", specifier = ">=2.3.0" }, + { name = "pytest", specifier = ">=9.1.1" }, + { name = "ruff", specifier = ">=0.15.21" }, +] + [[package]] name = "magpie-sourcehut" version = "0.1.0"