Skip to content

ci(skills): lint, type-check and test the Python under skills/ - #1054

Closed
potiuk wants to merge 2 commits into
apache:mainfrom
potiuk:ci/skills-python-checks
Closed

ci(skills): lint, type-check and test the Python under skills/#1054
potiuk wants to merge 2 commits into
apache:mainfrom
potiuk:ci/skills-python-checks

Conversation

@potiuk

@potiuk potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member

Summary

  • Nothing checked the Python in skills/. The workspace checks iterate over [tool.uv.workspace] members, every member lived under tools/, so ruff, mypy, and pytest never saw skills/** — eight helper scripts and guards, plus the nine tests fix(pr-management-triage): centralize terminal PR links #1049 had just added, none of them run.
  • Declares skills/ as one workspace member whose pyproject.toml is a config carrier, not a package (package = false). The existing machinery does the rest: run-workspace-check.sh discovers applicable checks 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.
  • Turning the checks on immediately found seven real things, all fixed here.

Closes #1053.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)

Test plan

  • prek run --all-files passes (23 hooks)
  • ruff check, mypy, and pytest all run and pass through the workspace runner for the new member — uv run --project . python -m … from skills/
  • Verified the coverage is real, not nominal: breaking the OSC 8 emission in pr_link.py now fails three tests under the workspace runner. Before this change the same break produced no signal at all — that is the whole point of the PR, so it seemed worth proving rather than asserting.
  • check-workspace-members passes — it already scans top-level directories (widened in fix(ai-tutors): stop nested code fences inverting the fence state #1011), so skills is discoverable and not reported as drift.

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — no new unrestricted host access; network reach declared in the adapter
  • Vendor neutrality — placeholders used in all skill / tool prose
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
  • Privacy LLM — private content does not reach a non-approved LLM; redactor invoked where needed

(None apply — CI wiring plus type/lint fixes.)

Linked issues

Closes #1053. Follow-up to #1049, which added the first tests under skills/, and to #1011, which fixed the same class of gap for ai-tutors/.

Notes for reviewers (optional)

Why one member and not per-skill packaging. Skills are symlinked into adopter repos one directory at a time (.claude/skills/magpie-<name>skills/<name>), so a pyproject.toml inside each skill would leak build metadata into every adopter. skills/pyproject.toml is never symlinked — the relays point at skills/<name>, never at skills/ itself. I checked that before choosing this shape.

What the checks found. None of it changes behaviour; these are the annotations and renames the checks require, which is rather the point — they were invisible until now.

  • scan_ci_runners.pyzip() without strict=; two same-named futures / future bindings of different types in one function, which is why the second pair needed renaming before mypy could annotate them; an unannotated rows; and step.get("with") called twice, so the isinstance narrowing never stuck.
  • collect_status.pyl as a variable name, and a mixed-value dict literal inferred as dict[str, object], which made a later indexed assignment invalid.
  • test_pr_link.py — a stale # noqa: E402 suppressing nothing.

One deliberate omission: ruff format is skipped for this member via [tool.magpie.checks] skip = ["ruff-format"]. The formatter wants to reflow ~280 lines across six pre-existing scripts — long set literals and regex constants — none of which this change touches. Folding that in would bury the substance behind a mechanical reformat nobody can review line by line. Lint, types and tests all run; formatting can land as its own commit and the skip be deleted then. Happy to do that immediately after if you would rather have it in one go.

Closes apache#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 apache#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/<name>`, 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)
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 apache#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)

@choo121600 choo121600 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!
lgtm

@potiuk

potiuk commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Superseded by #1077, now merged as 8b3b474.

#1077 started as the follow-up this PR asked for — deleting the temporary
[tool.magpie.checks] skip = ["ruff-format"] and applying the formatter —
but it had copied this branch's infrastructure rather than stacking on it,
so both PRs added the same skills/pyproject.toml and whichever merged
second would have conflicted. Rather than ask a first-time contributor to
rebase away work they had correctly identified as prerequisite, the two were
combined into #1077 with both of us as co-authors.

Verified before combining, rather than assumed: with this branch's
ruff-format skip removed and uv run --directory skills --project . ruff format . applied, every skills/**/*.py file is byte-identical to #1077's
branch. skills/pyproject.toml there is this PR's file with exactly the
10-line skip block removed, and the root pyproject.toml member
registration and uv.lock delta are identical. So nothing from this PR was
dropped.

Three out-of-scope markdown reflows on #1077 were reverted before merge
(ruff format does not touch .md, and one had broken a deliberate column
alignment pairing a Python example with the Groovy example below it).

Net effect: everything this PR did has landed, plus the formatting pass, and
ruff format (workspace) now runs with no skip — so #1076 is closed
properly rather than deferred again. #1053 is closed by the merge.

@potiuk potiuk closed this Aug 17, 2026
sgoel2be24-cyber pushed a commit to sgoel2be24-cyber/magpie that referenced this pull request Aug 17, 2026
apache#1077)

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, plus the nine tests added in apache#1049.

Declare `skills/` as one workspace member whose `pyproject.toml` is a config
carrier, not a package (`package = false`). The existing machinery does the
rest: `run-workspace-check.sh` discovers applicable checks 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
rejected because skills are symlinked into adopter repos one directory at a
time, which would leak build metadata into every adopter.

Turning the checks on found real issues, all fixed here: `zip()` without
`strict=`, two same-named bindings of different types in one function, an
unannotated local, a repeated `.get()` that defeated `isinstance` narrowing,
`l` as a variable name, a mixed-value dict literal that blocked a later
indexed assignment, and a stale `# noqa: E402`.

`ruff format` is applied across the tree in the same change, so no
`[tool.magpie.checks]` skip is needed and the formatter check runs from the
start.

This supersedes apache#1054, which carried the same wiring behind a temporary
ruff-format skip; that PR is closed in favour of this one. Verified before
combining: with apache#1054's skip removed and `ruff format` applied, every
`skills/**/*.py` file is byte-identical to this branch.

Closes apache#1053
Closes apache#1076

Co-authored-by: Jarek Potiuk <potiuk@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci(skills): Python under skills/ is neither linted, type-checked, nor tested

2 participants