feat(ci): add skill-quality static gate lane for changed skills (#530)

[kyle-sexton]

Summary

This repo edits its own skills daily via the autonomous pipeline, yet no CI lane invoked the skill-quality checker — 120 evals/evals.json went unexecuted and the 17-check static gate (skill-quality:check) was invoked by nothing. This wires that checker into CI as a deterministic skill-regression net.

Implements stage 1 only of #530 (the "cheap, now" deterministic lane). Stages 2-5 (model-graded judges, frozen regression suite, pass^k autopilot-unlock metric, clean-state isolation) remain a separate decompose target per the ratified operator decision on the issue.

Fix

New skill-quality-gate job in .github/workflows/ci.yml, added to the ci-status required aggregate:

• Static gate (scripts/check-changed-skills.sh) — on a PR, for each skill under plugins/*/skills/** the diff touches, runs plugins/skill-quality/scripts/check-skill.sh with CHECK_SKILL_SKILLS_ROOT set to the owning plugin's skills/ dir and CHECK_SKILL_BASE_REF set to the PR base. Because the checker's trigger-keyword-preservation check diffs against that base, a rewrite that silently drops a description trigger phrase (an auto-invocation regression) fails the check — replacing the work lane's manual high-blast-radius diff read with a deterministic one. Vendor subtrees map to the owning skill; deleted skills are filtered; any FAIL (or env error) exits non-zero. markdownlint (check 6) is deliberately skipped here (CHECK_SKILL_SKIP_MARKDOWNLINT=1) — SKILL.md markdown is already gated by the hygiene lane.
• validate-evals — the existing check-jsonschema composite action schema-validates every evals/evals.json against the bundled plugins/skill-quality/reference/evals.schema.json.
• Self-test (scripts/check-changed-skills.test.sh) — runs first so a broken orchestrator cannot mask a real regression behind a green gate.

No silent-skip (#445 precedent): the job itself never skips (a skipped required job reports success to branch protection) — only the inner PR-diff step is event-gated (mirroring hook-utils-sync's --check-bump), and the self-test + eval-schema steps give push-to-main a passing path. Missing base ref or missing checker fails closed (exit 2).

No new reusable action: reuses this repo's own local script plus the already-referenced check-jsonschema composite action — no change to melodic-software/ci-workflows.

No plugin modified → no version bump (consistent with CI-only PR #490, which bumped nothing). No plugin frontmatter, triggers, hooks, or cross-plugin contracts are touched.

Verification

Local, in the worktree:

• Orchestrator unit tests — bash scripts/check-changed-skills.test.sh → PASS=8 FAIL=0 (covers changed-skill detection, vendor-subtree mapping, dedupe, deletion filtering, env passthrough, pass/fail aggregation, fail-closed on bad base ref / missing checker).
• ShellCheck — shellcheck --rcfile .shellcheckrc scripts/check-changed-skills.sh scripts/check-changed-skills.test.sh → clean.
• End-to-end, real check-skill.sh against real skills (scripts/check-changed-skills.sh origin/main):
  • Happy path (a changed skill with no regression) → PASS, exit 0.
  • Simulated regression (dropped the 'lint my skill' trigger from skill-quality/check) → FAIL: dropped trigger keyword(s) vs origin/main (auto-invocation regression): 'lint my skill', exit 1.
• validate-evals — check-jsonschema --schemafile plugins/skill-quality/reference/evals.schema.json over all plugins/*/skills/*/evals/evals.json → 120/120 pass today (glob matches exactly the 120 eval sets, excludes vendor).

Closes #603

Related

• quality-net: skill changes ship with zero regression detection — 120 evals.json unexecuted, skill-quality:check uninvoked (staged eval/gate/pass^k umbrella) #530 — this PR is stage 1 of the staged quality-net umbrella; stages 2-5 stay a decompose target.
• CI-gate backlog: automatable conformance checks surfaced by the #313 fleet audit #445 — mechanical-conformance CI backlog; this extends gating from conformance to behavior and follows its silent-skip-gate anti-pattern warning.
• source-control(babysit-prs): absorb v4 loop-prompt operating rules into the skill #477-feat(claude-ops or session-flow): loop-prompt authoring skill — generate and maintain standing loop-lane prompts #480 — the rule→skill absorption lifecycle whose regression net this begins to supply.
• work-items(work): per-issue execution should compose installed skills as a role-separated mini-SDLC (implementer ≠ reviewer ≠ verifier, ephemeral fresh-context agents) #513 — the mini-SDLC verify lane that consumes these gates.

[claude]

Claude finished @kyle-sexton's task in 3m 56s —— View job

---

PR Review: feat/530-skill-quality-ci-gate

• Gather context and understand request
• Read changed files
• Review .github/workflows/ci.yml additions
• Review scripts/check-changed-skills.sh
• Review scripts/check-changed-skills.test.sh
• Post findings

---

Overall

This is a well-structured, correctness-first implementation. The no-silent-skip design, fail-closed exit codes, self-test-first ordering, and explicit env-var handling for script injection prevention are all solid. The issues below are all minor robustness concerns — none are blockers.

---

.github/workflows/ci.yml

Positive: skill-quality-gate correctly follows the established pattern — persist-credentials: false, fetch-depth: 0, timeout-minutes: 15, SHA-pinned actions, if: ${{ !cancelled() }} aggregate. The event-gating on only the diff step (not the whole job) is the right call for the required-lane invariant.

One flag: The validate-evals step uses files: plugins/*/skills/*/evals/evals.json. This glob skips evals nested under vendor subtrees (e.g. plugins/*/skills/*/vendor/*/evals/evals.json). If a vendor-bundled tool inside a skill happens to carry its own evals.json, it goes unvalidated. This is almost certainly intentional today, but it is silently different from the static gate's vendor-subtree-mapping behaviour (which does map vendor-subtree file touches back to the owning skill). A one-line YAML comment acknowledging "vendor subtrees excluded" would prevent future "why isn't this validated?" questions.

