diff --git a/.gitea/workflows/lint.yml b/.gitea/workflows/lint.yml index 054daf5..cee5566 100644 --- a/.gitea/workflows/lint.yml +++ b/.gitea/workflows/lint.yml @@ -249,3 +249,49 @@ jobs: - name: Doc claims match the build files run: bash scripts/check-doc-drift.sh + + skill-frontmatter: + # Gate the image-baked skills' frontmatter against the spec pi's own loader + # measures. Its own job, for the same reason as skill-floor and doc-drift: + # "a shipped skill violates the spec" should be a distinct red name, not a + # line buried in a job about workflow syntax. + # + # The gap it closes, measured 2026-10-23 while adding pi-devbox-firstboot: + # that skill's description was drafted at 1056 chars against the loader's + # MAX_DESCRIPTION_LENGTH of 1024. pi does NOT reject it — dist/core/skills.js + # pushes a diagnostic and then loads the skill anyway ("Still load the skill + # even with warnings, unless description is missing or empty"). So the only + # thing that would ever report the violation is a diagnostic inside a live pi + # session, and nothing in CI starts one. A spec violation that still works is + # the most durable kind of rot: nothing forces the fix. + # + # Not free in the way skill-floor was free: two of the five baked skills are + # already within ~20 chars of the limit (credential-incident-response 1022, + # pi-extensions 1003), so this fires on a plausible next edit rather than on + # a hypothetical one. + # + # The sibling failure it also catches is a silent DROP rather than a warning: + # a missing/empty description returns skill: null, so the file ships, the + # symlink resolves, smoke-test's `test -f` passes, pi-devbox-version lists + # it — and no agent can ever see it. + # + # Exit codes 0 valid / 1 violation / 2 cannot-run, matching lint-shell.sh, + # check-skill-floor.sh and check-doc-drift.sh. A missing skills tree or a + # python3 without the yaml module is a red 2, not a green tick. + runs-on: ubuntu-latest + container: + image: catthehacker/ubuntu:act-latest + steps: + - uses: actions/checkout@v4 + + # python3-yaml because `description: >-` is a folded block scalar: the + # value spans lines and joins with spaces, so a line-oriented length + # check measures the wrong string. Same package the actionlint job + # installs, same parser check-doc-drift.sh already relies on. + - name: Install python3-yaml + run: | + apt-get update + apt-get install -y --no-install-recommends python3-yaml + + - name: Baked skill frontmatter is valid + run: bash scripts/check-skill-frontmatter.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d75e83..87ea52c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,28 @@ Pre-v1.0.0 tags followed the pi npm version (`v{pi_version}[letter]`). the round only through that line, so rewording the paragraph must not be able to break the link silently. +- **`scripts/check-skill-frontmatter.sh` + a `skill-frontmatter` lint job: + baked skill frontmatter is now gated against the limits pi's own loader + measures.** Found while adding the skill above, whose `description` was + drafted at 1056 characters against the loader's `MAX_DESCRIPTION_LENGTH` of + 1024. pi does **not** reject that — `dist/core/skills.js` pushes a diagnostic + and then loads the skill anyway ("Still load the skill even with warnings, + unless description is missing or empty"), so the only thing that would ever + report the violation is a diagnostic inside a live pi session — and nothing in + CI starts one. A spec violation that still works is the most durable kind of + rot, because nothing forces the fix. + + Unlike `check-skill-floor.sh`, this one was **not** free to add: two of the + five baked skills are already within ~20 characters of the limit + (`credential-incident-response` 1022, `pi-extensions` 1003), so it fires on a + plausible next edit rather than a hypothetical one. It also catches the + sibling failure that is a silent **drop** rather than a warning — missing or + empty `description` returns `skill: null`, so the file ships, the symlink + resolves, smoke-test's `test -f` passes, `pi-devbox-version` lists it, and no + agent can ever see it. Exit codes 0 valid / 1 violation / 2 cannot-run, and + the vendored copies are deliberately in scope: the image ships those bytes, + so the violation is ours to carry whoever typed it. + ### Fixed - **CI: a smoke job's image could be deleted mid-test by another job's disk diff --git a/scripts/check-skill-frontmatter.sh b/scripts/check-skill-frontmatter.sh new file mode 100755 index 0000000..15a6200 --- /dev/null +++ b/scripts/check-skill-frontmatter.sh @@ -0,0 +1,189 @@ +#!/usr/bin/env bash +# check-skill-frontmatter.sh — fail when an image-baked skill's frontmatter +# violates the Agent Skills spec that pi's own loader measures against. +# +# THE DEFECT THIS EXISTS TO CATCH, measured 2026-10-23 while adding the +# pi-devbox-firstboot skill. Its `description` was drafted at 1056 characters. +# pi's shipped loader (dist/core/skills.js) defines MAX_DESCRIPTION_LENGTH = +# 1024 and pushes a diagnostic when a description is longer — and then says, in +# a comment directly above the return: +# +# // Still load the skill even with warnings, unless description is +# // missing or empty. +# +# So an over-long description does NOT break the skill. That is exactly why it +# needs a gate rather than trust: the skill works, the violation is real, and +# the only thing that would ever tell you is a diagnostic emitted inside a pi +# session. Nothing in CI starts a pi session and reads skill diagnostics, so the +# warning is invisible here by construction. A spec violation that still works +# is the most durable kind of rot — nothing ever forces the fix. +# +# One failure in this family is NOT a warning but a silent drop: a missing or +# empty description returns `{ skill: null }`, so the skill is never offered to +# the model at all. The skill file ships, `test -f` passes in smoke-test.sh, the +# symlink resolves, `pi-devbox-version` lists it — and the skill does not exist +# as far as any agent is concerned. This gate separates the two cases in its +# output, because they need different urgency even though both are red. +# +# WHY THE BAKED TREE SPECIFICALLY. These skills are the FALLBACK layer (see +# skills/VENDORED.md): they are what a container resolves with no skillset repo +# mounted, which is also the case of a brand-new device that has nothing else. +# A skillset-owned skill has its own upstream review; a baked one has only this. +# +# MEASURED HEADROOM when this gate was added, which is why it is worth having: +# credential-incident-response 1022 (2 chars spare) +# pi-extensions 1003 (21) +# pi-devbox-firstboot 952 (72) +# pi-devbox-environment 923 (101) +# mempalace 254 (770) +# Two of five are within one sentence of the limit, so "nobody would write a +# description that long" is already false here. +# +# A KNOWN COUPLING, stated rather than discovered later: `pi-extensions` and +# `mempalace` are VENDORED copies, so an upstream edit that pushes one over the +# limit turns this gate red on a file this repo does not author. That is +# deliberate and precedented — check-skill-floor.sh already couples to the +# pi-extensions package — because the image SHIPS those bytes, so the violation +# is ours to carry regardless of who typed it. The fix in that case is to +# refresh the snapshot after upstream trims, not to exempt the file. +# +# WHY NAME IS CHECKED TOO, including against the directory. The loader falls +# back to the PARENT DIRECTORY NAME when frontmatter has no usable `name` +# (`frontmatterName || parentDirName`), so a mismatch does not fail — it +# silently renames the skill, and every pointer written as +# `~/.agents/skills//SKILL.md` then refers to a skill the model knows under +# a different name. The managed AGENTS.md block points at skills by path, so +# that divergence is load-bearing here. +# +# EXIT CODES (same contract as lint-shell.sh / check-skill-floor.sh / +# check-doc-drift.sh — a gate that cannot run must not pass): +# 0 every baked skill's frontmatter is valid +# 1 VIOLATION — at least one skill breaks the spec +# 2 cannot run — no skills directory, or no python3 with a YAML module +# +# WHY PYTHON AND NOT grep/awk. `description: >-` is a YAML folded block scalar: +# its value spans lines and is joined with spaces, so a line-oriented length +# check measures the wrong string. python3-yaml is already installed in the +# actionlint job and already used by check-doc-drift.sh, so this adds no new +# dependency. NB the heredoc form below is `python3 - "$arg"` (script on stdin, +# argument on the command line); `echo x | python3 <<'EOF'` cannot work, which +# is the SC2259 defect the shellcheck gate was added for. +set -euo pipefail + +REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) +SKILLS_DIR="${REPO_ROOT}/rootfs/usr/local/share/pi-devbox/skills" + +usage() { + cat <<'USAGE' +Usage: scripts/check-skill-frontmatter.sh [options] + +Options: + --skills-dir DIR Validate DIR instead of the repo's baked skills tree + (used by the truth-table self-test). + -h, --help This text. + +Exit: 0 valid | 1 violation | 2 cannot run +USAGE +} + +while [ $# -gt 0 ]; do + case "$1" in + --skills-dir) shift; [ $# -gt 0 ] || { echo "::error::--skills-dir needs a value" >&2; exit 2; }; SKILLS_DIR="$1" ;; + -h|--help) usage; exit 0 ;; + *) echo "::error::unknown option: $1" >&2; usage >&2; exit 2 ;; + esac + shift +done + +if [ ! -d "$SKILLS_DIR" ]; then + echo "::error::skills directory not found: ${SKILLS_DIR}" + echo "A gate that cannot run must not pass — this is a red 2, not a green tick." + exit 2 +fi + +if ! command -v python3 >/dev/null 2>&1; then + echo "::error::python3 not found; cannot parse YAML frontmatter" + exit 2 +fi + +if ! python3 -c 'import yaml' >/dev/null 2>&1; then + echo "::error::python3 has no yaml module (apt: python3-yaml); cannot parse frontmatter" + exit 2 +fi + +python3 - "$SKILLS_DIR" <<'PY' +import os, re, sys +import yaml + +# Mirrors pi's dist/core/skills.js. If pi changes these, change them here. +MAX_NAME_LENGTH = 64 +MAX_DESCRIPTION_LENGTH = 1024 + +skills_dir = sys.argv[1] +fatal, violations, checked = [], [], 0 + +for name in sorted(os.listdir(skills_dir)): + d = os.path.join(skills_dir, name) + if not os.path.isdir(d): + continue + path = os.path.join(d, "SKILL.md") + rel = os.path.relpath(path, skills_dir) + + if not os.path.isfile(path): + fatal.append(f"{rel}: directory has no SKILL.md — the loader cannot see it") + continue + checked += 1 + + raw = open(path, encoding="utf-8").read() + m = re.match(r"^---\n(.*?)\n---\n", raw, re.S) + if not m: + fatal.append(f"{rel}: no YAML frontmatter block (must open on line 1 with ---)") + continue + try: + fm = yaml.safe_load(m.group(1)) or {} + except Exception as e: + fatal.append(f"{rel}: frontmatter is not valid YAML: {e}") + continue + if not isinstance(fm, dict): + fatal.append(f"{rel}: frontmatter is not a mapping") + continue + + desc = fm.get("description") + if not isinstance(desc, str) or desc.strip() == "": + # The one hard drop: the loader returns skill: null for this. + fatal.append(f"{rel}: description missing or empty — THE SKILL IS NEVER LOADED") + elif len(desc) > MAX_DESCRIPTION_LENGTH: + violations.append( + f"{rel}: description is {len(desc)} chars, exceeds {MAX_DESCRIPTION_LENGTH} " + f"(loader warns and still loads, so nothing else will tell you)" + ) + + fm_name = fm.get("name") if isinstance(fm.get("name"), str) else None + eff = fm_name or name + if len(eff) > MAX_NAME_LENGTH: + violations.append(f"{rel}: name is {len(eff)} chars, exceeds {MAX_NAME_LENGTH}") + if not re.fullmatch(r"[a-z0-9-]+", eff): + violations.append(f"{rel}: name {eff!r} must be lowercase a-z, 0-9 and hyphens only") + if eff.startswith("-") or eff.endswith("-"): + violations.append(f"{rel}: name {eff!r} must not start or end with a hyphen") + if "--" in eff: + violations.append(f"{rel}: name {eff!r} must not contain a double hyphen") + if fm_name is not None and fm_name != name: + violations.append( + f"{rel}: frontmatter name {fm_name!r} != directory {name!r} — the loader " + f"would use {fm_name!r} while every path-based pointer says {name!r}" + ) + +print(f"checked {checked} baked skill(s) in {skills_dir}") + +if not fatal and not violations: + print("OK: every baked skill's frontmatter is valid.") + sys.exit(0) + +for f in fatal: + print(f"::error::{f}") +for v in violations: + print(f"::error::{v}") +print(f"FAIL: {len(fatal)} fatal, {len(violations)} spec violation(s).") +sys.exit(1) +PY