From 30892017f70c48b227fcdb9eeee011720d85f21e Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 1 Aug 2026 05:36:38 +0200 Subject: [PATCH 1/2] 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" From 1cfcbd7047219dbe4ea4dd1d1d5efbaa517e3114 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 8 Aug 2026 01:17:03 +0800 Subject: [PATCH 2/2] fixup: track the ruff-format skip in an issue The skip is justified and carries an exit condition -- 'formatting should land as its own commit, after which this skip can be deleted' -- but nothing was tracking it. A documented temporary exemption with no tracker is how it becomes a permanent one, which is the failure mode this PR exists to fix in the first place. Filed #1076 with the measured size of the job (6 files, 269 lines of churn, all of it the formatter reflowing long set literals and long re.compile constants) and marked it good-first-issue, since the tooling decides the outcome and the only judgement needed is keeping it in its own commit. Generated-by: Claude Code (Opus 5) --- skills/pyproject.toml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/skills/pyproject.toml b/skills/pyproject.toml index d0fae2544..6ef3222c9 100644 --- a/skills/pyproject.toml +++ b/skills/pyproject.toml @@ -53,7 +53,9 @@ package = false # 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. +# be deleted. Tracked in +# https://github.com/apache/magpie/issues/1076 — a skip with no tracking issue +# is how a temporary exemption becomes a permanent one. [tool.magpie.checks] skip = ["ruff-format"]