Repository navigation
Fix STALE-CANDIDATE inconsistency in issue-backlog-stats skill - #1754
HarshRajSinghania wants to merge 2 commits into
Conversation
Remove STALE-CANDIDATE as a triage class since it is orthogonal to the primary classification. Document is_stale_candidate as a separate flag and update the aggregation and output spec accordingly. Closes apache#1678
Remove STALE-CANDIDATE as a triage class since it is orthogonal to the primary classification. Document is_stale_candidate as a separate flag and update the aggregation and output spec accordingly. Closes apache#1678
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
Thank you for working on #1678!
Review Summary
The core objective of this PR is spot-on: resolving the contradiction in #1678 by removing STALE-CANDIDATE as a fifth triage class, treating staleness strictly as an orthogonal boolean flag (is_stale_candidate), and updating tools/skill-evals/evals/issue-backlog-stats/step-2-classify/fixtures/output-spec.md.
However, the PR currently introduces substantial unintended deletions and modifications across SKILL.md:
Major Findings
-
Accidental Deletion of Core Skill Sections & Surface Hash Invalidation
- Several required sections were removed from
plugins/magpie-issue/skills/backlog-stats/SKILL.md:## Step 6 — Output## What this skill does NOT do## Budget discipline## Failure modes## References
- Section 5 ("Dashboard layout") was truncated from 9 detailed specifications (covering 4 hero cards with color thresholds, ASCII/HTML bar chart formats, staleness panels, and empty-area fallbacks) into a 7-bullet summary.
- Deleting structural
##headings breaks thesurface_hashverification (tools/dev/skill-surface-hash.py), drifting fromsha256:0f124437a9fa54f9tosha256:75b53c33f1cb749c. measured_tokensalso drifted significantly (from 4984 down to 3939).
- Several required sections were removed from
-
Modifications Inside the Framework Preflight Region
- The region between
<!-- START MAGPIE PREFLIGHT -->and<!-- END MAGPIE PREFLIGHT -->is a framework-managed template (tools/dev/preflight-block.md/tools/dev/check-shared-blocks.py) and should not be modified manually. - In particular, line 57 changed
$(git rev-parse --git-common-dir)/../.apache-magpie-localto$(git rev-parse --git-common-dir)/.apache-magpie-local. In a linked git worktree,--git-common-dirreturns<repo>/.git; removing/..points inside.gitwhere.apache-magpie-localdoes not exist, breaking preflight discovery. - Standard slash commands (
/magpie-setup:setup config, etc.) were changed to@magpie-setup, which deviates from the repository convention.
- The region between
Minor / Git History
- The branch contains two commits (
bc58d4c5and2e95e503) with identical commit messages, where the first commit deleted 398 lines and the second attempted a partial restoration. - Once the extra edits are reverted and only the targeted fix for #1678 is retained, squashing down to a single clean commit on top of
mainwill keep git history clean.
Recommendation
To resolve #1678 cleanly, the diff on SKILL.md should be narrowed down to just:
- Removing the
STALE-CANDIDATErow from the Step 2 class table. - Adding the clarifying note on
is_stale_candidate. - Updating the Step 3 aggregation bullet for
stale_candidate. - The schema change in
output-spec.md(which is already correct).
|
|
||
| ```bash | ||
| PYTHONPATH=".apache-magpie-local:$(git rev-parse --git-common-dir)/../.apache-magpie-local:$(git rev-parse --git-common-dir)/apache-magpie" \ | ||
| PYTHONPATH=".apache-magpie-local:$(git rev-parse --git-common-dir)/.apache-magpie-local:$(git rev-parse --git-common-dir)/apache-magpie" \ |
There was a problem hiding this comment.
The <!-- START MAGPIE PREFLIGHT --> block is a framework-managed region. Also, removing /.. breaks preflight discovery for linked worktrees where --git-common-dir points to <main>/.git.
| ``` | ||
|
|
||
| The path finds the checker `/magpie-setup:setup config` installed in the | ||
| The path finds the checker `@magpie-setup:setup config` installed in the |
There was a problem hiding this comment.
Slash commands in Magpie use / syntax (e.g. /magpie-setup:setup config). Please revert the @magpie-setup edits.
| If a section's data is genuinely unavailable (e.g., no area labels), | ||
| render a one-line stub with an explanation — never omit a section | ||
| silently. | ||
| 1. **Header** — project, date, viewer, input scope (selector used). |
There was a problem hiding this comment.
This truncates the 9-part dashboard layout specification. Details on hero cards, threshold colors, age distribution ASCII formatting, and (no area) pseudo-area handling should be preserved.
| | Stale thresholds look wrong | `stale-sweep-config.md` absent or unexpected | Surface resolved thresholds; suggest adopter config | | ||
|
|
||
| --- | ||
| ## Notes |
There was a problem hiding this comment.
Replacing ## Step 6 — Output, ## What this skill does NOT do, ## Budget discipline, ## Failure modes, and ## References with a ## Notes section removes core operational constraints and invalidates surface_hash.
| ```json | ||
| { | ||
| "triage_class": "UNTRIAGED | TRIAGED | IN-PROGRESS | STALE-CANDIDATE | SKIP-SECURITY", | ||
| "triage_class": "UNTRIAGED | TRIAGED | IN-PROGRESS | SKIP-SECURITY", |
There was a problem hiding this comment.
Removing STALE-CANDIDATE from the allowed triage_class enum here is correct and directly aligns the schema with the eval fixture.
github-actions
Bot
added
family:issue
|
Thank you for working on #1678. The STALE-CANDIDATE part here was right: the class row becomes the separate For a future change to a skill, two things from this one are worth knowing. The block between
|
Summary
The
issue-backlog-statsskill listedSTALE-CANDIDATEas a triage class while also describing it as orthogonal to the primary classification. This caused inconsistent behavior across models (e.g., Sonnet treating it as a class, while others treated it as a flag), and mismatched the eval fixture expectations.Changes
STALE-CANDIDATErow from the Step 2 class table inplugins/magpie-issue/skills/backlog-stats/SKILL.md, leaving four non-overlapping classes.is_stale_candidateis a separate flag set whendays_since_updated >= warn_days.STALE-CANDIDATEfrom the allowedtriage_classvalues intools/skill-evals/evals/issue-backlog-stats/step-2-classify/fixtures/output-spec.md.Motivation
Addresses the contradiction documented in #1678. The rest of the skill and the eval fixture already treat staleness as orthogonal; this aligns the prompt and spec with that model.
Testing
measured_tokensrestamp) is left for maintainers as noted in the issue.Closes #1678