The drift guard counted sites; now it names them - #772
Merged
Merged
Conversation
Comparing the runner's Regexp.new count to six answers cardinality and nothing else. A runner edit that drops one pattern key and adds another in the same commit leaves the count 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 the defect the check itself exists to remove. It now reads which case keys the runner takes and requires that set to equal the keys modelled here, with the keys that carry no pattern declared by name rather than inferred. 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: swapping reject_response for another key keeps the count at six and fails on identity, and adding a seventh key fails on both. Copilot raised this on #768; the pull request merged before the fix landed, so it follows here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approved
The focused guard change correctly detects both key-identity and pattern-count drift.
Review effort: Balanced
Findings: None
What changed in this PR
Strengthens the eval-pattern drift guard by validating runner key identity as well as pattern count.
Changes:
- Classifies non-pattern case keys explicitly.
- Detects added or removed pattern-bearing keys.
- Retains cardinality checks for additional pattern sites.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
scripts/check-eval-patterns.rb |
Adds key-set synchronization between the checker and eval runner. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The pattern checker added in A check that cannot run no longer reports success carries a guard meant to keep it in lockstep with the eval runner: if the runner starts compiling a pattern this checker does not know about, the checker would go on reporting success over the keys it still knows, which is the exact failure the check exists to catch. The guard asked the wrong question. It compared the runner's
Regexp.newcount to six and nothing else.Cardinality does not answer it. A runner edit that drops one pattern key and adds another in the same commit leaves the count at six, and the checker keeps compiling a key that no longer exists while never seeing the new one — green, over input it stopped examining.
Copilot raised this on #768. That pull request merged before the fix landed, so it follows here.
The change
The checker now reads which case keys the runner actually takes and requires that set to equal the keys it models. The keys that carry no pattern are 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. 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. Swapping
reject_responsefor another key keeps the count at six and fails on identity; adding a seventh key fails on both.