Copy skills from aw.yml manifest first; copy skill folders recursively and safely

docs/adr/35946-manifest-first-skill-ordering-and-recursive-copy.md

@@ -0,0 +1,81 @@
+# ADR-35946: Manifest-First Skill Ordering and Recursive Skill Folder Copy
+
+**Date**: 2026-05-30
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+[ADR-35778](35778-extend-aw-yml-for-skills-and-agents.md) added first-class skill distribution to the `aw.yml` package manifest, but the resolver used an *either/or* model: when `skills:` was present it used the explicit list, and otherwise it auto-scanned `skills/`. This left two gaps that surfaced as install bugs. First, ordering was non-deterministic relative to the manifest — a package could not guarantee that skills listed in `skills:` resolved before any extras discovered on disk. Second, the installer copied only the top-level files of each skill folder, so any nested subdirectory (e.g. `scripts/`, `prompts/`, `scripts/helpers/`) was silently dropped, breaking skills whose `SKILL.md` references helper scripts by relative path. Both gaps degrade a package author's ability to ship a complete, ready-to-run skill.
+
+### Decision
+
+We will resolve manifest-listed skills first and then **always** auto-scan `skills/`, appending only those skill folders not already covered by the manifest (deduplicated by skill name). We will also copy each skill folder **recursively**, preserving its full subdirectory structure on disk rather than flattening or dropping nested files. The relative path of each file within its skill directory is reconstructed using the `/<skill-name>/` path component as a boundary so that nesting is reproduced faithfully at the install destination.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep the either/or model and require explicit listing for ordering control
+
+We could preserve the original "explicit list OR auto-scan" behavior and tell authors to enumerate every skill in `skills:` when they care about order. Rejected because it reintroduces the manifest boilerplate ADR-35778 deliberately avoided and drifts out of sync as a package gains skills — a newly added skill folder would be invisible unless the author remembered to also list it. Manifest-first-then-append gives deterministic ordering *and* keeps zero-config discovery of extras.
+
+#### Alternative 2: Flatten nested skill files into the skill root
+
+Instead of preserving subdirectories, we could copy every nested file directly into `<skill-name>/`, discarding intermediate path segments. Rejected because skills reference their helpers by relative path (`scripts/query.sh`); flattening would break those references and risk filename collisions between files in different subdirectories.
+
+#### Alternative 3: Keep non-recursive copy and constrain skills to flat folders
+
+We could leave the copy logic untouched and declare that a skill folder must contain only top-level files. Rejected as too restrictive: real skills in this repository already use `scripts/` and similar subdirectories, so a flat-only rule would make them un-shippable via packages.
+
+### Consequences
+
+#### Positive
+- Skill ordering is deterministic: manifest skills always precede auto-scanned extras, so a package author controls precedence by listing.
+- Entire skill folders install intact, including nested `scripts/`, `prompts/`, and deeper helper directories, so skills that depend on relative-path assets work after `gh aw add`.
+- Auto-scan still runs even when `skills:` is present, so additional on-disk skills are no longer hidden by the presence of a manifest list; name-based deduplication prevents a manifest skill from being installed twice.
+
+#### Negative
+- Recursive enumeration via the GitHub Contents API issues one additional API call per subdirectory, increasing latency and rate-limit pressure for deeply nested skills (mitigated by a git-clone `ls-tree -r` fallback).
+- This relaxes the "single-level nesting / direct child only" contract asserted in ADR-35778's normative section; that ADR's nesting language is now partially superseded and should be reconciled.
+- Auto-scan now always executes, adding directory-listing and `SKILL.md` marker calls even for packages that fully specify `skills:`, where the previous code skipped scanning entirely.
+
+#### Neutral
+- A new injectable seam `listPackageDirFilesRecursivelyForHost` is introduced so tests can mock recursive listing independently of the existing non-recursive helper (agents still use the non-recursive path).
+- Relative-path reconstruction relies on locating the `/<skill-name>/` path component within the source path, falling back to `filepath.Base` when the boundary is absent.
+- The recursive walk is exposed publicly as `parser.ListDirAllFilesRecursivelyForHost`, paralleling the existing `ListDirAllFilesForHost` helper.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Skill Resolution Order
+
+1. The resolver **MUST** resolve manifest-listed (`skills:`) skill directories before any auto-scanned skill directories.
+2. The resolver **MUST** auto-scan the `skills/` directory even when the manifest provides an explicit `skills:` list, appending any discovered skill folder not already covered by the manifest.
+3. The resolver **MUST** deduplicate skills by skill name so that a skill present in both the manifest and the auto-scan is resolved exactly once, retaining its manifest-first position.
+4. A manifest-listed skill directory **MUST** be validated against its `SKILL.md` marker, and a missing marker **MUST** produce a warning rather than aborting resolution.
+
+### Recursive Folder Copy
+
+1. The installer **MUST** copy every file under a resolved skill folder at any nesting depth, not only the folder's top-level files.
+2. The installer **MUST** preserve each file's path relative to the skill directory when writing it to the destination, recreating intermediate subdirectories as needed.
+3. The installer **MUST NOT** flatten nested skill files into the skill root or otherwise discard their subdirectory structure.
+4. Relative-path reconstruction **MUST** locate the skill name as a complete `/<skill-name>/` path component so that a subdirectory whose name coincides with the skill name does not corrupt the computed relative path; when no such component is found, the installer **SHOULD** fall back to the file's base name.
+
+### Remote Listing
+
+1. Recursive remote listing **MUST** attempt the GitHub Contents API first and **MUST** fall back to a git clone with `ls-tree -r` on authentication or client-creation failure.
+2. The git fallback **MUST** return files at any depth under the target directory, not only direct children.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26687535424) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/add_command.go

@@ -626,21 +626,59 @@ func addActionWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTrac
 }
 
 // addSkillFileWithTracking installs a single skill file from a package to the agentic engine
-// skill directory.
+// skill directory. The file's path relative to the skill directory is preserved so that
+// nested files (e.g. scripts/ subdirectories) are written with their full structure intact.
 func addSkillFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions, gitRoot string) error {
 	engineSkillDir := parser.GetEngineSkillDir(opts.EngineOverride)
 	skillDir := filepath.Join(gitRoot, engineSkillDir, resolved.SkillName)
-	if err := os.MkdirAll(skillDir, constants.DirPermPublic); err != nil {
-		return fmt.Errorf("failed to create skill directory %s: %w", skillDir, err)
+
+	// Determine the relative path under the skill directory so nested files preserve
+	// structure (e.g. "scripts/query.sh"). Match a skill-name path component that is
+	// immediately under skills/ or .github/skills/ to avoid accidental first matches.
+	parts := strings.Split(filepath.ToSlash(resolved.Spec.WorkflowPath), "/")
+	var relParts []string
+	for i, part := range parts {
+		if i >= len(parts)-1 {
+			break
+		}
+		if part != resolved.SkillName {
+			continue
+		}
+		if i > 0 && parts[i-1] == "skills" {
+			relParts = parts[i+1:]
+			break
+		}
+		if i > 1 && parts[i-1] == "skills" && parts[i-2] == ".github" {
+			relParts = parts[i+1:]
+			break
+		}
+	}
+	if len(relParts) == 0 {
+		return fmt.Errorf("failed to determine relative path for skill %q from source path %q", resolved.SkillName, resolved.Spec.WorkflowPath)
+	}
+	relPath := filepath.Clean(filepath.Join(relParts...))
+	if relPath == "." || relPath == "" || relPath == string(os.PathSeparator) {
+		return fmt.Errorf("invalid relative skill path %q from source path %q", relPath, resolved.Spec.WorkflowPath)
 	}
 
-	fileName := filepath.Base(resolved.Spec.WorkflowPath)
-	destFile := filepath.Join(skillDir, fileName)
+	destFile := filepath.Join(skillDir, relPath)
+	relToSkillDir, err := filepath.Rel(skillDir, destFile)
+	if err != nil {
+		return fmt.Errorf("failed to validate destination path %q for skill %q: %w", destFile, resolved.SkillName, err)
+	}
+	if relToSkillDir == ".." || strings.HasPrefix(relToSkillDir, ".."+string(os.PathSeparator)) {
+		return fmt.Errorf("skill file path %q escapes destination skill directory %q", relPath, skillDir)
+	}
+
+	// Ensure the destination directory exists (handles nested subdirectories).
+	if err := os.MkdirAll(filepath.Dir(destFile), constants.DirPermPublic); err != nil {
+		return fmt.Errorf("failed to create skill directory %s: %w", filepath.Dir(destFile), err)
+	}
 
 	addLog.Printf("Adding skill file: dest=%s, skill=%s, content_size=%d bytes", destFile, resolved.SkillName, len(resolved.Content))
 
 	if opts.Verbose {
-		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Adding skill file to %s: %s", engineSkillDir+"/"+resolved.SkillName, fileName)))
+		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Adding skill file to %s: %s", engineSkillDir+"/"+resolved.SkillName, relPath)))
 	}
 
 	fileExists := false
@@ -668,7 +706,7 @@ func addSkillFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker,
 	}
 
 	if !opts.Quiet {
-		fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Added skill file: %s/%s/%s", engineSkillDir, resolved.SkillName, fileName)))
+		fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Added skill file: %s/%s/%s", engineSkillDir, resolved.SkillName, relPath)))
 	}
 
 	return nil

pkg/cli/add_command_test.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/github/gh-aw/pkg/parser"
 	"github.com/github/gh-aw/pkg/testutil"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
@@ -617,3 +618,61 @@ func TestAddWorkflowWithTracking_ActionWorkflow_Force(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, newContent, written)
 }