---

scripts/check-changed-skills.sh

Positive: Fail-closed design (exit 2 for environment errors), proper quoting throughout, "${changed[@]}" safely handles the empty-array case under bash 5 (ubuntu-24.04), and the [[ -f "$skill_dir/SKILL.md" ]] || continue deletion filter is correctly placed after the diff expansion.

Minor — working-tree vs HEAD diff (line 49):

git diff --name-only "$BASE" -- 'plugins/' |

git diff "$BASE" compares the base ref against the working tree, not HEAD. In CI's clean checkout this is equivalent, but explicitly writing git diff --name-only "$BASE" HEAD -- 'plugins/' would be self-documenting and robust against a future step earlier in the job leaving working-tree side-effects. Fix this →

---

scripts/check-changed-skills.test.sh

Positive: 8 scenarios cover the main paths well. The throwaway-repo pattern, git config isolation, and CHECK_SKILL_BIN injection are all clean. The self-test runs in a completely isolated git context so it can't pollute the real repo.

1. grep -q without -F makes the env-passthrough assertion fragile (line 134):

if grep -q "name=beta root=$r/plugins/p2/skills base=$b" "$r/checklog" 2>/dev/null; then

$r expands to something like /tmp/tmp.abc123; the . is a regex metachar and could match any character. $b is a 40-char hex SHA so it's fine, but the path comparison is technically a regex match, not an exact string match. grep -qF (fixed-string) is the correct tool here. Low actual risk, but a literal period in a temp-dir name matching any char is the kind of thing that produces false-pass tests on unusual systems. Fix this →

2. No cleanup trap on the stub temp file (line 27):

STUB="$(mktemp)"

