A check that cannot run no longer reports success

[jorgemanrubia]

The Skill Evals job has been passing without running a single eval. It needs ANTHROPIC_API_KEY, and this repository has none — no repository secret, no organization secret, and fork pull requests never receive one. The job noticed, printed a warning, and exited 0. So every file under skill-evals/ carried a green tick that meant nothing had been checked, which is worse than no check at all: it occupies the place a reader looks for assurance.

Originally tracked in The Skill Evals job reports success without running a case.

The second half of the same defect is a comment. The guard added in The connector runs where it is started: no directory is a project's is honest that it models the shape of a --serve value and nothing else — 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 even where it reads them. A malformed pattern in any of those three lands with CI green.

Which way the first half goes

A skip, not a failure. The key is genuinely unavailable here, and fork pull requests are structurally unable to receive one, so failing every pull request that touches a skill would produce a red everyone learns to click past. The job should say what it means instead.

A step-level if cannot do that — it leaves the job green. So the job is split: a preflight decides whether the evals can run, and Skill Evals runs only on its say-so. When a pull request changes a skill or its evals and the key is absent, the job now reports Skipped rather than success, and the preflight emits a warning annotation and a step summary naming how many cases were not run and what does still cover them. test.yml is in the paths filter, so this pull request demonstrates it on itself.

Skill Evals is not a required check, so nothing is blocked by it going grey.

Closing the second half

scripts/check-eval-patterns.rb compiles every pattern in every case file under the engine that reads them, run from make check and from the Integration Tests job. It does not run the evals. It catches the pattern that does not compile, which is what was landing unnoticed: break a mocks pattern in a connector case and the Go guard still passes while this one fails.

Its own floors are the assertion rather than setup for it. No case files found, no patterns compiled at all, or a runner that compiles more pattern kinds than the checker models are each a failure — a checker that has quietly fallen behind the runner would otherwise report success over the keys it still happens to know.

[github-actions]

Sensitive Change Detection (shadow mode)

This PR modifies control-plane files:

• .github/workflows/test.yml

Shadow mode — this check is informational only. When activated, changes to these paths will require approval from a maintainer.

[Copilot]

Copilot review overview

🟡 Changes recommended

The preflight lacks required pull-request permissions, and malformed case structures can pass validation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Grant pull-requests read permission to path-filter job · New
• Validate case structure types before normalization · New

What changed in this PR

Makes skill-eval CI accurately report when evaluations cannot run and adds Ruby-native regex validation.

Changes:

• Adds a preflight gate for honest skipped status.
• Compiles eval patterns under Onigmo during integration checks.
• Clarifies the Go guard’s limited scope.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File	Description
scripts/​check-eval-patterns.rb	Adds Ruby pattern validation.
Makefile	Integrates validation into make check.
internal/​commands/​connect_skilleval_test.go	Documents test coverage boundaries.
.github/​workflows/​test.yml	Adds validation and the eval preflight gate.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The documented mise and Nix environments do not provide the newly mandatory Ruby runtime.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Grant pull-requests read permission to path-filter job
• Validate case structure types before normalization

Previously missed (1)

In code that hasn't changed since last review

Add Ruby to documented mise and Nix development environments

Makefile:432

check now always runs check-eval-patterns, but neither documented development setup provides Ruby. bin/setup installs only the tools in .mise.toml, which has no Ruby entry, and the Nix shell package list in flake.nix:31-42 also omits it despite CONTRIBUTING.md:21 saying the shell provides every bin/ci tool. A contributor starting from either supported setup will therefore fail the standard local CI gate at this target. Add a pinned Ruby version to the mise and Nix environments and document it in the requirements.

[jorgemanrubia]

🤖 Correct, and it is my change that made CONTRIBUTING.md:21 false — bin/ci is make check, and make check now needs Ruby. Fixed in 61aade9: ruby = "3.3" in .mise.toml with the sync comment that file keeps for every other pin, ruby_3_3 in the flake's dev shell, and a line in Requirements.

Verified rather than asserted, and the two halves are not equally verified:

• mise: mise install ruby@3.3 fetched 3.3.12 here and mise exec -- ruby scripts/check-eval-patterns.rb ran clean under it. That one I ran.
• nix: there is no nix on this machine, so I did not run nix develop and am not claiming I did. What I did check is that ruby_3_3 exists at the exact nixpkgs revision flake.lock pins (c27cdad), by reading pkgs/development/interpreters/ruby/default.nix and pkgs/top-level/all-packages.nix at that revision.

