Skip to content

Add multi-SKIP case to gfi-sweep eval suite - #1062

Closed
deepusnath wants to merge 4 commits into
apache:mainfrom
deepusnath:gfi-sweep-multi-skip-case
Closed

Add multi-SKIP case to gfi-sweep eval suite#1062
deepusnath wants to merge 4 commits into
apache:mainfrom
deepusnath:gfi-sweep-multi-skip-case

Conversation

@deepusnath

Copy link
Copy Markdown
Contributor

Summary

  • Every existing step-3-present-proposals case in the good-first-issue-sweep eval suite has 0 or 1 SKIP issues, so skip_count_shown_as_summary_only was only ever vacuously satisfied — a model that always returns true would pass.
  • Adds case-5-multi-skip: 1 READY issue plus 6 SKIP issues spread across all three skip_reason categories (2 security-sensitive, 2 architectural-decision, 2 deprecation-decision), so the summary-only rule and the (security: M, architectural: K, deprecation: J) breakdown format from the skill's Step 3 SKIP section actually get exercised.
  • Updates the suite README to document the new case and bump the step-3 case count from 4 to 5.

Closes #1001

Test plan

  • PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/good-first-issue-sweep/step-3-present-proposals/fixtures/ — all 5 cases load and render cleanly (exit 0).
  • Verified expected.json contains all 8 output-spec fields with correct values for the new case.

Pattern 8's eval-suite layout omitted assertions.json, so a reader
following it exactly produces cases with has_*/mention_* expected
keys that silently fall back to MANUAL grading instead of PASS/FAIL.

- Add assertions.json to the fixtures directory-layout block.
- Add a paragraph explaining the has_*/mention_* structural-key rule
  and linking to eval-driven-development.md for the format.

Closes apache#1004
Every existing step-3-present-proposals case has 0 or 1 SKIP issues, so
skip_count_shown_as_summary_only was vacuously satisfied and a model
that always returned true would pass. case-5-multi-skip adds 1 READY
issue plus 6 SKIP issues spread across all three skip_reason categories
(2 security-sensitive, 2 architectural-decision, 2 deprecation-decision)
so the summary-only rule and the (security: M, architectural: K,
deprecation: J) breakdown format actually get exercised.

Closes apache#1001
potiuk pushed a commit that referenced this pull request Aug 17, 2026
…se (#1072)

`case-2-mixed` already asserted `skip_count_shown_as_summary_only`, but with
a single SKIP, where summarising and enumerating are nearly indistinguishable
in the output — a model that lists skip titles individually could still pass.

Add `case-5-multi-skip`: 2 READY + 1 NEAR-MISS + 4 SKIP spanning all three
skip reasons (two security-sensitive, one architectural-decision, one
deprecation-decision). Enumerating is now visibly wrong, so the summary-only
rule is genuinely exercised, and the case checks it alongside
`near_miss_has_label_proposal: false` rather than in isolation. It is also
the first fixture in the suite to exercise the deprecation-decision reason.

Supersedes #1062, which proposed the same case at the same paths with a
different mix.
@potiuk

potiuk commented Aug 17, 2026

Copy link
Copy Markdown
Member

Superseded by #1072, merged as d0571f6.

Both PRs added case-5-multi-skip at the same paths in the same suite, so
only one could land — they would have conflicted on identical new files.
Nothing was wrong with this one: the counts are internally consistent
(1 READY + 6 SKIP = 7 issues, matching ready_count: 1 /
near_miss_count: 0), deprecation-decision is a documented skip_reason,
and the README counters were correctly updated.

I went with #1072 because it is the later PR, its CI was green, and keeping a
NEAR-MISS alongside the SKIP block tests skip_count_shown_as_summary_only
and near_miss_has_label_proposal: false holding at the same time rather
than each in isolation.

Your design had a real argument in its favour that I want on the record:
two SKIPs per reason (2 security / 2 architectural / 2 deprecation) stresses
the summary-only rule harder than #1072's 2/1/1, and isolates it more
cleanly given NEAR-MISS behaviour is already covered by case-2-mixed and
case-3-near-miss-only. If we want to strengthen the mix, that is now a
small edit to the merged fixture rather than a new case.

Closing as superseded, not rejected. Thanks for the second look at the same
problem — please just fold a revision into the original PR next time rather
than opening a second one, so the two don't race for the same file.

@potiuk potiuk closed this Aug 17, 2026
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.

Add a multi-SKIP case so the gfi-sweep summary-only rule can actually fail

2 participants