diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index adb07fddd..4abd110f9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -193,6 +193,16 @@ jobs: - name: Test the skill drift check run: make test-skill-drift + # Same Ruby the eval runner gets, because the point of the check is the + # engine: these patterns are Onigmo, and Go's RE2 guard cannot see them. + - name: Set up Ruby + uses: ruby/setup-ruby@95ef2b042f9d7a56d8268cba8559e2842e2ad01b # v1.321.0 + with: + ruby-version: '3.3' + + - name: Compile the skill-eval patterns + run: make check-eval-patterns + cli-surface: name: CLI Surface Check runs-on: ubuntu-latest @@ -231,12 +241,20 @@ jobs: if: always() run: git worktree remove /tmp/baseline-tree --force 2>/dev/null || true - skill-eval: - name: Skill Evals + # Whether the evals can run at all, answered before the job that would run + # them. Split out because a job cannot skip itself: a step-level `if` leaves + # the job green, and "Skill Evals: success" with no case run is the reading + # this repository actually had. A `needs` + job-level `if` reports Skipped, + # which is what happened. + skill-eval-gate: + name: Skill Evals (preflight) runs-on: ubuntu-latest permissions: contents: read + pull-requests: read # dorny/paths-filter enumerates PR files via the REST API if: github.event_name == 'pull_request' + outputs: + run: ${{ steps.decide.outputs.run }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -246,26 +264,91 @@ jobs: id: filter uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 with: + # test.yml is in its own filter so the job proves itself on the PR + # that introduces or edits it. filters: | skill: - 'skills/basecamp/SKILL.md' - 'skills/basecamp-connect/SKILL.md' - 'skill-evals/**' + - '.github/workflows/test.yml' + + - name: Decide whether the evals can run + id: decide + env: + CHANGED: ${{ steps.filter.outputs.skill }} + # Presence only, never the value. Fork PRs receive no secrets, so this + # is false there by construction and the skip below is the honest + # answer for them too. + HAS_KEY: ${{ secrets.ANTHROPIC_API_KEY != '' }} # zizmor: ignore[secrets-outside-env] -- a presence test, not the key; adding an environment would block PR-triggered runs + run: | + # A preflight that cannot tell must not be allowed to say no. The + # filter answers true or false; anything else means it did not answer, + # and treating that as "nothing to run" would rebuild the defect this + # job exists to remove, one level up (Copilot on #768). + case "$CHANGED" in + true|false) ;; + *) + echo "::error::the paths filter did not answer (skill='$CHANGED'), so this preflight cannot say whether the evals are needed" + exit 1 + ;; + esac + + if [ "$CHANGED" != "true" ]; then + echo "run=false" >> "$GITHUB_OUTPUT" + echo "No skill or eval changes on this pull request." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + + cases=$(find skill-evals/cases -name '*.yml' | wc -l | tr -d ' ') + + if [ "$HAS_KEY" != "true" ]; then + echo "run=false" >> "$GITHUB_OUTPUT" + echo "::warning title=Skill Evals did not run::ANTHROPIC_API_KEY is not configured for this repository, so none of the $cases eval cases were run on a pull request that changes them. The Skill Evals job below reports Skipped, not success." + { + echo "### Skill Evals: NOT RUN" + echo + echo "This pull request changes a skill or its evals, and **$cases eval cases were not run**." + echo "\`ANTHROPIC_API_KEY\` is not configured for this repository, and fork pull requests never receive secrets." + echo + echo "What still covers these files:" + echo "- \`make check-eval-patterns\` compiles every pattern in every case under Onigmo, the engine the runner uses." + echo "- \`TestConnectSkillEvalRejectsHoldTheServeValueRule\` holds the \`--serve\` value rule in the connector cases." + echo + echo "Neither runs a case. If these evals must gate this repository, configure the secret; until then this job is Skipped rather than green." + } >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + + echo "run=true" >> "$GITHUB_OUTPUT" + + skill-eval: + name: Skill Evals + runs-on: ubuntu-latest + permissions: + contents: read + needs: skill-eval-gate + if: needs.skill-eval-gate.outputs.run == 'true' + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: Set up Ruby - if: steps.filter.outputs.skill == 'true' uses: ruby/setup-ruby@95ef2b042f9d7a56d8268cba8559e2842e2ad01b # v1.321.0 with: ruby-version: '3.3' - name: Run skill evals - if: steps.filter.outputs.skill == 'true' env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} # zizmor: ignore[secrets-outside-env] -- fork PRs don't receive secrets so untrusted code never sees the key; adding an environment would block PR-triggered runs run: | + # The gate decided the key is there. If it is not, the gate is broken, + # and that is a failure rather than a quiet exit 0 — the whole point + # of this job is that it never reports success without running a case. if [ -z "$ANTHROPIC_API_KEY" ]; then - echo "::warning::ANTHROPIC_API_KEY not configured, skipping skill evals" - exit 0 + echo "::error::the preflight said the evals could run, but ANTHROPIC_API_KEY is empty here" + exit 1 fi make skill-eval @@ -544,3 +627,13 @@ jobs: - name: Build the flake if: steps.filter.outputs.nix == 'true' run: scripts/nix-build-check.sh + + # `nix build` builds packages.default, so nothing here ever looked at the + # dev shell's package list — a misspelled attribute in it would land green + # and break `nix develop` for whoever followed CONTRIBUTING. Evaluating the + # derivation resolves every attribute without building any of them, which + # costs seconds and is the difference between a list that is checked and a + # list that is merely written down. + - name: Check the dev shell's package list resolves + if: steps.filter.outputs.nix == 'true' + run: nix eval --raw .#devShells.x86_64-linux.default.drvPath diff --git a/.mise.toml b/.mise.toml index f1ac9ee05..e27c26e44 100644 --- a/.mise.toml +++ b/.mise.toml @@ -13,3 +13,8 @@ powershell = "7.6.5" # Keep in sync with the zizmor-action `version:` input in # .github/workflows/test.yml so local bin/ci runs the same release CI does. zizmor = "1.30.0" +# Keep in sync with the ruby/setup-ruby `ruby-version:` inputs in +# .github/workflows/test.yml. make check compiles the skill-eval patterns under +# Ruby's own regex engine, so bin/ci needs the same Ruby CI uses — a different +# engine would answer a different question. +ruby = "3.3" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4cc6c200b..7b29bb7e4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,6 +41,8 @@ The `go.work` file is gitignored - your local setup won't affect the repo. - [bats-core](https://github.com/bats-core/bats-core) for integration tests - [golangci-lint](https://golangci-lint.run/) for linting - [jq](https://jqlang.github.io/jq/) for CLI surface checks +- Ruby 3.3 for the skill-eval pattern check, which compiles those patterns + under the same engine the eval runner uses ## Pull Request Process diff --git a/Makefile b/Makefile index 965be15a7..72da89f25 100644 --- a/Makefile +++ b/Makefile @@ -15,6 +15,7 @@ DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ") # Go parameters GOCMD := go +RUBY ?= ruby GOBUILD := $(GOCMD) build GOTEST := $(GOCMD) test GOVET := $(GOCMD) vet @@ -411,9 +412,24 @@ replace-check: check-smoke-coverage: build @scripts/check-smoke-coverage.sh +# Compile every skill-eval pattern under the engine that reads them (Ruby's +# Onigmo). This is not the evals — those need an API key CI does not have, and +# the Skill Evals job says so rather than running none and reporting success. +# It is the part of them that was landing malformed and green: the Go guard in +# internal/commands/connect_skilleval_test.go models the shape of a --serve +# value in four of the case files and nothing else, so a mock, expect_sequence +# or accept_response pattern that does not compile reached main unnoticed. +# +# Fails when ruby is missing rather than skipping: a check that cannot run and +# reports success is the defect this target exists to close. +.PHONY: check-eval-patterns +check-eval-patterns: + @command -v $(RUBY) >/dev/null || (echo "Install ruby: the skill-eval patterns are Ruby regexes and cannot be compiled without it" && exit 1) + @$(RUBY) scripts/check-eval-patterns.rb + # Run all checks (local CI gate) .PHONY: check -check: fmt-check vet lint lint-actions test test-e2e test-sync-skills check-naming check-surface check-skill-drift test-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage provenance-check tidy-check +check: fmt-check vet lint lint-actions test test-e2e test-sync-skills check-naming check-surface check-skill-drift test-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage check-eval-patterns provenance-check tidy-check # Lint GitHub Actions workflows (requires actionlint + zizmor) .PHONY: lint-actions diff --git a/flake.nix b/flake.nix index b8eff3957..685a583c5 100644 --- a/flake.nix +++ b/flake.nix @@ -38,6 +38,10 @@ gnumake jq ripgrep + # Keep in sync with .mise.toml and the ruby/setup-ruby pins in + # .github/workflows/test.yml: make check compiles the skill-eval + # patterns under Ruby's own regex engine. + ruby_3_3 zizmor ]; }; diff --git a/internal/commands/connect_skilleval_test.go b/internal/commands/connect_skilleval_test.go index 87a3d8942..47d878b24 100644 --- a/internal/commands/connect_skilleval_test.go +++ b/internal/commands/connect_skilleval_test.go @@ -14,18 +14,26 @@ import ( ) // The connector's skill evals are checked here, against this package's own -// parser, because nothing else checks them at all: CI's Skill Evals job exits -// 0 without running a case when ANTHROPIC_API_KEY is unset, which it is on -// this repository. +// parser, because CI does not run a case: the Skill Evals job needs +// ANTHROPIC_API_KEY, which this repository does not have, and the job now +// reports Skipped rather than success so nobody reads a green tick as an eval +// that ran. // // # What this holds, and what it does not // // The accept and reject patterns, and only for the shape of a --serve value -// and the removed --route flags. The mock, expect_sequence and -// accept_response patterns are read by the Ruby runner under its own regex -// semantics and are not modeled here, so a malformed one of those can still -// land without CI noticing (Copilot on #765). This is a guard on one -// invariant, not on the eval files. +// and the removed --route flags. This is a guard on one invariant, not on the +// eval files. +// +// The mock, expect_sequence and accept_response patterns are read by the Ruby +// runner under its own regex semantics, and this test models none of them — +// it compiles patterns with Go's regexp, which is RE2 and not Onigmo, so it +// cannot speak for them even where it reads them. That gap used to be the end +// of the sentence (Copilot on #765). It is now covered from the other side: +// scripts/check-eval-patterns.rb compiles every pattern in every case file +// under Onigmo itself, and make check and the Integration Tests job run it. +// A pattern that does not compile is caught there; what it means is still +// only caught by running the evals. // // One of the patterns is a blunt instrument and says so: a value of all // digits can still be too large for an int64, and no shape can see a numeric diff --git a/scripts/check-eval-patterns.rb b/scripts/check-eval-patterns.rb new file mode 100755 index 000000000..13a3168b4 --- /dev/null +++ b/scripts/check-eval-patterns.rb @@ -0,0 +1,111 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# Compile every pattern in every skill-eval case under the engine that reads +# them. The runner (skill-evals/run) is Ruby, so its patterns are Onigmo, not +# RE2 and not PCRE; a pattern that Go would reject may be fine here and a +# pattern Onigmo rejects raises RegexpError at eval time — where nothing +# catches it, because CI's Skill Evals job cannot run a case without +# ANTHROPIC_API_KEY. A malformed pattern currently lands green. +# +# This does not run the evals. It compiles what they are made of, which is the +# part that was landing unnoticed. + +require "yaml" + +root = File.expand_path("..", __dir__) +runner = File.join(root, "skill-evals", "run") +cases = Dir.glob(File.join(root, "skill-evals", "cases", "**", "*.yml")).sort + +# Where a pattern lives in a case file: a list of strings, or a list of hashes +# with the pattern under one key. Mirrors the six Regexp.new sites in the +# runner. +STRING_LISTS = %w[accept reject accept_response reject_response].freeze +MATCH_LISTS = { "mocks" => "match", "expect_sequence" => "match" }.freeze +MODELLED_SITES = STRING_LISTS.length + MATCH_LISTS.length + +errors = [] +patterns = 0 + +# A checker that has fallen behind the runner reports success over the keys it +# still knows. The runner compiles one pattern kind per Regexp.new; if that +# count moves, a pattern kind was added or removed and this list has to move +# with it. +sites = File.read(runner).scan(/Regexp\.new\(/).length +if sites != MODELLED_SITES + errors << "#{runner}: the runner compiles #{sites} pattern kinds, this checker models " \ + "#{MODELLED_SITES} (#{(STRING_LISTS + MATCH_LISTS.keys).join(", ")}) — update both together" +end + +cases.each do |path| + rel = path.delete_prefix("#{root}/") + doc = begin + YAML.safe_load_file(path) + rescue Psych::Exception => e + errors << "#{rel}: unparseable YAML: #{e.message}" + next + end + unless doc.is_a?(Hash) + errors << "#{rel}: expected a mapping at the top level" + next + end + + # Shape before content. Normalizing a malformed case into something + # compilable is how a checker reports success over input it never really + # examined: a scalar `accept:` coerced to a one-item list compiles fine here + # and raises NoMethodError in the runner, which calls .each on it, and a + # string where a mock belongs compiles fine here and raises TypeError there, + # where the entry is indexed as a mapping (Copilot on #768). + found = [] + STRING_LISTS.each do |key| + list = doc[key] + next if list.nil? # the runner reads these as `(c[key] || [])` + unless list.is_a?(Array) + errors << "#{rel}: #{key}: expected a list of patterns, got #{list.class} — the runner calls .each on it" + next + end + list.each_with_index { |pat, i| found << ["#{key}[#{i}]", pat] } + end + MATCH_LISTS.each do |key, field| + list = doc[key] + next if list.nil? + unless list.is_a?(Array) + errors << "#{rel}: #{key}: expected a list of entries, got #{list.class} — the runner iterates it" + next + end + list.each_with_index do |entry, i| + unless entry.is_a?(Hash) + errors << "#{rel}: #{key}[#{i}]: expected a mapping with a #{field}: key, got #{entry.class} — the runner indexes it as one" + next + end + found << ["#{key}[#{i}].#{field}", entry[field]] + end + end + + found.each do |key, pat| + unless pat.is_a?(String) + errors << "#{rel}: #{key}: expected a pattern string, got #{pat.class}" + next + end + patterns += 1 + begin + Regexp.new(pat) + rescue RegexpError => e + errors << "#{rel}: #{key}: /#{pat}/ does not compile: #{e.message}" + end + end +end + +# A check that compiled nothing passes for the wrong reason. Both floors are +# the assertion, not setup for it. +errors << "no case files found under skill-evals/cases" if cases.empty? +errors << "found #{cases.length} case file(s) and no patterns at all" if patterns.zero? + +if errors.empty? + puts "eval patterns: #{patterns} compiled under Onigmo across #{cases.length} case files" + exit 0 +end + +warn "eval patterns: #{errors.length} problem(s)" +errors.each { |e| warn " #{e}" } +exit 1