+
+func TestAddSkillFileWithTracking_PreservesPathFromSkillsRoot(t *testing.T) {
+	gitRoot := testutil.TempDir(t, "test-add-skill-path-*")
+	resolved := &ResolvedWorkflow{
+		Spec: &WorkflowSpec{
+			WorkflowPath: "vendor/foo/skills/foo/scripts/run.sh",
+		},
+		SkillName: "foo",
+		Content:   []byte("#!/usr/bin/env sh\necho ok\n"),
+	}
+
+	err := addSkillFileWithTracking(resolved, nil, AddOptions{Quiet: true}, gitRoot)
+	require.NoError(t, err)
+
+	skillRoot := filepath.Join(gitRoot, parser.GetEngineSkillDir(""), "foo")
+	expectedFile := filepath.Join(skillRoot, "scripts", "run.sh")
+	unexpectedFile := filepath.Join(skillRoot, "skills", "foo", "scripts", "run.sh")
+
+	_, err = os.Stat(expectedFile)
+	require.NoError(t, err, "expected nested skill file should exist")
+	content, err := os.ReadFile(expectedFile)
+	require.NoError(t, err, "expected nested skill file should be readable")
+	assert.Equal(t, []byte("#!/usr/bin/env sh\necho ok\n"), content, "expected nested skill file content should match")
+	_, err = os.Stat(unexpectedFile)
+	assert.True(t, os.IsNotExist(err), "unexpected first-match path should not be created")
+}
+
+func TestAddSkillFileWithTracking_RejectsInvalidPaths(t *testing.T) {
+	t.Run("rejects path that escapes skill directory", func(t *testing.T) {
+		gitRoot := testutil.TempDir(t, "test-add-skill-traversal-*")
+		resolved := &ResolvedWorkflow{
+			Spec: &WorkflowSpec{
+				WorkflowPath: "skills/foo/../../.github/workflows/evil.yml",
+			},
+			SkillName: "foo",
+			Content:   []byte("malicious"),
+		}
+
+		err := addSkillFileWithTracking(resolved, nil, AddOptions{Quiet: true}, gitRoot)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "escapes destination skill directory")
+	})
+
+	t.Run("rejects source path when skill root cannot be determined", func(t *testing.T) {
+		gitRoot := testutil.TempDir(t, "test-add-skill-missing-root-*")
+		resolved := &ResolvedWorkflow{
+			Spec: &WorkflowSpec{
+				WorkflowPath: "skills/bar/SKILL.md",
+			},
+			SkillName: "foo",
+			Content:   []byte("content"),
+		}
+
+		err := addSkillFileWithTracking(resolved, nil, AddOptions{Quiet: true}, gitRoot)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "failed to determine relative path")
+	})
+}

pkg/cli/add_package_manifest.go

