Skip to content

Commit b1e3be3

Browse files
A check that cannot run no longer reports success (#768)
* A check that cannot run no longer reports success 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) <noreply@anthropic.com> * Close the review's two findings, both the same shape as the PR's 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) <noreply@anthropic.com> * Ruby in both documented setups, and a dev shell list that is checked 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 1740952 commit b1e3be3

7 files changed

Lines changed: 254 additions & 15 deletions

File tree

‎.github/workflows/test.yml‎

Lines changed: 99 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,16 @@ jobs:
193193
- name: Test the skill drift check
194194
run: make test-skill-drift
195195

196+
# Same Ruby the eval runner gets, because the point of the check is the
197+
# engine: these patterns are Onigmo, and Go's RE2 guard cannot see them.
198+
- name: Set up Ruby
199+
uses: ruby/setup-ruby@95ef2b042f9d7a56d8268cba8559e2842e2ad01b # v1.321.0
200+
with:
201+
ruby-version: '3.3'
202+
203+
- name: Compile the skill-eval patterns
204+
run: make check-eval-patterns
205+
196206
cli-surface:
197207
name: CLI Surface Check
198208
runs-on: ubuntu-latest
@@ -231,12 +241,20 @@ jobs:
231241
if: always()
232242
run: git worktree remove /tmp/baseline-tree --force 2>/dev/null || true
233243

234-
skill-eval:
235-
name: Skill Evals
244+
# Whether the evals can run at all, answered before the job that would run
245+
# them. Split out because a job cannot skip itself: a step-level `if` leaves
246+
# the job green, and "Skill Evals: success" with no case run is the reading
247+
# this repository actually had. A `needs` + job-level `if` reports Skipped,
248+
# which is what happened.
249+
skill-eval-gate:
250+
name: Skill Evals (preflight)
236251
runs-on: ubuntu-latest
237252
permissions:
238253
contents: read
254+
pull-requests: read # dorny/paths-filter enumerates PR files via the REST API
239255
if: github.event_name == 'pull_request'
256+
outputs:
257+
run: ${{ steps.decide.outputs.run }}
240258
steps:
241259
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
242260
with:
@@ -246,26 +264,91 @@ jobs:
246264
id: filter
247265
uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3
248266
with:
267+
# test.yml is in its own filter so the job proves itself on the PR
268+
# that introduces or edits it.
249269
filters: |
250270
skill:
251271
- 'skills/basecamp/SKILL.md'
252272
- 'skills/basecamp-connect/SKILL.md'
253273
- 'skill-evals/**'
274+
- '.github/workflows/test.yml'
275+
276+
- name: Decide whether the evals can run
277+
id: decide
278+
env:
279+
CHANGED: ${{ steps.filter.outputs.skill }}
280+
# Presence only, never the value. Fork PRs receive no secrets, so this
281+
# is false there by construction and the skip below is the honest
282+
# answer for them too.
283+
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
284+
run: |
285+
# A preflight that cannot tell must not be allowed to say no. The
286+
# filter answers true or false; anything else means it did not answer,
287+
# and treating that as "nothing to run" would rebuild the defect this
288+
# job exists to remove, one level up (Copilot on #768).
289+
case "$CHANGED" in
290+
true|false) ;;
291+
*)
292+
echo "::error::the paths filter did not answer (skill='$CHANGED'), so this preflight cannot say whether the evals are needed"
293+
exit 1
294+
;;
295+
esac
296+
297+
if [ "$CHANGED" != "true" ]; then
298+
echo "run=false" >> "$GITHUB_OUTPUT"
299+
echo "No skill or eval changes on this pull request." >> "$GITHUB_STEP_SUMMARY"
300+
exit 0
301+
fi
302+
303+
cases=$(find skill-evals/cases -name '*.yml' | wc -l | tr -d ' ')
304+
305+
if [ "$HAS_KEY" != "true" ]; then
306+
echo "run=false" >> "$GITHUB_OUTPUT"
307+
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."
308+
{
309+
echo "### Skill Evals: NOT RUN"
310+
echo
311+
echo "This pull request changes a skill or its evals, and **$cases eval cases were not run**."
312+
echo "\`ANTHROPIC_API_KEY\` is not configured for this repository, and fork pull requests never receive secrets."
313+
echo
314+
echo "What still covers these files:"
315+
echo "- \`make check-eval-patterns\` compiles every pattern in every case under Onigmo, the engine the runner uses."
316+
echo "- \`TestConnectSkillEvalRejectsHoldTheServeValueRule\` holds the \`--serve\` value rule in the connector cases."
317+
echo
318+
echo "Neither runs a case. If these evals must gate this repository, configure the secret; until then this job is Skipped rather than green."
319+
} >> "$GITHUB_STEP_SUMMARY"
320+
exit 0
321+
fi
322+
323+
echo "run=true" >> "$GITHUB_OUTPUT"
324+
325+
skill-eval:
326+
name: Skill Evals
327+
runs-on: ubuntu-latest
328+
permissions:
329+
contents: read
330+
needs: skill-eval-gate
331+
if: needs.skill-eval-gate.outputs.run == 'true'
332+
steps:
333+
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
334+
with:
335+
persist-credentials: false
254336

