Improve add-new-jit-ee-api skill: description triggers, references, stop signals#126051
Improve add-new-jit-ee-api skill: description triggers, references, stop signals#126051lewing wants to merge 1 commit intodotnet:mainfrom
Conversation
…top signals - Rewrite description with USE FOR / DO NOT USE FOR trigger pattern - Add "When to Use This Skill" section (5 trigger scenarios) - Add "Stopping Conditions" section (3 rules) - Extract SuperPMI templates to references/superpmi-integration.md - SKILL.md: 226 lines → 129 lines (43% reduction), no content lost Validated with 3-model eval (Claude Sonnet 4, GPT-5.1, Gemini 3 Pro): scores 4.4/5, 5/5, 5/5 across trigger match, completeness, clarity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refines the .github/skills/add-new-jit-ee-api skill documentation to improve trigger clarity (“USE FOR / DO NOT USE FOR”), add explicit stopping conditions, and move detailed SuperPMI guidance into an on-demand reference document.
Changes:
- Rewrites the skill’s front-matter description and adds “When to Use This Skill” + “Stopping Conditions” sections.
- Replaces the previous inlined SuperPMI step-by-step templates with a short pointer to a new reference doc.
- Adds a new
references/superpmi-integration.mdwith the extracted SuperPMI integration walkthrough and templates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/skills/add-new-jit-ee-api/SKILL.md | Updates triggers/stop signals and condenses SuperPMI steps to a reference pointer. |
.github/skills/add-new-jit-ee-api/references/superpmi-integration.md | New reference doc containing the detailed SuperPMI integration walkthrough and templates. |
Comments suppressed due to low confidence (1)
.github/skills/add-new-jit-ee-api/SKILL.md:129
- The “Definition of Done” checklist currently requires SuperPMI files unconditionally, but the skill also instructs to stop after step 5 when SuperPMI is out of scope. This makes the self-check fail for the step-5 stopping condition. Consider splitting the checklist into a “without SuperPMI” vs “with SuperPMI” section, or marking the SuperPMI items as conditional when the user opts out.
#### 4 — Definition of Done (self-check list) * [ ] New API present in **all** layers. * [ ] Each source file changed exactly once; no unrelated edits. The following files must be changed: * `<repo_root>/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt` * `<repo_root>/src/coreclr/inc/corinfo.h` * `<repo_root>/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs` * `<repo_root>/src/coreclr/vm/jitinterface.cpp` * `<repo_root>/src/coreclr/tools/superpmi/superpmi-shared/agnostic.h` [optional - only if new types are needed] * `<repo_root>/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h` * `<repo_root>/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h` * `<repo_root>/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp` * `<repo_root>/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp` * `<repo_root>/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp` * [ ] All TODO/UNREACHABLE markers remain for future functional implementation. | | ||
| - ✅ **Stop after step 5** if the user only needs the interface + VM stub (no SuperPMI). Ask before proceeding to step 6. | ||
| - ✅ **Stop after step 8** if implementations use `NotImplementedException()` / `UNREACHABLE()`. The scaffolding is complete. | ||
| - ❌ **Never skip a layer** — partial implementations break SuperPMI replay. All files in the checklist must be updated. |
There was a problem hiding this comment.
The “Stopping Conditions” list is internally inconsistent: it says to stop after step 5 when SuperPMI isn’t needed, but then says “Never skip a layer” and that “All files in the checklist must be updated.” Consider scoping the “never skip a layer” rule to the SuperPMI path (steps 6–8), or clarifying that skipping SuperPMI is allowed only when explicitly requested by the user.
| - ❌ **Never skip a layer** — partial implementations break SuperPMI replay. All files in the checklist must be updated. | |
| - ❌ **For steps 6–8 (SuperPMI path), never skip a layer** — partial SuperPMI implementations break replay. When enabling SuperPMI, all files in the checklist must be updated. |
| Do not create them if one of the generics ones can be re-used such as `DLD`, `DD`, `DLDL`, etc. Use `DWORD*` | ||
| like types for integers. Inspect the whole file to see how other APIs are defined. | ||
| 6. **SuperPMI integration** — the most complex part. SuperPMI records and replays JIT-VM queries for jit-diffs. | ||
| You need to update 4 shared files (`agnostic.h`, `lwmlist.h`, `methodcontext.h`, `methodcontext.cpp`) plus 2 shim files. |
There was a problem hiding this comment.
Step 6 says the user needs to update the 4 shared files “plus 2 shim files”, but the reference doc’s “Step 6” section is explicitly about the shared files only (with the shims covered in Steps 7 and 8). Consider rewording step 6 to avoid implying the shims are part of that step, or explicitly pointing out that steps 7–8 cover the shim updates.
| You need to update 4 shared files (`agnostic.h`, `lwmlist.h`, `methodcontext.h`, `methodcontext.cpp`) plus 2 shim files. | |
| In this step, you need to update 4 shared files (`agnostic.h`, `lwmlist.h`, `methodcontext.h`, `methodcontext.cpp`). The shim files are updated in the next two steps. |
| Add a new `LWM` entry after the last existing one. **Use upper-case first letter** for the API name: | ||
| | ||
| ```diff | ||
| +LWM(GetUnboxedEntry, DWORDLONG, DLD); |
There was a problem hiding this comment.
In the lwmlist.h example, the LWM(...) line includes a trailing semicolon. In src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h most LWM(...) entries omit the semicolon, and some include contexts (e.g., string-literal list macros) are cleaner without it. Consider dropping the semicolon in the template to match the common pattern and avoid encouraging inconsistent entries.
| +LWM(GetUnboxedEntry, DWORDLONG, DLD); | |
| +LWM(GetUnboxedEntry, DWORDLONG, DLD) |
| I'm just curious - were we had problems with this skill? it seemed to work fine. The new change then needs a test that it can properly add an API that works |
The goal of this pr is to reduce the context use when it isn't being used. The problem was mostly the frontmatter/routing, the agent didn't know when not to read the skill so it would likely load it too often, and when it did it would load everything including all the tokens describing superpmi etc. The changes here are to meant to make it less expensive for everyone who isn't working on the jit. |
I thought AI decides that from the SKILL header only and doesn't load the skill content when not needed? |
it reads the frontmatter to decide to load the skill or not which is what the extra use for/ do not use for changes are about. If it decides to load the skill it loads all of SKILL.md but based on what it reads it might still not use the skill or might not need everything in the skill, that is the reasoning behind moving the superpmi details to the references doc where it will only load those additional tokens when it needs to. Progressive disclosure is the idea behind minimizing context use. |
Summary
Improves the
add-new-jit-ee-apiCopilot skill based on structured training.Changes (2 files)
USE FOR/DO NOT USE FORtrigger patternValidation
3-model eval (Claude Sonnet 4, GPT-5.1, Gemini 3 Pro):
cc @BruceForstall @dotnet/jit-contrib