If the test script exits early on an unexpected error (e.g. set -uo pipefail fires on a failed mktemp -d), $STUB is left in /tmp. Adding trap 'rm -f "$STUB"' EXIT immediately after the mktemp line handles this. Individual $r repos are each cleaned up at the end of their scenario, but if a scenario crashes mid-flight, that repo also leaks — a trap at scenario level or a global cleanup is the hardening step. Fix this →

3. Missing scenario: new skill added (no base commit):

All 6 scenarios that involve a checker invocation commit the skill into the base and then modify it. There is no test for the case where a skill is added for the first time (does not exist in the base commit at all). The behaviour should be correct (new files appear in git diff --name-only "$BASE", SKILL.md exists in the working tree, checker runs), but a dedicated test makes that explicit and would catch any future regression where "new skill has no base ref anchor" causes the trigger-preservation check to behave differently.

4. Missing scenario: partial failure across multiple skills:

There is a single-failing-skill test (scenario 3) and a dedupe test (scenario 4), but no test that verifies the summary line when two skills are changed, one passes and one fails — e.g. "2 skill(s) checked, 1 failed" with exit 1. This is the core aggregation logic (failed=$((failed + 1)) / ((failed == 0))) and the only test that would catch an off-by-one or logic inversion there.

---

Summary

File	Verdict
.github/workflows/ci.yml	✅ Correct — minor doc comment suggestion
scripts/check-changed-skills.sh	✅ Correct — one HEAD explicitness nit
scripts/check-changed-skills.test.sh	✅ Correct — grep -qF, trap, two missing scenarios

None of the issues above would cause the gate to incorrectly pass or fail in production CI. The grep -q / grep -qF issue is the only one that could produce a false-pass in the test suite itself (in theory). I'd consider items 1 and 2 in the test file as the highest-priority follow-ups.

[claude]

Claude finished @kyle-sexton's task in 5m 7s —— View job

---

PR Review: feat/530-skill-quality-ci-gate

• Gather context and understand request
• Read changed files
• Review .github/workflows/ci.yml additions
• Review scripts/check-changed-skills.sh
• Review scripts/check-changed-skills.test.sh
• Post findings

---

Overall

The core implementation is correct and well-structured: no-silent-skip design, fail-closed exit codes, self-test-first ordering, and explicit env-var forwarding are all solid. There is one merge-readiness issue caused by branch divergence, plus a few minor robustness concerns in the test file.

---

⚠️ Branch divergence — unintentional CI exclusion removal

This branch was cut before PR #377 (feat(autonomy): guardrail matrix, security binding, setup slice (WP5)) landed on main. That PR added ~100 adversarial fixture files under plugins/autonomy/skills/setup/evals/fixtures/security-binding/ and added the CI exclusion for them in the same commit.

This PR removes that exclusion:

-          exclude: >-
-            :(exclude)plugins/guardrails/lib/path-detection/**
-            :(exclude)plugins/autonomy/skills/setup/evals/fixtures/security-binding/**
+          exclude: ':(exclude)plugins/guardrails/lib/path-detection/**'

The original exclusion comment explains why it exists:

"The autonomy security-binding fixtures carry ADVERSARIAL host paths on purpose (invented /home users the checker must reject) — sanitizing them would gut the regressions they pin."

Because the PR branch predates those fixtures, the author didn't know they were there. GitHub's auto-merge commit (9e278c2) correctly restores the exclusion (the merge diff adds it back from main), so the merged result is safe. But the PR diff itself is misleading — it shows a deletion that isn't intentional and that the merge silently undoes.

Recommendation: rebase onto current main before merging. This makes the diff show only the intended changes (the three new/modified CI files), avoids confusion for future git blame readers, and removes a trap for anyone who cherry-picks the CI commit later.

Permalink: .github/workflows/ci.yml L123–131 on main

---

.github/workflows/ci.yml — new job

Positive: persist-credentials: false, fetch-depth: 0, timeout-minutes: 15, SHA-pinned actions, event-gating only on the diff step (not the whole job), and skill-quality-gate correctly added to ci-status needs. The no-silent-skip rationale (mirroring the hook-utils-sync --check-bump pattern) is sound.

Minor — validate-evals glob excludes vendor-bundled evals (informational):

files: plugins/*/skills/*/evals/evals.json

This glob skips plugins/*/skills/*/vendor/*/evals/evals.json. If a vendor-bundled tool inside a skill carries its own evals.json it goes unvalidated — silently different from the static gate's vendor-subtree-mapping behaviour (which does map vendor touches back to the owning skill). Almost certainly intentional today, but a one-line YAML comment (# vendor subtrees excluded) would prevent future "why isn't this validated?" questions.

