From 4a8a715b2e3f370eb885f9699faa35ca2d5ba011 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 09:12:28 +0200 Subject: [PATCH 1/3] A check that cannot run no longer reports success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Skill Evals job exited 0 when ANTHROPIC_API_KEY was unset, which it is here: no repository secret, no organization secret, and fork pull requests never receive one. Every eval file was covered by a green tick that meant nothing had run. The key is genuinely unavailable, so this is a skip rather than a failure — failing every pull request that touches a skill would be noise nobody reads. But a step-level `if` leaves the job green, so the job is split: a preflight decides whether the evals can run, and Skill Evals now reports Skipped when they cannot, with a warning annotation and a step summary naming how many cases were not run. test.yml is in the paths filter, so this pull request proves it on itself. And the guard in connect_skilleval_test.go named a limitation it could not enforce: the mock, expect_sequence and accept_response patterns are read by the Ruby runner under Onigmo, and a Go test compiling with RE2 cannot speak for them. scripts/check-eval-patterns.rb closes it from the other side — it compiles every pattern in every case file under Onigmo itself, run from make check and from the Integration Tests job. It does not run the evals. It catches the malformed pattern that was landing unnoticed: a broken mock in a connector case passes the Go guard today and fails this one. Its own floors are the assertion rather than setup for it — no case files found, no patterns compiled, or a runner that compiles more pattern kinds than this checker models are each a failure, because a checker that has fallen behind reports success over the keys it still knows. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 82 ++++++++++++++++++-- Makefile | 18 ++++- internal/commands/connect_skilleval_test.go | 24 ++++-- scripts/check-eval-patterns.rb | 84 +++++++++++++++++++++ 4 files changed, 193 insertions(+), 15 deletions(-) create mode 100755 scripts/check-eval-patterns.rb diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index adb07fddd..663c92e57 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,19 @@ 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 if: github.event_name == 'pull_request' + outputs: + run: ${{ steps.decide.outputs.run }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -246,26 +263,79 @@ 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: | + 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 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/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..495acb828 --- /dev/null +++ b/scripts/check-eval-patterns.rb @@ -0,0 +1,84 @@ +#!/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 + + found = STRING_LISTS.flat_map { |key| Array(doc[key]).map { |pat| [key, pat] } } + MATCH_LISTS.each do |key, field| + found += Array(doc[key]).map { |entry| ["#{key}[].#{field}", entry.is_a?(Hash) ? entry[field] : entry] } + 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 From 34f351858686a0010b750c7186ccc837b297d03f Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 09:30:31 +0200 Subject: [PATCH 2/3] Close the review's two findings, both the same shape as the PR's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preflight declared contents: read only, while dorny/paths-filter enumerates the pull request's files through the REST API. It works on this repository because the repository is public, which is incidental rather than a reason. Granted pull-requests: read, matching the other path-filter job in this workflow. The worse half of that finding is what the preflight does when the filter cannot answer. At this pinned version the action fails the step rather than returning an empty set, so the gate goes red rather than quietly deciding nothing changed — but the gate should not depend on that. It now refuses any answer that is not true or false: a preflight that cannot tell must not be allowed to say no, which is this pull request's own defect one level up. And the pattern checker normalized malformed cases into compilable ones. A scalar accept: became a one-item list and compiled; a string where a mock belongs was compiled as if it were the mock's pattern. Both pass a checker that coerces and crash the runner, which calls .each on the first and indexes the second as a mapping — NoMethodError and TypeError respectively, confirmed against ruby before the fix. Shape is now checked before content, with a nil value still allowed because the runner reads these as (c[key] || []). Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 13 +++++++++++++ scripts/check-eval-patterns.rb | 31 +++++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 663c92e57..457e703e5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -251,6 +251,7 @@ jobs: 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 }} @@ -281,6 +282,18 @@ jobs: # 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" diff --git a/scripts/check-eval-patterns.rb b/scripts/check-eval-patterns.rb index 495acb828..13a3168b4 100755 --- a/scripts/check-eval-patterns.rb +++ b/scripts/check-eval-patterns.rb @@ -50,9 +50,36 @@ next end - found = STRING_LISTS.flat_map { |key| Array(doc[key]).map { |pat| [key, pat] } } + # 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| - found += Array(doc[key]).map { |entry| ["#{key}[].#{field}", entry.is_a?(Hash) ? entry[field] : entry] } + 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| From 61aade9c4716b0368086ceb73333bd9a025479f4 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 09:43:56 +0200 Subject: [PATCH 3/3] Ruby in both documented setups, and a dev shell list that is checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit make check now runs check-eval-patterns on every invocation, and bin/ci is make check. Neither .mise.toml nor the flake's dev shell provided Ruby, so CONTRIBUTING's "the development shell provides Go and the tools required by bin/ci" stopped being true the moment that target was added — a contributor following the documented setup exactly would fail the standard local gate and read it as their machine being wrong. Pinned at 3.3 in both, matching the ruby/setup-ruby pins in test.yml, with the sync comment .mise.toml keeps for every other pin. Verified rather than assumed: mise install fetches 3.3.12 and the check runs under it. The flake side could not be verified the same way — there is no nix on this machine — and it turned out nothing verified it anywhere either, because the nix job builds packages.default and never looks at the dev shell. A misspelled attribute in that list lands green and breaks nix develop for whoever follows CONTRIBUTING, which is this pull request's subject in another file. The job now evaluates the dev shell derivation, which resolves every attribute in seconds without building any of them. ruby_3_3 exists at the nixpkgs revision flake.lock pins, read from that revision directly. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 10 ++++++++++ .mise.toml | 5 +++++ CONTRIBUTING.md | 2 ++ flake.nix | 4 ++++ 4 files changed, 21 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 457e703e5..4abd110f9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -627,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/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 ]; };