@@ -23,6 +23,7 @@ var (
 var downloadPackageFileFromGitHubForHost = downloadRepositoryPackageFileFromGitHubForHost
 var listPackageWorkflowFilesForHost = listRepositoryPackageWorkflowFilesForHost
 var listPackageDirFilesForHost = listRepositoryPackageDirFilesForHost
+var listPackageDirFilesRecursivelyForHost = listRepositoryPackageDirFilesRecursivelyForHost
 var listPackageDirSubdirsForHost = listRepositoryPackageDirSubdirsForHost
 var getRepositoryPackageDefaultBranch = resolveRepositoryPackageDefaultBranch
 var addPackageManifestLog = logger.New("cli:add_package_manifest")
@@ -500,27 +501,60 @@ func agentDirectoryRoot(cleaned string) string {
 }
 
 // resolvePackageSkillFiles returns the list of resolvedPackageSkillFile for a package.
-// If explicitSkillDirs is non-empty it is used; otherwise the skills/ directory is
-// auto-scanned for subdirectories that contain a SKILL.md file.
+// Manifest-specified skills (explicitSkillDirs) are resolved first. After that, the
+// skills/ directory is always auto-scanned for any additional skill subdirectories that
+// contain a SKILL.md file but are not already covered by the manifest. Each skill folder
+// is traversed recursively so that all nested files are included.
 func resolvePackageSkillFiles(owner, repo, packagePath, ref, host string, explicitSkillDirs []string) ([]resolvedPackageSkillFile, []string, error) {
-	var skillDirs []string
-	explicitMode := len(explicitSkillDirs) > 0
-	if len(explicitSkillDirs) > 0 {
-		for _, dir := range explicitSkillDirs {
-			skillDirs = append(skillDirs, joinRepositoryPackagePath(packagePath, dir))
-		}
-	} else {
-		autoScanned, err := scanPackageSkillDirs(owner, repo, packagePath, ref, host)
-		if err != nil {
+	// seenSkillDirs tracks full skill directories already added so that auto-scanned
+	// duplicates of manifest-specified skills are not added a second time.
+	seenSkillDirs := make(map[string]struct{})
+	var warnings []string
+
+	// Step 1: resolve manifest skills first (explicit dirs).
+	var manifestSkillDirs []string
+	for _, dir := range explicitSkillDirs {
+		manifestSkillDirs = append(manifestSkillDirs, joinRepositoryPackagePath(packagePath, dir))
+	}
+
+	// Step 2: always auto-scan and append any skills not already in the manifest.
+	autoScanned, err := scanPackageSkillDirs(owner, repo, packagePath, ref, host)
+	if err != nil {
+		// Auto-scan is supplementary for manifest-declared skills; preserve manifest
+		// resolution even when scan fails transiently.
+		if len(manifestSkillDirs) > 0 {
+			warnings = append(warnings, fmt.Sprintf("failed to auto-scan skills directory, proceeding with manifest skills only: %v", err))
+		} else {
 			return nil, nil, err
 		}
-		skillDirs = autoScanned
+	}
+
+	// Build the final ordered list: manifest skills first, then auto-scanned extras.
+	var skillDirs []string
+	appendIfNew := func(dir string) {
+		if _, exists := seenSkillDirs[dir]; !exists {
+			seenSkillDirs[dir] = struct{}{}
+			skillDirs = append(skillDirs, dir)
+		}
+	}
+	for _, dir := range manifestSkillDirs {
+		appendIfNew(dir)
+	}
+	for _, dir := range autoScanned {
+		appendIfNew(dir)
+	}
+
+	// manifestSkillDirSet is used to know which dirs require a SKILL.md marker check.
+	manifestSkillDirSet := make(map[string]struct{}, len(manifestSkillDirs))
+	for _, d := range manifestSkillDirs {
+		manifestSkillDirSet[d] = struct{}{}
 	}
 
 	var skillFiles []resolvedPackageSkillFile
-	var warnings []string
 	for _, skillDir := range skillDirs {
-		if explicitMode {
+		// For skills that came from the manifest, validate that the SKILL.md marker
+		// exists so that typos in the manifest surface as clear warnings.
+		if _, fromManifest := manifestSkillDirSet[skillDir]; fromManifest {
 			markerPath := joinRepositoryPackagePath(skillDir, packageSkillMarkerFile)
 			if _, err := downloadPackageFileFromGitHubForHost(owner, repo, markerPath, ref, host); err != nil {
 				if isRepositoryFileNotFound(err) {
@@ -531,7 +565,9 @@ func resolvePackageSkillFiles(owner, repo, packagePath, ref, host string, explic
 			}
 		}
 		skillName := filepath.Base(skillDir)
-		files, err := listPackageDirFilesForHost(owner, repo, ref, skillDir, host)
+		// Use recursive listing so that the entire skill folder (including any
+		// subdirectories) is copied, not just the top-level files.
+		files, err := listPackageDirFilesRecursivelyForHost(owner, repo, ref, skillDir, host)
 		if err != nil {
 			if isRepositoryFileNotFound(err) {
 				warnings = append(warnings, fmt.Sprintf("Skill directory %q not found in package, skipping", skillDir))
@@ -796,6 +832,11 @@ func listRepositoryPackageDirFilesForHost(owner, repo, ref, dirPath, host string
 	return files, normalizeRepositoryPackageRemoteError(err)
 }
 
+func listRepositoryPackageDirFilesRecursivelyForHost(owner, repo, ref, dirPath, host string) ([]string, error) {
+	files, err := parser.ListDirAllFilesRecursivelyForHost(owner, repo, ref, dirPath, host)
+	return files, normalizeRepositoryPackageRemoteError(err)
+}
+
 func listRepositoryPackageDirSubdirsForHost(owner, repo, ref, dirPath, host string) ([]string, error) {
 	dirs, err := parser.ListDirSubdirsForHost(owner, repo, ref, dirPath, host)
 	return dirs, normalizeRepositoryPackageRemoteError(err)