A check that cannot run no longer reports success

.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

.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"

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
 

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

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
             ];
           };

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

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