255337
- name: Set up Ruby
256-
if: steps.filter.outputs.skill == 'true'
257338
uses: ruby/setup-ruby@95ef2b042f9d7a56d8268cba8559e2842e2ad01b # v1.321.0
258339
with:
259340
ruby-version: '3.3'
260341

261342
- name: Run skill evals
262-
if: steps.filter.outputs.skill == 'true'
263343
env:
264344
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
265345
run: |
346+
# The gate decided the key is there. If it is not, the gate is broken,
347+
# and that is a failure rather than a quiet exit 0 — the whole point
348+
# of this job is that it never reports success without running a case.
266349
if [ -z "$ANTHROPIC_API_KEY" ]; then
267-
echo "::warning::ANTHROPIC_API_KEY not configured, skipping skill evals"
268-
exit 0
350+
echo "::error::the preflight said the evals could run, but ANTHROPIC_API_KEY is empty here"
351+
exit 1
269352
fi
270353
make skill-eval
271354
@@ -544,3 +627,13 @@ jobs:
544627
- name: Build the flake
545628
if: steps.filter.outputs.nix == 'true'
546629
run: scripts/nix-build-check.sh
630+
631+
# `nix build` builds packages.default, so nothing here ever looked at the
632+
# dev shell's package list — a misspelled attribute in it would land green
633+
# and break `nix develop` for whoever followed CONTRIBUTING. Evaluating the
634+
# derivation resolves every attribute without building any of them, which
635+
# costs seconds and is the difference between a list that is checked and a
636+
# list that is merely written down.
637+
- name: Check the dev shell's package list resolves
638+
if: steps.filter.outputs.nix == 'true'
639+
run: nix eval --raw .#devShells.x86_64-linux.default.drvPath

‎.mise.toml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,8 @@ powershell = "7.6.5"
1313
# Keep in sync with the zizmor-action `version:` input in
1414
# .github/workflows/test.yml so local bin/ci runs the same release CI does.
1515
zizmor = "1.30.0"
16+
# Keep in sync with the ruby/setup-ruby `ruby-version:` inputs in
17+
# .github/workflows/test.yml. make check compiles the skill-eval patterns under
18+
# Ruby's own regex engine, so bin/ci needs the same Ruby CI uses — a different
19+
# engine would answer a different question.
20+
ruby = "3.3"

