Scope PR evaluation to only changed skills - #76
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the evaluation workflow to scope skill evaluations to only the skills that have changed in a pull request, rather than evaluating all skills in a component when any file in that component changes. This significantly reduces CI time and resource usage for PRs that modify individual skills.
Changes:
- Modified the discover job to track individual skill entries in addition to components, with PR events detecting changed skills at the skill level
- Updated the evaluate job to use a matrix based on skill entries rather than components, enabling parallel evaluation of individual changed skills
- Adjusted artifact naming, job naming, and summary reporting to reflect skill-specific evaluation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeffhandley
left a comment
jeffhandley
left a comment
There was a problem hiding this comment.
LGTM. General style feedback for another time
| } | ||
| # Extract unique component/skill pairs from changed files under src/*/skills/*/ or src/*/tests/*/ | ||
| # NOTE: Changes under src/*/skills/shared/ won't match here since shared directories | ||
| # don't have SKILL.md files. Use workflow_dispatch to manually evaluate affected |
| echo "components=$cjson" >> $env:GITHUB_OUTPUT | ||
| echo "has_components=true" >> $env:GITHUB_OUTPUT | ||
| } | ||
| shell: pwsh |
There was a problem hiding this comment.
NIT for another time / general style suggestion: I recommend putting shell: pwsh above run so readers more readily see the language before the code and need a moment to recognize which language it is.
|
@adityamandaleeka How do we validate related skills that contextually reference the changed skill or vice versa? That was the reason why I didn't make this change. I already had it locally but then abandoned the idea. Essentially, we are shipping all the skills per component as a plugin. Changing one skill impacts the success rate of another skill. We have seen countless examples of that in our playing group repo. |
Sorry, I missed this comment @ViktorHofer. You're right, running everything in the component is probably the better approach. |
No description provided.