---

scripts/check-changed-skills.sh

Positive: fail-closed design (exit 2 for env errors), proper quoting throughout, mapfile -t with < <(...) safe subshell, and [[ -f "$skill_dir/SKILL.md" ]] || continue deletion filter correctly placed after diff expansion. ((failed == 0)) as the script's final expression correctly propagates exit status without set -e.

Minor — working-tree vs HEAD diff (line 49):

git diff --name-only "$BASE" -- 'plugins/' |

git diff "$BASE" compares the base ref against the working tree, not HEAD. In a clean CI checkout these are identical, but writing git diff --name-only "$BASE" HEAD -- 'plugins/' is explicit and defensive against a future step that leaves working-tree side-effects before this one runs. Fix this →

---

scripts/check-changed-skills.test.sh

Positive: throwaway-repo pattern, git config isolation (commit.gpgsign false), CHECK_SKILL_BIN injection, and 8 scenarios covering the main paths are all clean. The self-test runs in a completely isolated git context.

1. grep -q without -F makes the env-passthrough assertion fragile (line 134):

if grep -q "name=beta root=$r/plugins/p2/skills base=$b" "$r/checklog" 2>/dev/null; then

$r expands to something like /tmp/tmp.abc123; the . is a regex metacharacter and could match any character in the path comparison. Use grep -qF (fixed-string) to make the match literal. Low actual risk today, but the canonical failure mode is a false-pass on an unusual temp-dir name. Fix this →

2. No cleanup trap on the stub temp file (line 27):

STUB="$(mktemp)"

If the script exits early (e.g. set -uo pipefail fires on a failed mktemp -d), $STUB is left in /tmp. Adding trap 'rm -f "$STUB"' EXIT immediately after the mktemp line handles this. Individual $r repos each clean up at the end of their scenario, but a mid-scenario crash leaks the repo too — a global REPOS=() accumulator + trap 'rm -rf "${REPOS[@]}"' EXIT would cover both. Fix this →

3. Missing scenario — new skill added (no base commit anchor):

All 6 checker-invocation scenarios commit the skill into the base and then modify it. There is no test for a skill added for the first time (not present in the base commit at all). The expected behaviour is correct (new files appear in git diff --name-only "$BASE", SKILL.md exists in the working tree, checker runs), but a dedicated test makes that explicit and catches any future edge where "new skill has no base ref anchor" causes the trigger-preservation check to behave unexpectedly.

4. Missing scenario — partial failure across multiple skills:

There is a single-failing-skill test (scenario 3) and a deduplication test (scenario 4), but no test verifying the summary line when two skills are changed, one passes and one fails — e.g. "2 skill(s) checked, 1 failed" with exit 1. This is the core aggregation logic (failed=$((failed + 1)) / ((failed == 0))) and the only test that would catch an off-by-one or logic inversion there.

---

Summary

Area	Verdict
Branch state	⚠️ Needs rebase onto current main before merging
.github/workflows/ci.yml new job	✅ Correct — one informational doc comment suggestion
scripts/check-changed-skills.sh	✅ Correct — one HEAD explicitness nit
scripts/check-changed-skills.test.sh	✅ Correct — grep -qF, trap, two missing scenarios

