Skip to content

Add multi-SKIP case to good-first-issue-sweep step-3 evals - #1072

Merged
potiuk merged 1 commit into
apache:mainfrom
deepusnath:add-multi-skip-gfi-sweep-eval-case
Aug 17, 2026
Merged

Add multi-SKIP case to good-first-issue-sweep step-3 evals#1072
potiuk merged 1 commit into
apache:mainfrom
deepusnath:add-multi-skip-gfi-sweep-eval-case

Conversation

@deepusnath

Copy link
Copy Markdown
Contributor

The step-3-present-proposals fixtures only ever exercised a single SKIP issue at a time, so the "summary count only, grouped by reason" rule (issue #1001) couldn't actually fail: a model that just prints the one issue's title/reason still looks like a valid one-line summary.

case-5-multi-skip adds 4 SKIP issues spanning all three skip reasons (2 security, 1 architectural, 1 deprecation) alongside READY/NEAR-MISS issues, so a regression that lists individual SKIP titles/reasons or miscounts a category is actually caught.

Summary

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other:

Test plan

  • prek run --all-files passes
  • For Python packages touched: uv run pytest / ruff check / mypy passes
  • For Groovy bridges touched: command-line invocation tested end-to-end
  • For skill changes: eval suite passes for the affected skill
    (PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/<skill>/)
  • For skill behaviour changes: a new or updated eval fixture is included in this PR
    (a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md)
  • Other:

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — no new unrestricted host access; network reach declared in the adapter
  • Vendor neutrality — placeholders (<PROJECT>, <tracker>, <upstream>, <security-list>) used in all skill / tool prose (the check-placeholders prek hook is the mechanical gate)
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
  • Privacy LLM — private content does not reach a non-approved LLM; redactor invoked where needed

Linked issues

Notes for reviewers (optional)

The step-3-present-proposals fixtures only ever exercised a single SKIP
issue at a time, so the "summary count only, grouped by reason" rule
(issue apache#1001) couldn't actually fail: a model that just prints the one
issue's title/reason still looks like a valid one-line summary.

case-5-multi-skip adds 4 SKIP issues spanning all three skip reasons
(2 security, 1 architectural, 1 deprecation) alongside READY/NEAR-MISS
issues, so a regression that lists individual SKIP titles/reasons or
miscounts a category is actually caught.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The gap this fills is real and worth naming precisely, because
it is easy to look at the existing suite and think it was already covered.

case-2-mixed does already assert skip_count_shown_as_summary_only: true
— but with exactly one SKIP, where "shown as a summary count" and
"listed individually" are nearly indistinguishable in the output. A model
that enumerates skip titles could pass that case. With four SKIPs spanning
all three skip reasons, enumerating becomes visibly wrong, so the invariant
is genuinely exercised for the first time.

Checked:

  • Counts are internally consistent — the report header says 7 issues and
    lists exactly 2 READY (42, 88), 1 NEAR-MISS (77), and 4 SKIP (101, 102
    security-sensitive; 103 architectural-decision; 104
    deprecation-decision), matching ready_count: 2 and
    near_miss_count: 1.
  • deprecation-decision is a real value, not invented for the fixture —
    skills/good-first-issue-sweep/SKILL.md:214 declares
    security-sensitive | architectural-decision | deprecation-decision | null.
    This is also the first fixture in the suite to exercise the deprecation
    reason at all; step-2-classify covers only G5 and G6.
  • Keeping the NEAR-MISS in the mix means the case also re-verifies
    near_miss_has_label_proposal: false while several SKIPs are being
    summarised — the two rules holding simultaneously, rather than each in
    isolation.
  • Both README counters are updated: the Coverage table (4 → 5) and the
    per-case table row.

On the duplicate

This supersedes #1062, which proposed the same case-5-multi-skip at the
same paths with a different mix (1 READY + 6 SKIP, two per reason, no
NEAR-MISS). Because both create identical new files, only one could land.
I have gone with this one: it is the later PR, its CI is green, and keeping
a NEAR-MISS alongside the SKIP block tests the two invariants together.

#1062's design had a fair argument in its favour — two SKIPs per reason
stresses the summary rule harder than 2/1/1 — so if you want to strengthen
the mix later, that is a one-line change to this fixture rather than a new
case. Closing #1062 as superseded rather than rejected.

Thanks for both attempts; a second design is more useful than no second
opinion, it just needed to be one PR.


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.

@potiuk
potiuk merged commit d0571f6 into apache:main Aug 17, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants