Extend aw.yml to support includes, skills, and agents#35778
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e path helper for marker Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends repository package manifests (aw.yml) so package installs can include skills and agents in addition to workflow files, wiring discovery, resolution, and install handling through the existing add pipeline.
Changes:
- Adds
skillsandagentsmanifest fields and parsing/validation logic. - Adds remote directory listing helpers for skill/agent discovery.
- Routes resolved package skill and agent files to dedicated install handlers and adds related tests.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schemas/aw_manifest_schema.json |
Adds schema entries for skills and agents. |
pkg/parser/remote_fetch.go |
Adds remote directory file/subdirectory listing with API and git fallback paths. |
pkg/cli/spec.go |
Extends WorkflowSpec with package skill/agent metadata. |
pkg/cli/add_workflow_resolution.go |
Resolves package skill and agent specs alongside workflow specs. |
pkg/cli/add_package_manifest.go |
Parses manifest fields and resolves explicit or auto-discovered skill/agent files. |
pkg/cli/add_package_manifest_test.go |
Adds and updates tests for manifest parsing and package resolution. |
pkg/cli/add_command.go |
Installs package skill and agent files into engine-specific directories. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 4
| lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "--name-only", "HEAD", dirPath+"/") | ||
| lsTreeOutput, err := lsTreeCmd.CombinedOutput() | ||
| if err != nil { | ||
| remoteLog.Printf("Failed to list dir files: %s", string(lsTreeOutput)) | ||
| return nil, fmt.Errorf("failed to list dir files: %w", err) | ||
| } | ||
|
|
||
| lines := strings.Split(strings.TrimSpace(string(lsTreeOutput)), "\n") | ||
| var files []string | ||
| for _, line := range lines { | ||
| line = strings.TrimSpace(line) | ||
| if line == "" { | ||
| continue | ||
| } | ||
| // Only include direct children (no additional path separator after dirPath/) | ||
| afterDirPath := strings.TrimPrefix(line, dirPath+"/") | ||
| if !strings.Contains(afterDirPath, "/") && afterDirPath != "" { | ||
| files = append(files, line) | ||
| } |
| for _, file := range files { | ||
| skillFiles = append(skillFiles, resolvedPackageSkillFile{ | ||
| SourcePath: file, | ||
| SkillName: skillName, | ||
| }) | ||
| } |
| // addSkillFileWithTracking installs a single skill file from a package to the agentic engine | ||
| // skill directory. | ||
| func addSkillFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions, gitRoot string) error { | ||
| engineSkillDir := parser.GetEngineSkillDir("") |
| // addAgentFileWithTracking installs a single agent file from a package to the agentic engine | ||
| // agents directory. | ||
| func addAgentFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions, gitRoot string) error { | ||
| engineAgentsDir := parser.GetEngineSubAgentDir("") |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,099 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References: §26661810526
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — two new blocking issues in the git fallback path, plus four already-flagged issues from the prior review pass that need to be resolved before merge.
### Blocking issues (this pass)
refempty string →git clonecrash (remote_fetch.go:996,1098): ifrefis"", the git clone command emitsfatal: invalid branch name: "". Guard bothlistDirAllFilesViaGitForHostandlistDirSubdirsViaGitForHostbefore constructing the clone command.- N+1 git clones per package (
remote_fetch.go:981): eachlistDir*ViaGitcall clones the repo independently. A package with N skills in fallback mode incurs N+2 sequential clones. Extract a shared shallow-clone helper or switch the fallback to the GitHub Trees API.
### Blocking issues (prior pass, still open)
add_command.go:631—GetEngineSkillDir("")ignoresopts.EngineOverrideadd_command.go:680—GetEngineSubAgentDir("")ignoresopts.EngineOverrideremote_fetch.go:1021—ls-tree --name-onlyincludes directory entries; file list will contain subdirectory namesadd_package_manifest.go:418— explicitskills:entries bypass theSKILL.mdmarker check
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2M
| } | ||
| defer os.RemoveAll(tmpDir) | ||
|
|
||
| cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir) |
There was a problem hiding this comment.
ref empty string crashes the git clone with a cryptic fatal error — if ref is "", the command becomes git clone --depth 1 --branch --single-branch ... which git rejects with fatal: invalid branch name: "", surfacing as an opaque error rather than a clear validation message.
💡 Suggested fix
Guard before constructing the command:
if ref == "" {
return nil, fmt.Errorf("ref must be non-empty for git clone fallback")
}
cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, ...)The same guard is needed in listDirSubdirsViaGitForHost (line 1098). The REST API path silently treats an empty ref as the default branch, but the git path does not.
| return files, nil | ||
| } | ||
|
|
||
| func listDirAllFilesViaGitForHost(owner, repo, ref, dirPath, host string) ([]string, error) { |
There was a problem hiding this comment.
N+1 git clones per package resolution — severe network regression — each call to listDirAllFilesViaGitForHost and listDirSubdirsViaGitForHost performs an independent git clone --depth 1. For a package with N skill directories the fallback path incurs: 1 clone (subdir listing) + N clones (one per skill dir file listing) + 1 clone (agents dir). All sequential over the network.
💡 Suggested fix
The simplest fix: extract a cloneRepoShallow(owner, repo, ref, host) (string, func(), error) helper that clones once and returns the temp dir path plus a cleanup func. Pass the temp dir through to the ls-tree helpers so a single clone serves all subsequent listing calls for the same (owner, repo, ref) tuple.
Alternatively, use the GitHub Trees API with recursive=1, which returns the full tree in one HTTP request and eliminates the fallback entirely:
GET /repos/{owner}/{repo}/git/trees/{ref}?recursive=1
Note: the current approach also duplicates ~40 lines of clone boilerplate verbatim between listDirAllFilesViaGitForHost and listDirSubdirsViaGitForHost; fixing the N+1 issue with a shared helper also resolves the duplication.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /tdd, /diagnose, and /zoom-out. Solid feature addition overall — no blocking issues — but a few things worth addressing before merging.
📋 Key Themes & Findings
Key Themes
-
Code duplication —
addSkillFileWithTrackingandaddAgentFileWithTrackingshare ~50 lines of identical file-write logic. Extracting a shared helper now will pay off every time the install behaviour evolves. -
N+1 API calls in auto-scan —
scanPackageSkillDirsprobes each skill subdir with an individualdownloadPackageFileFromGitHubForHostcall to check forSKILL.md, thenresolvePackageSkillFilescallslistPackageDirFilesForHoston the same directories. The marker check can be folded into the file-list step, eliminating one round-trip per skill. -
Inconsistent SKILL.md validation — auto-scan enforces the
SKILL.mdmarker; explicit manifest entries do not. A directory listed inaw.ymlunderskills:without aSKILL.mdwill be installed silently. -
Missing tests for the installation layer —
addSkillFileWithTracking/addAgentFileWithTrackinghave no unit tests. The manifest-parsing and resolution layers are well-tested; the final file-write step is not. -
Silent non-
.mdfilter inresolvePackageAgentFiles— auto-scan drops non-.mdfiles without a warning, while the skills path installs all file types. The asymmetry is surprising and undocumented.
Positive Highlights
- ✅ Clean separation between resolution (manifest parsing, auto-scan) and installation (file-write)
- ✅ Good use of
path.Clean(filepath.ToSlash(...))to normalise path separators and prevent traversal in validators - ✅ Comprehensive table-driven tests for path validators and manifest extraction
- ✅ Consistent use of
joinRepositoryPackagePathto handle nested package paths - ✅ Git fallback implemented for both new
listDirAllFilesandlistDirSubdirsfunctions
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 3.5M
|
|
||
| // addAgentFileWithTracking installs a single agent file from a package to the agentic engine | ||
| // agents directory. | ||
| func addAgentFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions, gitRoot string) error { |
There was a problem hiding this comment.
[/improve-codebase-architecture] addAgentFileWithTracking duplicates ~50 lines from addSkillFileWithTracking. The only real differences are the destination directory, file-kind label, and success-message format — everything else (exist-check, force-overwrite, tracker, write) is identical.
💡 Suggested refactor
Extract a shared helper:
type artifactInstallOptions struct {
destDir string
kind string // "skill" or "agent"
label string // used in console messages
}
func addArtifactFileWithTracking(
resolved *ResolvedWorkflow,
tracker *FileTracker,
opts AddOptions,
destDir string,
kind string,
) error {
if err := os.MkdirAll(destDir, constants.DirPermPublic); err != nil {
return fmt.Errorf("failed to create %s directory %s: %w", kind, destDir, err)
}
// ... shared logic ...
}Then addSkillFileWithTracking and addAgentFileWithTracking become thin wrappers that build destDir and call this helper. This would shrink the diff by ~45 lines and make future changes (e.g. adding a --dry-run flag) apply to both artifact types automatically.
| } | ||
| } | ||
| return skillDirs, nil | ||
| } |
There was a problem hiding this comment.
[/improve-codebase-architecture] scanPackageSkillDirs makes N+1 API calls: one listPackageDirSubdirsForHost to enumerate subdirs, then one downloadPackageFileFromGitHubForHost per subdir to check for the SKILL.md marker. However resolvePackageSkillFiles immediately follows with a listPackageDirFilesForHost call for every qualifying dir anyway, so the marker content is redundantly fetched.
💡 Suggested fix
Remove scanPackageSkillDirs's per-subdir download and instead defer the SKILL.md check to resolvePackageSkillFiles where the file listing is already available:
// In resolvePackageSkillFiles, after listing files:
hasMarker := slices.ContainsFunc(files, func(f string) bool {
return filepath.Base(f) == packageSkillMarkerFile
})
if !hasMarker {
warnings = append(warnings, fmt.Sprintf("skill dir %q has no SKILL.md, skipping", skillDir))
continue
}This eliminates N extra HTTP round-trips (one per skill subdirectory in the auto-scan path).
| } | ||
| } else { | ||
| autoScanned, err := scanPackageSkillDirs(owner, repo, packagePath, ref, host) | ||
| if err != nil { |
There was a problem hiding this comment.
[/diagnose] Explicit skill dirs listed in the manifest skip the SKILL.md marker check that auto-scan enforces (see scanPackageSkillDirs). A user could list skills/not-a-skill in aw.yml and its files would be installed to the skill directory even without a SKILL.md, silently creating a broken skill.
💡 Suggested fix
Apply the same marker validation regardless of discovery path. Whether the dirs come from the manifest or auto-scan, after listing files check that SKILL.md is present:
hasMarker := false
for _, f := range files {
if filepath.Base(f) == packageSkillMarkerFile {
hasMarker = true
break
}
}
if !hasMarker {
warnings = append(warnings, fmt.Sprintf(
"skill directory %q has no SKILL.md — skipping (add SKILL.md to qualify it as a skill)",
skillDir,
))
continue
}| // addSkillFileWithTracking installs a single skill file from a package to the agentic engine | ||
| // skill directory. | ||
| func addSkillFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions, gitRoot string) error { | ||
| engineSkillDir := parser.GetEngineSkillDir("") |
There was a problem hiding this comment.
[/tdd] addSkillFileWithTracking (and its agent counterpart) have no unit tests. The resolution and manifest-parsing layers are well-covered, but the actual file-installation functions — directory creation, force-overwrite behaviour, tracker interaction, and quiet/verbose output — are not tested at all.
💡 Suggested test skeleton
func TestAddSkillFileWithTracking(t *testing.T) {
t.Run("creates new skill file", func(t *testing.T) { ... })
t.Run("skips existing file without --force", func(t *testing.T) { ... })
t.Run("overwrites existing file with --force", func(t *testing.T) { ... })
t.Run("tracks created vs modified files via tracker", func(t *testing.T) { ... })
}Consider using t.TempDir() to create an isolated git root so tests don't touch the real filesystem.
| } | ||
|
|
||
| // Auto-scan agents/ directory for .md files. | ||
| agentsDir := joinRepositoryPackagePath(packagePath, packageAgentsDirectory) |
There was a problem hiding this comment.
[/zoom-out] resolvePackageAgentFiles silently drops non-.md files during auto-scan (line ~440: strings.HasSuffix(strings.ToLower(f), ".md")), but returns no warning. A user who accidentally places a .sh helper alongside their agent .md might expect it to be installed too and would get no feedback when it is skipped.
💡 Suggestion
Either:
- Emit a warning for each skipped non-
.mdfile:"agents/%s was skipped (only .md files are installed from the agents/ directory)" - Or document this filter clearly in a comment so the omission is intentional and obvious.
The skills path already surfaces a similar concern — non-.md files in a skill dir are installed (e.g. prompt.sh), which makes this asymmetry surprising.
|
@copilot move all files under a "includes" folder and use folder naming conventions to determine the type of file. Same structure as .github folder:
also support direct reference
|
…odemod warning Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…dance Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in
|
| - workflows/review.md # agentic workflow — compiled on install | ||
| - .github/workflows/nightly-review.md | ||
| includes: | ||
| - includes/workflows/review.md # agentic workflow — compiled on install |
There was a problem hiding this comment.
@copilot avoid the "includes/" folder, just "workflows/" . includes is th ename of the frontmatter field not the folders.
There was a problem hiding this comment.
Updated in 4da41ab. Manifest include path conventions now use top-level workflows/, skills/, and agents/ (plus direct .github/workflows, .github/skills, .github/agents), and includes/* folder paths are no longer part of inferred types.
|
@copilot also support |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all review comments from copilot and github-actions actor |
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>
Reviewed all Copilot and github-actions review comments. The previously flagged install-path and SKILL.md checks are already in place, and I addressed the remaining git-fallback blockers in commits 523170f, dd23d88, and 07c02dd (empty-ref validation + shared clone reuse for fallback dir listing + follow-up polish/tests). |
|
@copilot add optional license and description fields |
Adds first-class support for
includesin theaw.ymlpackage manifest, with folder-based type inference for workflows, skills, and agents, while retaining support for explicitskillsandagentsfields.Manifest format
Legacy
filesis still supported and now emits a deprecation warning plus codemod guidance to migrate toincludes.Include path conventions
Type is inferred by folder naming:
workflows/,agentic-workflows/,.github/workflows/skills/,.github/skills/agents/,.github/agents/When explicit skill directories are provided (
skillsor inferred viaincludes),SKILL.mdis validated as required.Install targets
GetEngineSkillDir)GetEngineSubAgentDir)Engine override (
--engine) is now correctly honored for package-installed skills and agents.Changes
aw_manifest_schema.json— addedincludes, retainedfilesas deprecated, and documented manifest semantics (includingagentic-workflows/workflow support)remote_fetch.go— fixed git fallback forListDirAllFilesForHostto avoid treating directories as filesadd_package_manifest.go— addedincludesparsing, type inference, legacyfileswarning + codemod suggestion, expanded path validation (workflows/*,agentic-workflows/*,skills/*,agents/*, and.github/*), skill marker validation, and updated auto-scan logicadd_command.go— fixed package skill/agent install path resolution to honoropts.EngineOverrideadd_package_manifest_test.go— updated and expanded tests for includes, deprecation warnings, codemod behavior,agentic-workflows/workflow paths, direct.github/*references, and skill marker validationincludesand migration fromfiles