The branch divergence / exclusion removal is the only merge-blocking concern. The test robustness items (grep -qF, trap, two missing scenarios) are the highest-priority code improvements; none would cause the gate to incorrectly pass or fail in production CI.

[kyle-sexton]

Thanks for the review. Classification of each finding (fixes in c97269cd; self-test now PASS=10 FAIL=0):

#	Finding	Classification	Evidence	Reacted
1	ci.yml validate-evals glob excludes vendor-bundled evals — add a doc comment	VALID — fixed	Added a rationale comment on the files: line noting vendor subtrees are intentionally excluded (a bundled tool owns its own eval contract).	👍
2	check-changed-skills.sh L49: git diff "$BASE" → "$BASE" HEAD	INCORRECT (remedy declined)	Observation is technically true, but the literal fix breaks 6 self-test scenarios that apply uncommitted working-tree changes and rely on git diff <base> seeing them — with HEAD added, HEAD==base → empty diff → "nothing to gate". No production upside either: the CI checkout is clean and the self-test runs in isolated /tmp repos (never mutates the checkout), so working-tree==HEAD in CI regardless. Kept as-is.	👍
3	test L134: grep -q → grep -qF	VALID — fixed	Switched to grep -qF; the temp-dir path is now matched literally, not as a regex.	👍
4	test L27: add cleanup trap for $STUB	VALID — fixed	Added trap 'rm -f "$STUB"' EXIT right after the mktemp; removed the now-redundant final rm -f "$STUB".	👍
5	Missing scenario: new skill added (no base anchor)	VALID — fixed	Added "newly added skill is gated". The new skill is committed onto the head (an untracked file never appears in git diff <base>), matching how a real PR carries it.	👍
6	Missing scenario: partial failure across multiple skills	VALID — fixed	Added "two changed skills, one pass one fail → gate fails, 2 skill(s) checked, 1 failed".	👍

(👍 is per-comment; GitHub allows one reaction per user.)

[kyle-sexton]

Thanks — second round. Classification (code fixes in c97269cd; self-test now PASS=10 FAIL=0):

#	Finding	Classification	Evidence	Reacted
1	Branch divergence — PR diff shows the security-binding exclusion being removed	VALID (defer to human merger)	Confirmed: this branch predates #377, so main carries both the exclusion and the ~100 fixtures while this branch has neither. API reports MERGEABLE (no conflict) and the auto-merge restores the exclusion, so the merged result is safe. Per babysit policy this lane does not rebase/merge main in absent a real conflict; restoring only the exclusion line here would add an exclusion for fixtures that do not exist on this branch (doing part of the forbidden rebase). Left for the human at merge — an optional rebase would clean up the diff.	👍
2	ci.yml validate-evals glob excludes vendor-bundled evals — add a doc comment	VALID — fixed	Added a rationale comment on the files: line.	👍
3	check-changed-skills.sh L49: git diff "$BASE" → "$BASE" HEAD	INCORRECT (remedy declined)	True observation, but the literal fix breaks 6 self-test scenarios that diff uncommitted working-tree changes (HEAD==base → empty diff → "nothing to gate"). CI checkout is clean and the self-test runs in isolated /tmp repos, so working-tree==HEAD in CI anyway — no upside. Kept as-is.	👍
4	test L134: grep -q → grep -qF	VALID — fixed	Switched to grep -qF (literal match).	👍
5	test L27: add cleanup trap for $STUB	VALID — fixed	Added trap 'rm -f "$STUB"' EXIT; removed the redundant final rm.	👍
6	Missing scenario: new skill added	VALID — fixed	Added "newly added skill is gated" (new skill committed onto head).	👍
7	Missing scenario: partial failure across multiple skills	VALID — fixed	Added "2 checked, 1 failed" scenario.	👍

(👍 is per-comment; GitHub allows one reaction per user.)