Add Skills migration recommendation and example implementations#6
Add Skills migration recommendation and example implementations#6Copilot wants to merge 8 commits intocopilot/duplicate-powershell-script-prfrom
Conversation
Co-authored-by: PureWeen <[email protected]>
Co-authored-by: PureWeen <[email protected]>
|
@copilot why would we get rid of all the agents and just use skills? |
You don't have to! Looking at the docs more closely, Why you might keep agents:
Why you might add skills:
My updated recommendation: Consider a hybrid approach - keep your working agents and selectively add skills only where you need the additional capabilities (like bundling the Appium test templates with sandbox-testing). The recommendation doc includes Option B (Hybrid) for this exact scenario. Want me to update the PR to remove the "remove agents" recommendation and emphasize the hybrid approach instead? |
|
@copilot so should I move all the ps1 scripts that I'm using into skills folders? like the ones for building the host app? testing? etc.. ? |
No, keep scripts in Current structure (recommended to keep): Best practice for sharing scripts across skills:
What TO put in skill folders:
What NOT to put in skill folders:
Your current organization is already correct. The scripts are shared infrastructure - skills just document when/how to use them. |
|
@copilot give me an example of a template i would put into a skill |
Good example: A UI test boilerplate template that only the Example namespace Microsoft.Maui.TestCases.Tests.Issues;
public class Issue{{ISSUE_NUMBER}} : _IssuesUITest
{
public override string Issue => "{{ISSUE_DESCRIPTION}}";
public Issue{{ISSUE_NUMBER}}(TestDevice device) : base(device) { }
[Test]
[Category(UITestCategories.{{CATEGORY}})]
public void {{TEST_METHOD_NAME}}()
{
App.WaitForElement("{{AUTOMATION_ID}}");
// TODO: Add test logic
}
}Why this belongs in the skill folder:
Compare to
|
|
Only sandbox should be using That's it Only uitest skill should be using templates for uitests |
Co-authored-by: PureWeen <[email protected]>
Done in 693a422. Added skill-specific templates: sandbox-testing: uitest-coding: Each skill now has its own templates bundled. The SKILL.md files reference these directly (e.g., |
|
Should any of the ps1 scripts move? The sandbox.ps1 is only relevant to sandbox The runhostapp is only relevant to uitests |
|
What is your thoughts on this analysis? Skills Architecture ReviewThanks for the initial migration! Here's my feedback: ✅ What's Good
|
| Skill | Phase | allowed-tools |
|---|---|---|
issue-analysis |
1 | Read, Grep, Glob, Bash |
issue-investigation |
2 | Read, Grep, Glob, Bash |
issue-implementation |
3 | (no restriction) |
📝 Recommended Changes
-
Split
issue-resolverinto 3 skills with distinct trigger descriptions:issue-analysis: "investigate issue", "reproduce bug", "fix issue #X"issue-investigation: "proceed with investigation", "find root cause"issue-implementation: "proceed with implementation", "create PR"
-
Add
allowed-toolsto Phase 1 and 2 to prevent premature editing -
Other agents (pr-reviewer, sandbox-testing, etc.) - single-file migration is fine since they don't have multi-phase requirements
You're right - applying the same logic as templates, skill-specific scripts should move: Move to skill folders:
Keep in
Result: Want me to make this change? |
|
Assuming an initial state with no previous work The workflow for fixing an issue should always be
|
Got it - simpler 2-phase model: Phase 1 (
Phase 2 (
The checkpoint enforcement comes from the skill boundary - user explicitly invokes Phase 2 only after Phase 1 confirms reproduction. I'll implement this now along with:
|
…ve scripts to skill folders Co-authored-by: PureWeen <[email protected]>
|
On the original pr from the dotnet/maui repository we had this agent that generalized the rules of writing tests Where does this figure in? This has the rules we want to follow when an agent is figuring out what kind of tests to write Unit tests vs uitests This is what the issue analyzer should take into account We also want the pr analyzer to use these rules to analyze a pr thats been submitted to evaluate the uitests |
Co-authored-by: PureWeen <[email protected]>
Done in 6037dec. Created Shared rules now used by both skills:
Key rules codified:
The original
|
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
What should we do with the uitesting skill?
Should we create a general skill for testing that includes info on uitests and unit tests?
Or do we need the separation?
Please do a deep analysis of all testing instructions and skills and figure out the best way to structure testing skills, agents, instructions
Here's my deep analysis: Current State - Testing AssetsInstructions (file-pattern based):
Skills:
Analysis: Do We Need Separation?Keep
No need for a separate
The
RecommendationKeep current separation, but:
Summary: Current structure is correct. Only change: add frontmatter to testing-strategy.instructions.md |
…to test directories Co-authored-by: PureWeen <[email protected]>
|
Alright can you review the specs here for skills Heres the spec for skills https://agentskills.io/home And then evaluate any changes we should make? Deeply analyze skills specs and how that might apply here |
After reviewing the Agent Skills spec at agentskills.io, here's my analysis: ✅ What We're Already Doing Correctly
|
|
@copilot please assess these suggested changed and let me know tour thoughts. Think really hard about if you agree or disagree ## Revised Architecture: Agent as Controller + Skills as Implementation
Based on research into how GitHub Copilot handles agents and skills, I want to restructure this PR to use a **lightweight agent with binary checkpoint** that routes to **detail-heavy skills**.
### Key Insight
Custom agents cannot programmatically invoke skills. However, the **language used in an agent's instructions** influences which skills Copilot loads via its semantic matching. We can exploit this by having the agent output specific task descriptions that trigger skill discovery.
### Architecture
issue-resolver.agent.md (small, always loaded) .github/skills/ Success Criteria
@copilot Please analyze this architecture before changing anything. Start with the agent file, then create both skills. Keep the agent minimal (checkpoint logic only) and put all detailed instructions in the skills. |
Here's my honest assessment: What I agree with:
My concerns:
My verdict: The architecture is conceptually superior to our current approach. Our current "two separate skills with explicit user invocation" is more predictable but less elegant. However, the implicit routing mechanism is the riskiest part. If Copilot's semantic matching is reliable, this is the right approach. If not, we'd need to fall back to explicit invocation. Suggested path forward:
Want me to proceed with implementing this architecture? |
|
@copilot sounds good let's do it |
Co-authored-by: PureWeen <[email protected]>
Implemented in afa5db6. The new architecture:
The agent outputs specific language ("create reproduction test" or "implement fix") that triggers skill discovery via semantic matching. The controller is always loaded, enforcing the checkpoint. |
…tnet#33071) App crashes because of changing a shared Drawable on the fly. I found out it is the SearchBar. It only happens on Android and in large apps with lots of screens. Unfortunately I can't reproduce it in the TestSuite. But by reading the docs of Android this crash is common and caused by changing a Drawable which is still referenced. With a custom SearchBarHandler the issues is resolved. ```Backtrace (top frames): #00 pc 00000000006b8694 /system/lib64/libhwui.so android::getRootAlpha(_JNIEnv*, _jobject*, long) +4 #1 pc 0000000002256c90 /memfd:jit-cache (deleted) art_jni_trampoline +112 #2 pc 000000000223bc4c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.-$$Nest$smnGetRootAlpha +108 #3 pc 000000000223bb20 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable$VectorDrawableState.getAlpha +144 #4 pc 00000000025c50e0 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getAlpha +128 #5 pc 00000000025c4f9c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getOpacity +124 #6 pc 00000000025c1ea8 /memfd:jit-cache (deleted) android.widget.ImageView.isOpaque +152 #7 pc 000000000227979c /memfd:jit-cache (deleted) android.view.View.invalidateInternal +428 #8 pc 00000000025c4790 /memfd:jit-cache (deleted) android.widget.ImageView.invalidateDrawable +256 #9 pc 000000000224419c /memfd:jit-cache (deleted) android.graphics.drawable.Drawable.invalidateSelf +156 #10 pc 000000000260e710 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.setTintList +192 #11 pc 00000000025d0094 /memfd:jit-cache (deleted) **android.graphics.drawable.Drawable.setTint +148** ``` ### Description of Change - Changes tinting of Androids SearchBar to unified setTint instead of setColorFilter - Mutates the drawable before setting the tint. ### Issues Fixed Issue is fixed with a custom handler for now. Fixes dotnet#33070
Review and Recommendation: Migrating Agents to Skills
.github/agents/.github/skills/Summary
The new architecture uses a lightweight agent as controller that enforces a binary checkpoint, with skills providing domain knowledge.
Architecture: Agent as Controller + Skills as Implementation
Why This Approach
Files Changed
Controller Agent:
.github/agents/issue-resolver.md- Lightweight controller with binary gateDomain Knowledge Skills:
.github/skills/issue-reproduction/SKILL.md- Test creation methodology.github/skills/issue-fix/SKILL.md- Fix implementation methodologyDocumentation:
Skills_Migration_Recommendation.md- Updated architecture docsFinal Structure
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.