Add inline skill extraction/runtime support mirroring inline sub-agents#34874
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot model is not a skill attribute. See skills reference:
Specification
Directory structureA skill is a directory containing, at minimum, a
|
| Field | Required | Constraints |
|---|---|---|
name |
Yes | Max 64 characters. Lowercase letters, numbers, and hyphens only. Must not start or end with a hyphen. |
description |
Yes | Max 1024 characters. Non-empty. Describes what the skill does and when to use it. |
license |
No | License name or reference to a bundled license file. |
compatibility |
No | Max 500 characters. Indicates environment requirements (intended product, system packages, network access, etc.). |
metadata |
No | Arbitrary key-value mapping for additional metadata. |
allowed-tools |
No | Space-separated string of pre-approved tools the skill may use. (Experimental) |
---
name: skill-name
description: A description of what this skill does and when to use it.
---Example with optional fields:
---
name: pdf-processing
description: Extract PDF text, fill forms, merge files. Use when handling PDFs.
license: Apache-2.0
metadata:
author: example-org
version: "1.0"
---name field
The required name field:
- Must be 1-64 characters
- May only contain unicode lowercase alphanumeric characters (
a-z,0-9) and hyphens (-) - Must not start or end with a hyphen (
-) - Must not contain consecutive hyphens (
--) - Must match the parent directory name
name: pdf-processingname: data-analysisname: code-reviewInvalid examples:
name: PDF-Processing # uppercase not allowedname: -pdf # cannot start with hyphenname: pdf--processing # consecutive hyphens not alloweddescription field
The required description field:
- Must be 1-1024 characters
- Should describe both what the skill does and when to use it
- Should include specific keywords that help agents identify relevant tasks
description: Extracts text and tables from PDF files, fills PDF forms, and merges multiple PDFs. Use when working with PDF documents or when the user mentions PDFs, forms, or document extraction.Poor example:
description: Helps with PDFs.license field
The optional license field:
- Specifies the license applied to the skill
- We recommend keeping it short (either the name of a license or the name of a bundled license file)
license: Proprietary. LICENSE.txt has complete termscompatibility field
The optional compatibility field:
- Must be 1-500 characters if provided
- Should only be included if your skill has specific environment requirements
- Can indicate intended product, required system packages, network access needs, etc.
compatibility: Designed for Claude Code (or similar products)compatibility: Requires git, docker, jq, and access to the internetcompatibility: Requires Python 3.14+ and uvmetadata field
The optional metadata field:
- A map from string keys to string values
- Clients can use this to store additional properties not defined by the Agent Skills spec
- We recommend making your key names reasonably unique to avoid accidental conflicts
metadata:
author: example-org
version: "1.0"allowed-tools field
The optional allowed-tools field:
- A space-separated string of tools that are pre-approved to run
- Experimental. Support for this field may vary between agent implementations
allowed-tools: Bash(git:*) Bash(jq:*) ReadBody content
The Markdown body after the frontmatter contains the skill instructions. There are no format restrictions. Write whatever helps agents perform the task effectively.
Recommended sections:
- Step-by-step instructions
- Examples of inputs and outputs
- Common edge cases
Note that the agent will load this entire file once it's decided to activate a skill. Consider splitting longer SKILL.md content into referenced files.
Optional directories
scripts/
Contains executable code that agents can run. Scripts should:
- Be self-contained or clearly document dependencies
- Include helpful error messages
- Handle edge cases gracefully
Supported languages depend on the agent implementation. Common options include Python, Bash, and JavaScript.
references/
Contains additional documentation that agents can read when needed:
REFERENCE.md- Detailed technical referenceFORMS.md- Form templates or structured data formats- Domain-specific files (
finance.md,legal.md, etc.)
Keep individual reference files focused. Agents load these on demand, so smaller files mean less use of context.
assets/
Contains static resources:
- Templates (document templates, configuration templates)
- Images (diagrams, examples)
- Data files (lookup tables, schemas)
Progressive disclosure
Agents load skills progressively, pulling in more detail only as a task calls for it. Skills should be structured to take advantage of this:
- Metadata (~100 tokens): The
nameanddescriptionfields are loaded at startup for all skills - Instructions (< 5000 tokens recommended): The full
SKILL.mdbody is loaded when the skill is activated - Resources (as needed): Files (e.g. those in
scripts/,references/, orassets/) are loaded only when required
Keep your main SKILL.md under 500 lines. Move detailed reference material to separate files.
File references
When referencing other files in your skill, use relative paths from the skill root:
See [the reference guide](references/REFERENCE.md) for details.
Run the extraction script:
scripts/extract.pyKeep file references one level deep from SKILL.md. Avoid deeply nested reference chains.
Validation
Use the skills-ref reference library to validate your skills:
skills-ref validate ./my-skillThis checks that your SKILL.md frontmatter is valid and follows all naming conventions.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This reverts commit f8fcd3a. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR adds first-class inline skill blocks to workflow markdown (## skill: \name``), extracts them at runtime into engine-specific skill directories, removes those blocks from the main prompt content, and wires the resulting skill files through the activation artifact/restore flow (mirroring inline sub-agent handling).
Changes:
- Add Go parser support to detect/strip inline skills and validate inline-skill frontmatter fields.
- Add runtime JS extraction/writing for inline skills and a restore script to materialize them into the workspace from the activation artifact.
- Update compiler orchestration and wasm golden fixtures to include the new skill staging + restore behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Golden fixture updated to upload skill staging dir and restore inline skills in main job. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Golden fixture updated to include .github/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Golden fixture updated to include .github/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Golden fixture updated to include .github/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Golden fixture updated to include .github/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Golden fixture updated to include .gemini/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Golden fixture updated to include .github/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Golden fixture updated to include .codex/skills in artifact + restore step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Golden fixture updated to include .claude/skills in artifact + restore step. |
| pkg/workflow/runtime_import_validation.go | Surfaces inline-skill frontmatter warnings when validating runtime-imported markdown. |
| pkg/workflow/inline_skill_step.go | Generates the “restore inline skills” step with engine-specific skill dir env. |
| pkg/workflow/compiler_yaml_main_job.go | Injects inline-skill restore step into the main job flow. |
| pkg/workflow/compiler_orchestrator_tools.go | Strips inline skill sections from effective markdown during compile-time processing. |
| pkg/workflow/compiler_activation_job_builder.go | Uploads engine-specific inline skill staging dirs as part of activation artifact. |
| pkg/parser/inline_skill_validation_test.go | Adds Go unit tests for inline-skill frontmatter validation. |
| pkg/parser/inline_skill_extractor.go | Adds Go inline-skill extraction, name de-dupe, H2 boundary handling, and validation helpers. |
| pkg/parser/inline_skill_extractor_test.go | Adds Go unit tests for inline-skill extraction behavior. |
| actions/setup/sh/restore_inline_skills.sh | Adds restore script to copy staged inline skill files into the workspace. |
| actions/setup/js/interpolate_prompt.cjs | Wires inline-skill extraction into runtime interpolation pipeline after runtime imports. |
| actions/setup/js/extract_inline_skills.cjs | Adds JS extraction/writing for inline skills with engine-specific output paths. |
| actions/setup/js/extract_inline_skills.test.cjs | Adds Vitest coverage for JS inline-skill extraction/write/frontmatter filtering. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/extract_inline_skills.cjs:96
- filterInlineSkillFrontmatter’s key regex does not match YAML keys containing hyphens (e.g. "bad-field:"), so those unsupported fields can slip through unchanged whenever at least one supported key has already been kept. Either broaden the key regex to include hyphens (and strip them) or switch to a YAML parser so unsupported top-level keys are reliably removed.
for (const line of fmLines) {
// Match a simple scalar YAML key at the start of the line.
// YAML keys are plain identifiers (no hyphens).
const keyMatch = line.match(/^([a-z_][a-z0-9_]*)[ \t]*:/);
if (keyMatch) {
const key = keyMatch[1];
if (SUPPORTED_FRONTMATTER_FIELDS.includes(key)) {
kept.push(line);
} else {
stripped.push(key);
}
} else {
// Continuation / comment / blank line — keep only when at least one
// supported key has already been accepted, so multi-line values (e.g.
// `description: |`) are preserved correctly.
if (kept.length > 0) {
kept.push(line);
}
}
- Files reviewed: 21/21 changed files
- Comments generated: 4
| var validInlineSkillFrontmatterFields = map[string]bool{ | ||
| "description": true, | ||
| } |
| // Supported frontmatter fields (all others are stripped with a warning) | ||
| // ───────────────────────────────────────────────────────────────────── | ||
| // description Human-readable description of the skill's role. | ||
| // | ||
| // If no ## skill: markers are present the content is returned unchanged and no | ||
| // files are written. | ||
|
|
||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
|
|
||
| // Supported frontmatter fields for inline skills. | ||
| // Any other field is stripped with a warning. | ||
| const SUPPORTED_FRONTMATTER_FIELDS = ["description"]; | ||
|
|
| * returned unchanged. | ||
| * | ||
| * Skill files are written relative to `skillsBaseDir` (defaults to `workspaceDir`). | ||
| * Pass the gh-aw tmp directory (`/tmp/gh-aw`) as `agentsBaseDir` in production so |
| // Best-effort: detect and validate inline sub-agent frontmatter in the | ||
| // runtime-imported file. Unknown fields are collected and returned to the | ||
| // caller so it can emit them through the normal warning counter. | ||
| for _, w := range parser.ValidateInlineSubAgentsFrontmatter(string(content)) { | ||
| subAgentWarnings = append(subAgentWarnings, fmt.Sprintf("runtime-import %q: %s", filePath, w)) | ||
| } | ||
| for _, w := range parser.ValidateInlineSkillsFrontmatter(string(content)) { | ||
| subAgentWarnings = append(subAgentWarnings, fmt.Sprintf("runtime-import %q: %s", filePath, w)) | ||
| } |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (+739 lines in default business directories) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once the ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Inline skills are a deliberate extension of the inline sub-agent pattern — that design choice (and the engine-specific output-path routing, frontmatter narrowing, and two-stage artifact handoff) deserves a captured decision so future contributors understand why the parser surface grew this way rather than via the alternatives. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
|
@copilot extend https://github.com/github/gh-aw/blob/main/.github/aw/skills.md with inline skills information . The strategy is to fusion skill or partial skills to keep the main prompt slim and flexible |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 97/100 — Excellent
📊 Metrics & Test Classification (50 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
Highlights:
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
✅ Test Quality Sentinel: 97/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 50 tests enforce observable behavioral contracts with strong edge-case coverage.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2.2M
| if (kept.length > 0) { | ||
| kept.push(line); | ||
| } | ||
| } |
There was a problem hiding this comment.
Continuation lines of stripped YAML keys are incorrectly kept in output, corrupting skill frontmatter.
💡 Details and fix
The else branch checks kept.length > 0 — meaning "has any supported key been seen". This is wrong: it gates on the existence of any prior kept key, not on whether the current key was kept. When a supported key precedes a stripped key with a multi-line value, the indented continuation lines of the stripped key are promoted into the output.
Example frontmatter:
---
description: A helper
tools:
github:
toolsets: [issues]
---After filtering, tools: is stripped (key regex matches), but github: and toolsets: [issues] fall into the else branch. Because kept.length > 0 (description was added), they are kept, producing malformed frontmatter:
---
description: A helper
github:
toolsets: [issues]
---Fix — track whether the last-seen key was kept:
let lastKeyKept = false;
for (const line of fmLines) {
const keyMatch = line.match(/^([a-z_][a-z0-9_]*)[ \t]*:/);
if (keyMatch) {
const key = keyMatch[1];
lastKeyKept = SUPPORTED_FRONTMATTER_FIELDS.includes(key);
if (lastKeyKept) kept.push(line);
else stripped.push(key);
} else {
if (lastKeyKept) kept.push(line);
}
}The existing test at line ~196 checks not.toContain("tools:") but does not assert not.toContain("github:") or not.toContain("toolsets:"), so the bug passes undetected. A test covering nested YAML under a stripped key is needed.
| // Supported frontmatter fields for inline skills. | ||
| // Any other field is stripped with a warning. | ||
| const SUPPORTED_FRONTMATTER_FIELDS = ["description"]; | ||
|
|
There was a problem hiding this comment.
model is stripped from every skill file despite being featured in the PR's own usage example, silently breaking model selection.
💡 Details
The PR description shows:
## skill: `repo-triage`
---
description: Classify incoming issues
model: small
---
Read the issue and return one label suggestion with rationale.But SUPPORTED_FRONTMATTER_FIELDS = ["description"] does not include model. Any skill authored with model: small (or any other model specifier) will have that field silently stripped with a warning, and the resulting SKILL.md will not carry the intended model hint.
The same omission exists in the Go implementation (pkg/parser/inline_skill_extractor.go):
var validInlineSkillFrontmatterFields = map[string]bool{
"description": true,
}Fix: add "model" to SUPPORTED_FRONTMATTER_FIELDS in JS and to validInlineSkillFrontmatterFields in Go, or remove the model example from all documentation if it is intentionally unsupported.
| /// <reference types="@actions/github-script" /> | ||
|
|
||
| import { describe, it, expect, beforeEach, afterEach } from "vitest"; | ||
| const fs = require("fs"); |
There was a problem hiding this comment.
ESM import in a .cjs file is a syntax error in Node.js — if Vitest is not configured to transpile .cjs test files, the entire test suite will fail to load.
💡 Details
Node.js treats .cjs files as CommonJS unconditionally, regardless of package.json "type". A bare import statement at the top level of a CJS module is a hard parse error:
SyntaxError: Cannot use import statement in a CommonJS module
The rest of the file uses require() idioms (const fs = require("fs"), etc.), confirming CJS intent.
Fix option A — stay CJS, use CommonJS require:
const { describe, it, expect, beforeEach, afterEach } = require("vitest");Fix option B — go fully ESM: rename to .test.mjs and replace all require() calls with import.
If the project's Vitest config already transforms .cjs test files to ESM, this is still a maintenance hazard because the file's extension advertises CommonJS while behaving as ESM — add a comment explaining the setup, and verify CI actually runs these tests.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 2.4M
| const START_MARKER_RE = /^##[ \t]+skill:[ \t]+`([a-z][a-z0-9_-]*)`[ \t]*$/gm; | ||
|
|
||
| // Regex that matches the start of any level-2 Markdown heading (## ). | ||
| // Used to find the boundary where each skill block ends. |
There was a problem hiding this comment.
[/grill-with-docs] model is listed as a supported frontmatter field in the PR description but is absent from SUPPORTED_FRONTMATTER_FIELDS. If model is intentionally unsupported it should be removed from the PR description; if it is intended to be supported, add it here and to the Go counterpart (validInlineSkillFrontmatterFields).
💡 Detail
The PR description states:
Supported frontmatter fields (all others are stripped with a warning):
description,model
But the implementation only allows description:
const SUPPORTED_FRONTMATTER_FIELDS = ["description"];The JS test at line 516 confirms model: gpt-4o is stripped rather than kept, so the current behaviour contradicts the stated design. Decide which is correct and align them.
| "skill %q: unknown frontmatter field(s): %s (valid fields: description)", | ||
| skill.Name, strings.Join(unknown, ", "), | ||
| )} | ||
| } |
There was a problem hiding this comment.
[/tdd] GetEngineSkillDir and GetEngineSkillExt are the core engine-routing functions but have no unit tests. A wrong mapping here silently writes skill files to the wrong path for every workflow using that engine.
💡 Suggested test skeleton
func TestGetEngineSkillDir(t *testing.T) {
tests := []struct{ engine, want string }{
{"claude", ".claude/skills"},
{"codex", ".codex/skills"},
{"gemini", ".gemini/skills"},
{"copilot", ".github/skills"},
{"", ".github/skills"}, // default
{"CLAUDE", ".claude/skills"}, // case-insensitive
}
for _, tt := range tests {
t.Run(tt.engine, func(t *testing.T) {
assert.Equal(t, tt.want, GetEngineSkillDir(tt.engine))
})
}
}Similar table for GetEngineSkillExt.
|
|
||
| if [ -d "$SRC" ]; then | ||
| echo "[restore-inline-skills] restoring skills from $SRC to $DST" | ||
| mkdir -p "$DST" |
There was a problem hiding this comment.
[/diagnose] cp -R "$SRC/"* "$DST/" uses shell glob expansion. When the source directory exists but is empty the glob expands to the literal string $SRC/*, causing cp to fail with "No such file or directory" — the error is swallowed by 2>/dev/null so skills are silently not restored.
💡 Suggested fix
Use rsync or a find-based copy, or guard with an explicit file count:
if [ -d "$SRC" ] && [ -n "$(ls -A "$SRC" 2>/dev/null)" ]; then
cp -R "$SRC/". "$DST/"
else
echo "[restore-inline-skills] no files to copy"
ficp -R "$SRC/". "$DST/" (trailing dot) copies directory contents without glob expansion and works correctly on empty and non-empty directories alike.
| core.info("[main] STEP 1.6: Inline Skill Extraction"); | ||
| core.info("========================================"); | ||
| const hasSkillMarkers = /^##[ \t]+skill:[ \t]+`[a-z]/m.test(content); | ||
| if (hasSkillMarkers) { |
There was a problem hiding this comment.
[/zoom-out] The quick pre-check regex /^##[ \t]+skill:[ \t]+[a-z]/mdoesn't anchor to end-of-line or verify the closing backtick, so it can match lines that the real extraction regex rejects (e.g.## skill: abc def`` or ## skill: abc extra text`). This causes the step to log "Inline skills extracted and written" even when no skills were actually extracted, making the log misleading.
💡 Suggestion
Use the same pattern as the extractor (or import the constant):
const HAS_SKILL_MARKER_RE = /^##[ \t]+skill:[ \t]+`[a-z][a-z0-9_-]*`[ \t]*$/m;
const hasSkillMarkers = HAS_SKILL_MARKER_RE.test(content);This keeps the fast-path heuristic but avoids false positives.
| // caller so it can emit them through the normal warning counter. | ||
| for _, w := range parser.ValidateInlineSubAgentsFrontmatter(string(content)) { | ||
| subAgentWarnings = append(subAgentWarnings, fmt.Sprintf("runtime-import %q: %s", filePath, w)) | ||
| } |
There was a problem hiding this comment.
[/grill-with-docs] Skill warnings are appended to the existing subAgentWarnings slice. The variable name is misleading when the warnings are about inline skills, not sub-agents. Consider introducing a unified slice (e.g. inlineBlockWarnings) or appending to a separate skillWarnings slice so callers can distinguish the source if needed.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all comments and pull request review comments |
This reverts commit afdd64d. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot apply skill inlining to a few large agentic workflows.non-essential parts of the prompt can be moved as skills. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in d55244e. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed the comment threads and PR review feedback; actionable follow-up updates are in 6b7a6ab.
Done in 6b7a6ab. I applied inline skill sections to a few larger workflows ( |
|
@copilot add more parser tests |
This change adds first-class inline skills to workflow markdown, using the same parsing/extraction model as inline sub-agents with
skillas the delimiter keyword. Inline skills can now be authored inline (full or partial blocks) and materialized into engine-specific skill locations at runtime.Parser and syntax support
## skill: \name`markers, duplicate-name checks, H2 boundary handling, and frontmatter field validation (description,model`)..*/skills/<name>.md.github/skills/<name>/SKILL.mdRuntime extraction and file emission
extract_inline_skills.cjs) and wired it into prompt interpolation after runtime imports./tmp/gh-aw/<engine-skill-dir>/...for artifact handoff.Workflow artifact and restore flow
restore_inline_skills.sh), parallel to sub-agent restore flow.Compiler orchestration and validation integration
Coverage updates