That gap was worth more than a disclaimer, because nothing else was checking it either: the nix job runs nix build, which builds packages.default and never touches the dev shell, so a misspelled attribute in that package list would land green and break nix develop for whoever followed CONTRIBUTING. Same shape as this pull request, one file over. The job now also evaluates the dev shell derivation, which resolves every attribute without building any of them — seconds, and it turns a list that was merely written down into one that is checked. It runs on this pull request, so the flake half is verified by CI rather than by me.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The preflight currently prevents existing push and manual Skill Eval executions.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Preflight skips evals on push and manual triggers

.github/​workflows/​test.yml:255

This condition skips the preflight on both push to main and workflow_dispatch, even though both remain top-level workflow triggers. Because skill-eval needs this job and only runs when its run output is true, the eval job is then skipped unconditionally on those events. Previously it could run after its path filter, so even configuring ANTHROPIC_API_KEY would no longer enable post-merge or manual eval runs. Preserve these trigger paths and make the preflight's change detection event-aware.

Drift guard validates pattern count but not pattern identity

scripts/​check-eval-patterns.rb:37

The drift guard compares only the number of Regexp.new calls with six. If a runner change removes one modeled pattern site and adds a new case-file pattern key in the same edit, the count remains six; this checker continues scanning the removed key, ignores the new one, and can pass a malformed regex. Verify the identity of each modeled runner site, or share the pattern schema with the runner, rather than checking cardinality alone.

[jorgemanrubia]

🤖 On the "Preflight skips evals on push and manual triggers" note — the regression half of this is not right, and I checked before acting on it.

skill-eval on main already carries if: github.event_name == 'pull_request' (.github/workflows/test.yml:239). Evals have never run on push or workflow_dispatch, with or without ANTHROPIC_API_KEY. Under this change the preflight inherits that same condition, so on those events the gate is skipped, its run output is empty, and skill-eval is skipped — the identical outcome to today, reached one job later.

So nothing about post-merge or manual eval runs is lost here, because there was nothing there to lose.

The underlying observation is still worth recording, just not as a regression: the evals cannot run on push or manually at all, and that predates this change. Whether they should is a real question — a post-merge eval run would catch a case file that only breaks once merged — but it is a scope decision about when evals run, not about whether a job lies when it cannot run. That is this change's subject, and it is settled: Skill Evals now reports Skipped instead of Success when it cannot execute.

[jorgemanrubia]

🤖 Two findings in the latest review. One is right, one is not.

Preflight skips evals on push and manual triggers — not correct, and worth being precise about. The finding says "previously it could run after its path filter" on push and workflow_dispatch. It could not. origin/main's skill-eval job carried if: github.event_name == 'pull_request' at line 239, immediately above the checkout, and that condition is the one I moved to the preflight verbatim. Skill Evals has never run on a push to main or on a manual dispatch, before this pull request or after it — git show origin/main:.github/workflows/test.yml has it. Behaviour on those two triggers is unchanged, so there is no path to preserve here. Whether the evals ought to run post-merge is a real question, but it is a different change and it was not true before.

The drift guard checks cardinality, not identity — correct, and it was the weakest thing in the file. A runner edit that drops one pattern key and adds another in the same commit leaves Regexp.new at six, and the checker would go on compiling the key that no longer exists while never seeing the new one. A guard reporting success over input it stopped examining, which is what this whole pull request is about.

Fixed in 149a4e5. The checker now reads which case keys the runner actually takes and requires that set to equal the keys it models, with the keys that carry no pattern (context, file, max_commands, tags, task) declared by name rather than inferred — so a key the runner starts reading has to be classified by someone, and the failure when it is not is this check going red. Both directions fail: a pattern key the checker does not compile, and a modelled key the runner no longer reads. The count check stays, because it still catches a second pattern field added to a key already modelled, which leaves the key set unchanged.

Proved by breaking the runner both ways before trusting it:

$ # swap reject_response for deny_response — count still 6
$ ruby scripts/check-eval-patterns.rb
eval patterns: 2 problem(s)
  skill-evals/run: reads c["deny_response"], which this checker does not compile — add it to STRING_LISTS or MATCH_LISTS, or to NON_PATTERN_KEYS if it carries no pattern
  skill-evals/run: no longer reads c["reject_response"], but this checker still compiles it