fix(skill-evals): correct blocked drop reason - #1061
Conversation
…blocked `gate:G5` already means "failed Stage-1 gate G5" — the batch-level obviously-conflicting cull — so reusing it for the Stage-3 mergeable-but-blocked outcome gives one code two meanings, which is the conflation apache#949 set out to remove. Rename the Stage-3 code to `gate:G5-blocked` and add the row the authoritative drop-reason taxonomy in candidate-rules.md was missing, so the eval fixture and the skill agree on the code and on how it is reported. Generated-by: Claude Code (Opus 5)
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Good catch on the root cause — the fixture really was mislabelling a
mergeable-but-blocked PR as a merge conflict, and #949's own hint to
"find the one describing a blocked-but-mergeable PR" was wrong:
candidate-rules.md had no code for that state at all. So the issue was
under-specified, not something you missed.
I've pushed a fixup to your branch rather than send you round again for
it. Two changes on top of yours:
gate:G5→gate:G5-blocked. A baregate:G5already means "failed
Stage-1 gate G5" — the batch-level obviously conflicting cull —
so it would have given one code two meanings, which is the same
conflation #949 set out to remove. The sibling Stage-3 codes are
suffixed (gate:G5-conflict,gate:G5-unknown) and this one now
matches.- Added the missing row to
candidate-rules.md§ Drop-reason
taxonomy, so the authoritative doc actually defines the code your
fixture asserts, plus the reporting line —gate:G5-blockedjoins
gate:G5-conflictin the drops reported with PR numbers, since a
mergeable-but-blocked PR is exactly the "so-close" case a maintainer
wants to glance at.
Everything else was right and I left it alone: case-5 matches the
shape of case-2 / case-3, the SPDX header is on the new report.md,
the suite auto-discovers case directories so no step-config.json
change was needed, and the README case count is updated. prek run is
green and all five stage-3 cases render.
Thanks for picking this one up — it was a genuine correctness bug in the
fixture, and finding that the taxonomy was missing an entry is the more
useful half of it.
Worth a second look from
This change touches tools/skill-evals/evals/pr-management-quick-merge/;
folks with the most context here:
@justinmclean— authored 2 of the last 5 commits under that path (committer)
They have not been notified — asking for an extra pass is the
maintainer's call, and optional.
This review was drafted by an AI-assisted tool and
confirmed by a Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Magpie handles maintainer review:
CONTRIBUTING.md.
2 participants
Summary
gate:G5-conflicttogate:G5, matching the authoritative candidate rules.gate:G5-conflictreserved for genuine merge conflicts.Type of change
Test plan
uv run --project tools/skill-evals pytest tools/skill-evals/tests -qpassesprekand commit-time hooks passprek run --all-filespasses (the local one-vCPU runner exceeded its bounded execution window; CI will run the full repository gates)Linked issues
Closes #949
Notes for reviewers
The new case isolates
mergeable=true,mergeable_state=blocked, andreviewDecision=APPROVED. The prior fixture text mislabeled this non-conflict state asgate:G5-conflict;gate:G5is the existing non-approval blocker code.