diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c6f95c86e..2a470f68a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -373,6 +373,33 @@ jobs: schemafile: plugins/skill-quality/reference/evals.schema.json files: plugins/*/skills/*/evals/evals.json + # Portability lint: skills declared ecosystem/forge/tracker-agnostic must not + # ship bare hardcoded stack/forge/branch/tracker defaults — the coupling class + # the external reviewer re-caught PR after PR. On a PR it scans the skill files + # the change touches (not the whole corpus) against the coupling-token list, so + # enabling a token class prevents NEW coupling without red-lining existing + # violations that member issues own. The job itself never skips (a skipped + # required job reports success to branch protection) — only the PR-diff step is + # event-gated, and the self-test gives push a passing path and stops a broken + # detector masking a real violation behind a green gate. + portability-lint: + runs-on: ubuntu-24.04 + timeout-minutes: 15 + steps: + - name: Check out + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + # Full history so the PR base ref resolves for the changed-file diff. + fetch-depth: 0 + - name: Test the portability gate + run: bash scripts/check-skill-portability.test.sh + - name: Gate changed skill files on the portability contract + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + run: scripts/check-skill-portability.sh "origin/$BASE_REF" + runner-policy: name: Runner policy runs-on: ubuntu-24.04 @@ -408,6 +435,7 @@ jobs: - miro-plugin - runner-policy - skill-quality-gate + - portability-lint - zizmor # Fail-closed through execution: !cancelled() (never a success-guard) so a # lane failure still runs this required aggregate and the result join below diff --git a/docs/PLUGIN-PHILOSOPHY.md b/docs/PLUGIN-PHILOSOPHY.md index d3c405963..d00bb7f66 100644 --- a/docs/PLUGIN-PHILOSOPHY.md +++ b/docs/PLUGIN-PHILOSOPHY.md @@ -135,6 +135,14 @@ as defaults. This governs every plugin and every convention, not one class. Two table assuming them. A convention a consumer could reasonably do differently belongs in lane 2, as a discovered-and-externalized extensibility point, never as a lane-1 default. +A bare lane-1 hardcode in a skill declared agnostic — a fixed default branch, forge, ecosystem, or +tracker where the consuming repo could reasonably differ — is a defect, mechanically caught rather +than asserted only in prose. A detection-first or presence-gated use is compliant. A capability +genuinely and inherently locked to one branch, forge, ecosystem, or tracker declares that narrower, +inherent scope at the coupling site — the same declared-narrower-boundary allowance the +cross-platform contract makes for OS platform — rather than shipping the assumption bare under a +neutral name. + ## Configuration ownership and scope Choose one authoritative owner for each value: diff --git a/scripts/check-skill-portability.sh b/scripts/check-skill-portability.sh new file mode 100755 index 000000000..354a39f2f --- /dev/null +++ b/scripts/check-skill-portability.sh @@ -0,0 +1,225 @@ +#!/usr/bin/env bash +# Portability-lint gate: skills declared ecosystem/forge/tracker-agnostic must +# not ship bare hardcoded stack/forge/branch/tracker defaults. The agnosticism +# contract lives in docs/PLUGIN-PHILOSOPHY.md (Design boundary, +# Two-lane convention posture, Cross-platform contract's declared-narrower-scope +# allowance); prose states it but cannot self-verify, so this gate turns the +# assertion into a mechanical check. +# +# scripts/check-skill-portability.sh gate skill files a PR changed +# scripts/check-skill-portability.sh --all audit every skill file +# scripts/check-skill-portability.sh --paths F... scan exactly these files +# +# WHAT is detected is data, not logic: the coupling tokens live in +# scripts/skill-portability-tokens.txt (override with SKILL_PORTABILITY_TOKENS), +# one ERE pattern per active line, so a reviewer re-catch is a one-line data edit +# and rollout stages one token-class at a time. HOW a legitimate hit is excused +# is this script's job. +# +# Scope declaration, resolved per the ratified declaration-first plan: no new +# frontmatter field. A skill is agnostic by default — the Design boundary already +# binds every plugin — so the gated set is "the skill files a change touches", +# exactly as the skill-quality gate scopes itself. A skill with an inherent, +# declared narrower scope (a genuinely forge- or ecosystem-locked capability +# under a neutral name) opts out with a reviewer-visible comment, reusing the +# annotated-exemption shape the silent-skip gate established rather than inventing +# a declaration mechanism. +# +# Changed-FILE (not whole-skill-dir) scoping keeps a PR responsible only for the +# files it actually edits, so enabling a token class never red-lines main: main's +# push event scans nothing (the self-test is the push path), and existing +# violations wait for the owning follow-up fix or the file's next edit. +# +# A token hit fails UNLESS one of three reviewer-visible escapes applies: +# 1. a detection-first resolution use — an auto-recognized branch-resolution +# command on the hit line (an `origin/HEAD` / symbolic-ref / merge-base / +# PR-baseRefName ladder that falls back to origin/main is the CORRECT +# pattern, not a bare assumption); the resolution command is the evidence, +# not the surrounding prose, so a bare default whose resolution is not +# co-located on the line flags and uses a per-site portability-ok escape; +# 2. a per-site recorded exemption `portability-ok: ` on the hit line +# or in the contiguous comment block directly above it; +# 3. a whole-file `portability-scope: ` declaration anywhere in the +# file (the inherent, declared narrower boundary). +# +# This is a grep-level tripwire, not a semantic proof. Guard markers are seeded +# for the one active class (branch/default-branch); enabling a further class +# revisits them here, proven against an `--all` audit first. +# +# Files scanned within a changed skill: *.md and *.sh, excluding *.test.sh (test +# fixtures), vendor/ (upstream-synced copies with their own drift gate), and +# evals/ (eval fixtures carry adversarial example prompts by design). +# +# Exit 0 = clean (or nothing in scope); 1 = one or more violations; 2 = usage / +# environment error (fail closed — never a silent skip). +set -uo pipefail + +cd "$(dirname "${BASH_SOURCE[0]}")/.." || exit 2 + +TOKENS="${SKILL_PORTABILITY_TOKENS:-scripts/skill-portability-tokens.txt}" +if [[ ! -f "$TOKENS" ]]; then + printf 'Error: token list not found: %s\n' "$TOKENS" >&2 + exit 2 +fi + +usage() { + printf 'usage: check-skill-portability.sh | --all | --paths FILE...\n' >&2 + exit 2 +} + +# is_scannable — a skill file the gate authors are responsible for. +is_scannable() { + local f="$1" + case "$f" in + */vendor/* | */evals/*) return 1 ;; + *.test.sh) return 1 ;; + *.md | *.sh) return 0 ;; + *) return 1 ;; + esac +} + +# Resolve the file set for the requested mode. +files=() +if (($# == 0)); then + usage +fi + +mode="$1" +case "$mode" in +--all) + shift + (($# == 0)) || usage + while IFS= read -r f; do + is_scannable "$f" && files+=("$f") + done < <(find plugins -type f -path 'plugins/*/skills/*' \( -name '*.md' -o -name '*.sh' \) | sort) + ;; +--paths) + shift + (($# > 0)) || usage + files=("$@") + ;; +-*) + usage + ;; +*) + # Changed-file mode: . + base="$mode" + shift + (($# == 0)) || usage + if ! git rev-parse --verify --quiet "${base}^{commit}" >/dev/null; then + printf 'Error: base ref %s is not a valid commit\n' "$base" >&2 + exit 2 + fi + # Diff on plugins/ then filter the skill path in-script: a `plugins/*/skills/` + # git pathspec does not match under git's default (non-pathname) globbing. + # NUL-delimited (-z) so a pathname Git would C-quote (non-ASCII bytes under the + # default core.quotePath, or a literal quote/backslash) arrives verbatim: a + # quoted `"plugins/…"` would miss the glob below and the file would be silently + # dropped — the silent exclusion the contract forbids. + while IFS= read -r -d '' f; do + case "$f" in + plugins/*/skills/*) ;; + *) continue ;; + esac + is_scannable "$f" || continue + [[ -f "$f" ]] || continue # a rename-away/deletion leaves nothing to scan + files+=("$f") + done < <(git diff --name-only --diff-filter=d -z "$base" -- 'plugins/' | sort -z -u) + ;; +esac + +if ((${#files[@]} == 0)); then + echo "No skill files in scope — nothing to gate." + exit 0 +fi + +# scan_file — print `LINE: token -> text` for each unexcused hit. +scan_file() { + local file="$1" + # A whole-file declared narrower scope (the inherent-boundary case) excuses + # every hit in the file; the declaration is visible in the diff. + if grep -qE 'portability-scope:' -- "$file"; then + return 0 + fi + awk ' + function is_annotated(l) { return l ~ /portability-ok:/ } + function is_comment(l) { return l ~ /^[[:space:]]*#/ || l ~ /')" +if scan_paths "$f" >/dev/null 2>&1; then + ok "same-line portability-ok passes" +else + fail "same-line portability-ok should pass" +fi +rm -f "$f" + +# --- portability-ok in the comment block above passes ---------------------- +f="$(tmpfile ' +The base branch is origin/main here.')" +if scan_paths "$f" >/dev/null 2>&1; then + ok "comment-block-above portability-ok passes" +else + fail "comment-block-above portability-ok should pass" +fi +rm -f "$f" + +# --- annotation does not leak past intervening code ------------------------ +f="$(tmpfile ' +diff against origin/main here +now a plain line +diff against origin/main again')" +if out="$(scan_paths "$f" 2>&1)"; then + fail "annotation should not sanction a later hit, got success: $out" +elif echo "$out" | grep -q ":4:" && ! echo "$out" | grep -q ":2:"; then + ok "annotation covers line 2 only and does not leak past intervening code" +else + fail "expected line 4 flagged and line 2 clean, got: $out" +fi +rm -f "$f" + +# --- whole-file portability-scope declaration passes ----------------------- +f="$(tmpfile ' +This skill diffs origin/main and pushes with origin/master.')" +if scan_paths "$f" >/dev/null 2>&1; then + ok "whole-file portability-scope passes" +else + fail "whole-file portability-scope should pass" +fi +rm -f "$f" + +# --- staged (commented) tokens stay inactive under the REAL token list ----- +f="$(mktemp --suffix=.md)" +printf '%s\n' 'This agnostic skill mentions dotnet and raw.githubusercontent.com.' >"$f" +if SKILL_PORTABILITY_TOKENS="$REAL_TOKENS" bash "$SCRIPT" --paths "$f" >/dev/null 2>&1; then + ok "staged tokens (dotnet, raw.githubusercontent) are inactive in the shipped list" +else + fail "shipped list should only enforce the active branch class" +fi +rm -f "$f" + +# --- missing token list fails closed (exit 2) ------------------------------ +f="$(tmpfile 'origin/main')" +SKILL_PORTABILITY_TOKENS="/nonexistent/tokens.txt" bash "$SCRIPT" --paths "$f" >/dev/null 2>&1 +if [[ "$?" -eq 2 ]]; then + ok "missing token list exits 2 (fail closed)" +else + fail "missing token list should exit 2" +fi +rm -f "$f" + +# --- an invalid base ref fails closed (exit 2) ----------------------------- +(cd "$REPO_ROOT" && SKILL_PORTABILITY_TOKENS="$TEST_TOKENS" bash "$SCRIPT" definitely-not-a-ref >/dev/null 2>&1) +if [[ "$?" -eq 2 ]]; then + ok "invalid base ref exits 2 (fail closed)" +else + fail "invalid base ref should exit 2" +fi + +# --- --all excludes vendor/, evals/, and *.test.sh ------------------------- +fx="$(mktemp -d)" +mkdir -p "$fx/scripts" "$fx/plugins/alpha/skills/x/vendor" "$fx/plugins/alpha/skills/x/evals" +cp "$SCRIPT" "$fx/scripts/" +printf 'diff against origin/main\n' >"$fx/plugins/alpha/skills/x/SKILL.md" +printf 'origin/main\n' >"$fx/plugins/alpha/skills/x/vendor/upstream.md" +printf 'origin/main\n' >"$fx/plugins/alpha/skills/x/evals/e.md" +printf 'origin/main\n' >"$fx/plugins/alpha/skills/x/gen.test.sh" +out="$(cd "$fx" && SKILL_PORTABILITY_TOKENS="$TEST_TOKENS" bash scripts/check-skill-portability.sh --all 2>&1)" +rc=$? +if [[ "$rc" -ne 0 ]] && + echo "$out" | grep -q 'skills/x/SKILL.md' && + ! echo "$out" | grep -qE 'vendor/|evals/|test\.sh'; then + ok "--all scans SKILL.md but excludes vendor/, evals/, and *.test.sh" +else + fail "--all exclusion set wrong (rc=$rc): $out" +fi +rm -rf "$fx" + +# --- empty scope passes ---------------------------------------------------- +fx="$(mktemp -d)" +mkdir -p "$fx/scripts" "$fx/plugins" +cp "$SCRIPT" "$fx/scripts/" +if (cd "$fx" && SKILL_PORTABILITY_TOKENS="$TEST_TOKENS" bash scripts/check-skill-portability.sh --all >/dev/null 2>&1); then + ok "empty skill tree passes" +else + fail "empty skill tree should pass" +fi +rm -rf "$fx" + +# --- diff-mode reads a Git-quoted (non-ASCII) changed path ----------------- +# A changed file whose pathname triggers Git's C-style quoting (here a non-ASCII +# byte; core.quotePath defaults on) must still be gated. Without -z, git diff +# emits `"plugins/.../quoted-\303\251.md"`, the leading quote misses the +# plugins/*/skills/* glob, and the file is silently dropped — the silent +# exclusion the contract forbids. The fixture commits one ASCII-named and one +# non-ASCII-named coupling file, then diffs against the empty base commit: the +# ASCII hit proves -z left the common path intact, and two COUPLING lines prove +# the quoted path was read, not skipped. The non-ASCII name is built with octal +# escapes so this test source stays pure ASCII. +fx="$(mktemp -d)" +mkdir -p "$fx/scripts" +cp "$SCRIPT" "$fx/scripts/" +quoted_name="$(printf 'quoted-\303\251.md')" # trailing U+00E9 byte — non-ASCII, triggers Git quoting +out="$( + cd "$fx" && + git init -q && + git config user.email test@example.com && + git config user.name test && + git commit -q --allow-empty -m base && + base="$(git rev-parse HEAD)" && + mkdir -p 'plugins/p/skills/s' && + printf 'diff against origin/main\n' >'plugins/p/skills/s/plain.md' && + printf 'diff against origin/main\n' >"plugins/p/skills/s/${quoted_name}" && + git add -A >/dev/null 2>&1 && + git commit -q -m add-skills && + SKILL_PORTABILITY_TOKENS="$TEST_TOKENS" bash scripts/check-skill-portability.sh "$base" 2>&1 +)" +rc=$? +if [[ "$rc" -ne 0 ]] && + echo "$out" | grep -q 'plain.md:1:' && + [[ "$(echo "$out" | grep -c 'COUPLING:')" -eq 2 ]]; then + ok "diff-mode gates a Git-quoted (non-ASCII) changed path (not silently dropped)" +else + fail "diff-mode should flag both the ASCII and non-ASCII coupling files (rc=$rc): $out" +fi +rm -rf "$fx" + +echo +echo "PASS=$PASS FAIL=$FAIL" +[[ "$FAIL" -eq 0 ]] diff --git a/scripts/skill-portability-tokens.txt b/scripts/skill-portability-tokens.txt new file mode 100644 index 000000000..4a36da1ef --- /dev/null +++ b/scripts/skill-portability-tokens.txt @@ -0,0 +1,74 @@ +# Portability-lint token list — coupling tokens that break declared skill +# agnosticism (issue #531). Each ACTIVE line is one ERE pattern that +# scripts/check-skill-portability.sh matches against changed skill files. +# +# Ownership / growth: this list is the gate's data, deliberately kept OUT of the +# script so a reviewer re-catch of this coupling class becomes a one-line data +# change, never a gate-logic edit. Every reviewer re-catch of a bare stack / +# forge / branch / tracker default is a missing token — add it here (or uncomment +# a staged class once its member issue lands and the corpus is proven clean for +# it), closing the loop so the gate is self-improving. +# +# Rollout is one token-class at a time (ratified decision on #531, Option A): a +# staged class stays commented with its enable-trigger until the class is proven +# green against the real corpus. Changed-file scoping (see the script header) +# means an active class only ever gates the files a PR actually touches, so +# enabling a class never red-lines main — it prevents NEW coupling and forces a +# fix-or-declare when a coupled file is next edited. +# +# Format: blank lines and lines beginning with `#` are ignored; every other line +# is an ERE pattern (leading/trailing whitespace trimmed). Labels live in the +# comment above each pattern, never on the pattern line. +# +# Escaping a legitimate hit (all three are comments the reviewer sees in the diff, +# never a frontmatter field — see the script header for detection detail): +# - a detection-first / presence-gated use is auto-recognized (guard markers); +# - a per-site recorded exemption: `portability-ok: `; +# - a whole-file inherent, declared narrower scope (the #441 case): +# `portability-scope: `. + +# --- ACTIVE --- + +# Branch / default-branch hardcode (the #467 review-churn class: "branch/remote +# x4"). A bare `origin/main` / `origin/master` asserts the consumer's default +# branch instead of resolving it (symbolic-ref / merge-base / origin/HEAD, with +# origin/main only as a documented last-resort fallback — those detection-ladder +# uses are auto-recognized as guarded). +origin/(main|master) + +# --- STAGED (commented until the class is proven clean against the corpus) --- +# +# Before enabling ANY staged class below, beyond the corpus audit: +# - Audit and EXTEND `is_guarded()` in check-skill-portability.sh with the +# markers the new class legitimately needs. Its markers are scoped to the +# active branch/default-branch class — branch-detection evidence only. A +# class that genuinely needs optional-dependency presence signals (e.g. +# "useful if using .NET — requires dotnet CLI") must add its own +# class-scoped markers; a generic `if using` / `when present` must never +# guard a bare hardcode, or the gate goes green on a real coupling. +# - If the pattern uses `\b` as a word boundary, it is NOT reliable: POSIX ERE +# defines no word-boundary escape, and on common awks (gawk AND mawk/nawk) +# `\b` matches a literal backspace byte, not a boundary — verified: gawk uses +# `\y` for that, and `\bdotnet\b` matches nothing here. So a `\b`-anchored +# class silently never matches (gate stays green while missing real +# violations). Rewrite to a POSIX-safe boundary before activating, e.g. +# `(^|[^a-zA-Z0-9_])dotnet([^a-zA-Z0-9_]|$)`, or a simpler `[Dd]otnet`-style +# alternative. + +# Ecosystem hardcode: .NET-flavored defaults in an ecosystem-agnostic skill +# (#491 #492). Enable when the .NET-example member issues land and an audit run +# (`--all`) shows the agnostic skills clean (ecosystem-specific plugins declare +# `portability-scope`). Bare token is high-volume in the corpus today (56 files). +# dotnet +# \bClean Arch(itecture)?\b + +# Forge/marketplace-internal hardcode: a hardcoded raw GitHub content URL where a +# portable fetch or a declared scope belongs. Enable when #405 and the forge-lock +# members (e.g. #441 declares source-control's inherent GitHub scope) land. +# raw\.githubusercontent\.com + +# Tracker hardcode: bare `gh` tracker calls in a tracker-agnostic skill that +# should route the work-item seam (#416). Enable once the seam-routing members +# land; `gh` is legitimate in forge-scope-declared skills, so this class needs +# the declared-scope escape hatch exercised first. +# \bgh (issue|api|pr|label)