‎CONTRIBUTING.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ The `go.work` file is gitignored - your local setup won't affect the repo.
4141
- [bats-core](https://github.com/bats-core/bats-core) for integration tests
4242
- [golangci-lint](https://golangci-lint.run/) for linting
4343
- [jq](https://jqlang.github.io/jq/) for CLI surface checks
44+
- Ruby 3.3 for the skill-eval pattern check, which compiles those patterns
45+
under the same engine the eval runner uses
4446

4547
## Pull Request Process
4648

‎Makefile‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ")
1515

1616
# Go parameters
1717
GOCMD := go
18+
RUBY ?= ruby
1819
GOBUILD := $(GOCMD) build
1920
GOTEST := $(GOCMD) test
2021
GOVET := $(GOCMD) vet
@@ -411,9 +412,24 @@ replace-check:
411412
check-smoke-coverage: build
412413
@scripts/check-smoke-coverage.sh
413414

415+
# Compile every skill-eval pattern under the engine that reads them (Ruby's
416+
# Onigmo). This is not the evals — those need an API key CI does not have, and
417+
# the Skill Evals job says so rather than running none and reporting success.
418+
# It is the part of them that was landing malformed and green: the Go guard in
419+
# internal/commands/connect_skilleval_test.go models the shape of a --serve
420+
# value in four of the case files and nothing else, so a mock, expect_sequence
421+
# or accept_response pattern that does not compile reached main unnoticed.
422+
#
423+
# Fails when ruby is missing rather than skipping: a check that cannot run and
424+
# reports success is the defect this target exists to close.
425+
.PHONY: check-eval-patterns
426+
check-eval-patterns:
427+
@command -v $(RUBY) >/dev/null || (echo "Install ruby: the skill-eval patterns are Ruby regexes and cannot be compiled without it" && exit 1)
428+
@$(RUBY) scripts/check-eval-patterns.rb
429+
414430
# Run all checks (local CI gate)
415431
.PHONY: check
416-
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
432+
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
417433

418434
# Lint GitHub Actions workflows (requires actionlint + zizmor)
419435
.PHONY: lint-actions

‎flake.nix‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@
3838
gnumake
3939
jq
4040
ripgrep
41+
# Keep in sync with .mise.toml and the ruby/setup-ruby pins in
42+
# .github/workflows/test.yml: make check compiles the skill-eval
43+
# patterns under Ruby's own regex engine.
44+
ruby_3_3
4145
zizmor
4246
];
4347
};

‎internal/commands/connect_skilleval_test.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,26 @@ import (
1414
)
1515

1616
// The connector's skill evals are checked here, against this package's own
17-
// parser, because nothing else checks them at all: CI's Skill Evals job exits
18-
// 0 without running a case when ANTHROPIC_API_KEY is unset, which it is on
19-
// this repository.
17+
// parser, because CI does not run a case: the Skill Evals job needs
18+
// ANTHROPIC_API_KEY, which this repository does not have, and the job now
19+
// reports Skipped rather than success so nobody reads a green tick as an eval
20+
// that ran.
2021
//
2122
// # What this holds, and what it does not
2223
//
2324
// The accept and reject patterns, and only for the shape of a --serve value
24-
// and the removed --route flags. The mock, expect_sequence and
25-
// accept_response patterns are read by the Ruby runner under its own regex
26-
// semantics and are not modeled here, so a malformed one of those can still
27-
// land without CI noticing (Copilot on #765). This is a guard on one
28-
// invariant, not on the eval files.
25+
// and the removed --route flags. This is a guard on one invariant, not on the
26+
// eval files.
27+
//
28+
// The mock, expect_sequence and accept_response patterns are read by the Ruby
29+
// runner under its own regex semantics, and this test models none of them —
30+
// it compiles patterns with Go's regexp, which is RE2 and not Onigmo, so it
31+
// cannot speak for them even where it reads them. That gap used to be the end
32+
// of the sentence (Copilot on #765). It is now covered from the other side:
33+
// scripts/check-eval-patterns.rb compiles every pattern in every case file
34+
// under Onigmo itself, and make check and the Integration Tests job run it.
35+
// A pattern that does not compile is caught there; what it means is still
36+
// only caught by running the evals.
2937
//
3038
// One of the patterns is a blunt instrument and says so: a value of all
3139
// digits can still be too large for an int64, and no shape can see a numeric

‎scripts/check-eval-patterns.rb‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
#!/usr/bin/env ruby
2+
# frozen_string_literal: true
3+
4+
# Compile every pattern in every skill-eval case under the engine that reads
5+
# them. The runner (skill-evals/run) is Ruby, so its patterns are Onigmo, not
6+
# RE2 and not PCRE; a pattern that Go would reject may be fine here and a
7+
# pattern Onigmo rejects raises RegexpError at eval time — where nothing
8+
# catches it, because CI's Skill Evals job cannot run a case without
9+
# ANTHROPIC_API_KEY. A malformed pattern currently lands green.
10+
#
11+
# This does not run the evals. It compiles what they are made of, which is the
12+
# part that was landing unnoticed.
13+
14+
require "yaml"
15+
16+
root = File.expand_path("..", __dir__)
17+
runner = File.join(root, "skill-evals", "run")
18+
cases = Dir.glob(File.join(root, "skill-evals", "cases", "**", "*.yml")).sort
19+
20+
# Where a pattern lives in a case file: a list of strings, or a list of hashes
21+
# with the pattern under one key. Mirrors the six Regexp.new sites in the
22+
# runner.
23+
STRING_LISTS = %w[accept reject accept_response reject_response].freeze
24+
MATCH_LISTS = { "mocks" => "match", "expect_sequence" => "match" }.freeze
25+
MODELLED_SITES = STRING_LISTS.length + MATCH_LISTS.length
26+
27+
errors = []
28+
patterns = 0
29+
30+
# A checker that has fallen behind the runner reports success over the keys it
31+
# still knows. The runner compiles one pattern kind per Regexp.new; if that
32+
# count moves, a pattern kind was added or removed and this list has to move
33+
# with it.
34+
sites = File.read(runner).scan(/Regexp\.new\(/).length
35+
if sites != MODELLED_SITES
36+
errors << "#{runner}: the runner compiles #{sites} pattern kinds, this checker models " \
37+
"#{MODELLED_SITES} (#{(STRING_LISTS + MATCH_LISTS.keys).join(", ")}) — update both together"
38+
end
39+
40+
cases.each do |path|
41+
rel = path.delete_prefix("#{root}/")
42+
doc = begin
43+
YAML.safe_load_file(path)
44+
rescue Psych::Exception => e
45+
errors << "#{rel}: unparseable YAML: #{e.message}"
46+
next
47+
end
48+
unless doc.is_a?(Hash)
49+
errors << "#{rel}: expected a mapping at the top level"
50+
next
51+
end
52+
53+
# Shape before content. Normalizing a malformed case into something
54+
# compilable is how a checker reports success over input it never really
55+
# examined: a scalar `accept:` coerced to a one-item list compiles fine here
56+
# and raises NoMethodError in the runner, which calls .each on it, and a
57+
# string where a mock belongs compiles fine here and raises TypeError there,
58+
# where the entry is indexed as a mapping (Copilot on #768).
59+
found = []
60+
STRING_LISTS.each do |key|
61+
list = doc[key]
62+
next if list.nil? # the runner reads these as `(c[key] || [])`
63+
unless list.is_a?(Array)
64+
errors << "#{rel}: #{key}: expected a list of patterns, got #{list.class} — the runner calls .each on it"
65+
next
66+
end
67+
list.each_with_index { |pat, i| found << ["#{key}[#{i}]", pat] }
68+
end
69+
MATCH_LISTS.each do |key, field|
70+
list = doc[key]
71+
next if list.nil?
72+
unless list.is_a?(Array)
73+
errors << "#{rel}: #{key}: expected a list of entries, got #{list.class} — the runner iterates it"
74+
next
75+
end
76+
list.each_with_index do |entry, i|
77+
unless entry.is_a?(Hash)
78+
errors << "#{rel}: #{key}[#{i}]: expected a mapping with a #{field}: key, got #{entry.class} — the runner indexes it as one"
79+
next
80+
end
81+
found << ["#{key}[#{i}].#{field}", entry[field]]
82+
end
83+
end
84+
85+
found.each do |key, pat|
86+
unless pat.is_a?(String)
87+
errors << "#{rel}: #{key}: expected a pattern string, got #{pat.class}"
88+
next
89+
end
90+
patterns += 1
91+
begin
92+
Regexp.new(pat)
93+
rescue RegexpError => e
94+
errors << "#{rel}: #{key}: /#{pat}/ does not compile: #{e.message}"
95+
end
96+
end
97+
end
98+
99+
# A check that compiled nothing passes for the wrong reason. Both floors are
100+
# the assertion, not setup for it.
101+
errors << "no case files found under skill-evals/cases" if cases.empty?
102+
errors << "found #{cases.length} case file(s) and no patterns at all" if patterns.zero?
103+
104+
if errors.empty?
105+
puts "eval patterns: #{patterns} compiled under Onigmo across #{cases.length} case files"
106+
exit 0
107+
end
108+
109+
warn "eval patterns: #{errors.length} problem(s)"
110+
errors.each { |e| warn " #{e}" }
111+
exit 1

0 commit comments

Comments
 (0)