feat: Support agent skills in agent mode and add preferences settings#133
feat: Support agent skills in agent mode and add preferences settings#133
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…pilot-for-eclipse into ethan/support-skill
| /** | ||
| * Returns a priority for how well the template matches the prefix (lower is better), | ||
| * or -1 if it does not match at all. | ||
| * | ||
| * <p>Priority buckets: | ||
| * 0 – id starts with prefix (or prefix is empty) | ||
| * 1 – id contains prefix (or skill shortDescription contains prefix) | ||
| * 2 – description starts with prefix | ||
| * 3 – description contains prefix | ||
| */ | ||
| private int getMatchPriority(ConversationTemplate template, String lowerPrefix) { | ||
| if (lowerPrefix.isEmpty()) { | ||
| return 0; | ||
| } | ||
| ConversationTemplate[] templates = commandService.getTemplates(); | ||
| for (ConversationTemplate template : templates) { | ||
| if (prefix.isEmpty() || template.getId().startsWith(prefix)) { | ||
| proposals.add(new ChatCompletionProposal(ChatCompletionService.TEMPLATE_MARK, template.getId(), | ||
| template.getDescription())); | ||
| } | ||
| boolean isSkill = template.source() == TemplateSource.SKILL; | ||
| String id = template.id() != null ? template.id().toLowerCase() : ""; | ||
| String desc = template.description() != null ? template.description().toLowerCase() : ""; | ||
| String shortDesc = template.shortDescription() != null ? template.shortDescription().toLowerCase() : ""; | ||
|
|
||
| if (id.startsWith(lowerPrefix)) { | ||
| return 0; | ||
| } else if (id.contains(lowerPrefix) || (isSkill && shortDesc.contains(lowerPrefix))) { | ||
| return 1; | ||
| } else if (desc.startsWith(lowerPrefix)) { | ||
| return 2; | ||
| } else if (desc.contains(lowerPrefix)) { | ||
| return 3; | ||
| } | ||
| return proposals.toArray(new ICompletionProposal[proposals.size()]); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
The priority aligns with Jetbrains
| @@ -359,6 +426,73 @@ private void assertExists(ProbeStep step) { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Please check if this change influence the existing testing.
There was a problem hiding this comment.
Pull request overview
Adds Skills support to Copilot Chat’s Agent experience by wiring a new “Enable Skills” preference through to CLS settings, extending the conversation templates LSP request to include workspace folders (so CLS can discover SKILL.md / .prompt.md), and updating UI/test infrastructure to validate skills rendering via SWTBot probes.
Changes:
- Add “Enable Skills” preference (UI strings, preference page UI, defaults) and propagate it to language server settings.
- Extend
conversation/templatesrequest to include workspace folders; enrich template model withsource(BUILTIN/PROMPT/SKILL) and update completion filtering/sorting. - Add SWTBot probe actions + a new probe script to validate skills appear/disappear based on the preference.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/messages.properties | Adds UI strings for Skills toggle; adjusts sub-agent label. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/Messages.java | Adds NLS keys for Skills toggle. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManager.java | Syncs enableSkills to CLS settings, including live updates. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/CopilotPreferenceInitializer.java | Sets default values for sub-agent + skills toggles. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/ChatPreferencesPage.java | Adds “Enable Skills” field editor; refactors layout helpers. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatCompletionService.java | Refreshes templates using workspace folders; listens for skill/prompt file changes + pref changes. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatAssistProcessor.java | Filters templates by mode scope and improves matching/sorting; displays skills using short description. |
| com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManagerTests.java | Updates mocks for the new preference key. |
| com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/McpExtensionPointManagerTest.java | Fixes test setup to avoid NPE; improves null-map assertion. |
| com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatCompletionServiceTest.java | Updates tests for new templates API + job family; adds static mocking of CopilotUi. |
| com.microsoft.copilot.eclipse.swtbot.test/src/com/microsoft/copilot/eclipse/swtbot/test/probe/StepExecutor.java | Adds new probe actions (preferences, workspace file ops, job waiting, content-assist assertions, key typing). |
| com.microsoft.copilot.eclipse.swtbot.test/src/com/microsoft/copilot/eclipse/swtbot/test/probe/ProbeStep.java | Adds fields needed for the new probe actions. |
| com.microsoft.copilot.eclipse.swtbot.test/src/com/microsoft/copilot/eclipse/swtbot/test/probe/Locator.java | Documents new checkBox locator type. |
| com.microsoft.copilot.eclipse.swtbot.test/probe-scripts/skills-file-with-pref-001.json | New probe validating Skills enabled/disabled behavior. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TemplateSource.java | New enum identifying template source type. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/CopilotAgentSettings.java | Adds enableSkills setting. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/ConversationTemplatesParams.java | New params record for conversation/templates request. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/ConversationTemplate.java | Converts template model to a record; adds source. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnection.java | Updates listConversationTemplates to pass workspace folders. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServer.java | Updates LSP request signature for templates to accept params. |
| com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/Constants.java | Adds ENABLE_SKILLS preference key. |
| .github/skills/ui-action/SKILL.md | Documents new probe actions used by the skills probe script. |
Comments suppressed due to low confidence (2)
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatCompletionService.java:120
- InterruptedException is caught and only logged here. The thread interrupt flag should be restored (Thread.currentThread().interrupt()) so callers/framework code can react appropriately (e.g., job cancellation).
} catch (InterruptedException | ExecutionException e) {
CopilotCore.LOGGER.error(e);
}
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatCompletionService.java:234
- syncCommands() mutates the current HashSet/ArrayList instances via clear(). Since fetchAsync() updates these fields from a background Job, readers (e.g., isCommand() from the UI thread) can race with clear(), and HashSet/ArrayList are not thread-safe for concurrent reads+writes. Prefer swapping in new immutable/empty instances (e.g., Collections.emptySet()/emptyList()) to keep the snapshot swap pattern consistent and avoid concurrent mutation.
private void syncCommands(String status) {
switch (status) {
case CopilotStatusResult.OK:
fetchAsync();
break;
default:
allCommands.clear();
templates.clear();
agents.clear();
break;
| /** | ||
| * Extracts the underlying SWT {@link Widget} from an SWTBot wrapper via the | ||
| * public {@code widget} field. Uses reflection to avoid a compile-time | ||
| * dependency on {@code AbstractSWTBot} (which transitively pulls SLF4J). | ||
| */ | ||
| private static Widget extractWidget(Object botWrapper) { | ||
| ReflectiveOperationException reflectErr = null; | ||
| try { | ||
| java.lang.reflect.Field f = botWrapper.getClass().getField("widget"); | ||
| Object raw = f.get(botWrapper); | ||
| if (raw instanceof Widget) { | ||
| return (Widget) raw; | ||
| } | ||
| } catch (ReflectiveOperationException e) { | ||
| reflectErr = e; | ||
| } | ||
| if (botWrapper instanceof Widget) { | ||
| return (Widget) botWrapper; | ||
| } | ||
| throw new IllegalArgumentException( | ||
| "Cannot extract SWT Widget from " + botWrapper.getClass().getSimpleName(), | ||
| reflectErr); | ||
| } |
There was a problem hiding this comment.
extractWidget() relies on reflection against a public field named "widget" (Class#getField). SWTBot wrappers typically don't expose this as a public field, so this can throw and break pressKey/typeKeys. Consider using a supported API (e.g., SWTBotWidget#widget or a common interface), or at least use getDeclaredField() and walk superclasses with setAccessible(true) to make this robust.
| @AfterAll | ||
| static void tearDown() { | ||
| if (copilotUiMock != null) { | ||
| copilotUiMock.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
ChatCompletionService registers workspace and preference listeners in its constructor; this test never calls chatCompletionService.dispose(). That can leak listeners into other tests (or subsequent test classes) and cause flakiness. Call dispose() in @AfterAll (and null-check in case setup fails).
| /** | ||
| * Types text character-by-character via SWT event notification on the located | ||
| * widget. Unlike {@code typeIn} (which uses {@code setText}), this fires | ||
| * {@code KeyDown}/{@code KeyUp} events that trigger key listeners such as | ||
| * content-assist auto-activation. | ||
| */ | ||
| private void typeKeys(ProbeStep step) { | ||
| Object botWidget = resolve(step.locator); | ||
| String payload = required(step.text, "text"); | ||
| if (botWidget instanceof SWTBotStyledText) { | ||
| ((SWTBotStyledText) botWidget).setFocus(); | ||
| } else if (botWidget instanceof SWTBotText) { | ||
| ((SWTBotText) botWidget).setFocus(); | ||
| } | ||
| Widget target = extractWidget(botWidget); | ||
| for (char c : payload.toCharArray()) { | ||
| Display.getDefault().syncExec(() -> { | ||
| Event down = new Event(); | ||
| down.type = SWT.KeyDown; | ||
| down.character = c; | ||
| down.keyCode = c; | ||
| target.notifyListeners(SWT.KeyDown, down); | ||
|
|
||
| Event up = new Event(); | ||
| up.type = SWT.KeyUp; | ||
| up.character = c; | ||
| up.keyCode = c; | ||
| target.notifyListeners(SWT.KeyUp, up); | ||
| }); |
There was a problem hiding this comment.
typeKeys() uses target.notifyListeners(SWT.KeyDown/KeyUp, ...) to "type" characters, but notifyListeners only invokes registered listeners and does not perform the widget's default key handling/text insertion. This makes the step unreliable for entering text (and for interacting with content-assist selection via ENTER). Prefer SWTBot's keyboard APIs (or AWT Robot) to generate real key events, or fall back to setText()+explicit content-assist invocation if available.
| private void pressKey(ProbeStep step) { | ||
| String key = required(step.key, "key"); | ||
| org.eclipse.jface.bindings.keys.KeyStroke stroke = toKeystroke(key); | ||
| int[] keyInfo = resolveKey(key); | ||
| int keyCode = keyInfo[0]; | ||
| char character = (char) keyInfo[1]; | ||
| Display display = Display.getDefault(); | ||
|
|
||
| // Resolve target widget: either the locator target or the display's focus control. | ||
| AtomicReference<Widget> targetRef = new AtomicReference<>(); | ||
| if (step.locator != null) { | ||
| Object widget = resolve(step.locator); | ||
| if (widget instanceof SWTBotStyledText) { | ||
| ((SWTBotStyledText) widget).pressShortcut(stroke); | ||
| return; | ||
| } | ||
| if (widget instanceof SWTBotText) { | ||
| ((SWTBotText) widget).pressShortcut(stroke); | ||
| return; | ||
| Object botWidget = resolve(step.locator); | ||
| if (botWidget instanceof SWTBotStyledText) { | ||
| ((SWTBotStyledText) botWidget).setFocus(); | ||
| } else if (botWidget instanceof SWTBotText) { | ||
| ((SWTBotText) botWidget).setFocus(); | ||
| } | ||
| // Fall through to shell-level press if the widget wrapper lacks pressShortcut. | ||
| targetRef.set(extractWidget(botWidget)); | ||
| } | ||
| if (targetRef.get() == null) { | ||
| display.syncExec(() -> targetRef.set(display.getFocusControl())); | ||
| } | ||
| Widget target = targetRef.get(); | ||
| if (target == null) { | ||
| throw new RuntimeException("pressKey: no focused widget to receive key event"); | ||
| } | ||
|
|
||
| display.syncExec(() -> { | ||
| Event down = new Event(); | ||
| down.type = SWT.KeyDown; | ||
| down.keyCode = keyCode; | ||
| down.character = character; | ||
| target.notifyListeners(SWT.KeyDown, down); | ||
|
|
||
| Event up = new Event(); | ||
| up.type = SWT.KeyUp; | ||
| up.keyCode = keyCode; | ||
| up.character = character; | ||
| target.notifyListeners(SWT.KeyUp, up); | ||
| }); |
There was a problem hiding this comment.
pressKey() has the same issue as typeKeys(): notifyListeners(KeyDown/KeyUp) does not emulate real keyboard input, and may not trigger the default behavior of the focused control or the content-assist popup. Use SWTBot's keyboard/shortcut support (or Robot) to send actual key events to the UI.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
| */ | ||
| private class SkillFileChangeListener implements IResourceChangeListener { | ||
| @Override | ||
| public void resourceChanged(IResourceChangeEvent event) { |
There was a problem hiding this comment.
Is there any notification that we can use to update the available templates?
I mean, here although we can listen to the workspace's change, what about the global's?
| private List<ConversationTemplate> templates = new ArrayList<>(); | ||
| private List<ConversationAgent> agents = new ArrayList<>(); | ||
| private HashSet<String> allCommands = new HashSet<>(); | ||
| private volatile List<ConversationTemplate> templates = new ArrayList<>(); |
There was a problem hiding this comment.
nit:
volatile makes the reference swap visible across threads, but the published ArrayList/HashSet instances are still mutable. Since this code appears to use a snapshot/copy-on-write pattern, consider publishing immutable snapshots with List.copyOf(...) and Set.copyOf(...). That would prevent accidental mutation of a collection after it has been published to other threads, making readers see a stable old-or-new snapshot instead of risking concurrent modification or inconsistent completion state.
|
Looking forward to it, however README should probably updated as well for this feature. |
/, the skills should be rendered / hide corresponding to the preference settings.