Support source-to-destination mappings in aw.yml includes - #53698
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
includes
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
The manifest-mapping parsing and validation look mostly reasonable, but the update path still assumes every newly added manifest-managed workflow is markdown. That leaves the new source→destination support incomplete for .yml action-workflow mappings and will break gh aw update for that case.
Blocking theme
- New manifest-managed action workflows added during
gh aw updateare still funneled through markdown-only frontmatter rewriting and written as*.md, so mapped.ymlentries cannot be added correctly.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 25.6 AIC · ⌖ 10 AIC · ⊞ 7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/update_manifest.go:301
This breaks gh aw update for manifest-managed .yml mappings: when a package adds a new action-workflow entry, addManifestManagedWorkflow still rewrites frontmatter and writes name + ".md", so the update path either errors on YAML input or installs the new file with the wrong extension instead of creating the declared .yml workflow.
<details><summary>💡 Why this is blocking</summary>
manifestWorkflowPathByName() now correctly includes only markdown entries, but the add path still a…
There was a problem hiding this comment.
Review: Support source-to-destination mappings in aw.yml includes
The implementation is well-structured. The type refactor ([]string → []resolvedPackageInstallable) is applied consistently across all call sites, the JSON schema is updated to accept both string and object entries, and the new test file provides solid coverage of the mapping feature.
Two issues worth addressing:
🟡 Medium — validateUniqueManifestInstallDestinations not called in validateLocalRepositoryPackageContents
resolveRepositoryPackageInstallablePaths (remote) and resolveLocalRepositoryPackage (local install) both call validateUniqueManifestInstallDestinations after the PR. However, validateLocalRepositoryPackageContents in compile_repository_manifest.go (used by gh aw compile) only calls validateManifestInstallableWorkflowPrivacy — it does not call validateUniqueManifestInstallDestinations. Two mapping entries with the same destination will therefore pass gh aw compile silently, even though the spec says duplicate destinations must be rejected before any file is written.
🔵 Low — normalizePackageInstallablePaths deduplicates on source path, not destination
if _, exists := seen[sourcePath]; exists {
continue
}
seen[sourcePath] = struct{}{}Two entries with different sources but the same destination (e.g. a string entry and a mapping entry that both resolve to .github/workflows/foo.md) will both be retained and will only be caught later by validateUniqueManifestInstallDestinations. That later check covers the case, so this isn't a bug per se — but adding the destination to the seen key (or checking both) in normalizePackageInstallablePaths would make the deduplication self-contained and easier to reason about.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 54.4 AIC · ⌖ 9.01 AIC · ⊞ 5.7K
| installationSources = packageInstallablesFromSourcePaths(scanned) | ||
| } | ||
|
|
||
| return validateManifestInstallableWorkflowPrivacy(manifestPath, installationSources, func(sourcePath string) ([]byte, error) { |
There was a problem hiding this comment.
Missing validateUniqueManifestInstallDestinations call
validateLocalRepositoryPackageContents (used by gh aw compile) builds installationSources but then only calls validateManifestInstallableWorkflowPrivacy. It never calls validateUniqueManifestInstallDestinations, so two mapping entries that resolve to the same destination will silently pass gh aw compile even though the spec requires this to be rejected before any file is written.
Add the missing check after the existing validateManifestInstallableWorkflowPrivacy call:
if err := validateUniqueManifestInstallDestinations(installationSources, manifestPath); err != nil {
return err
}@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds source-to-destination mappings for repository package workflows while preserving legacy include resolution.
Changes:
- Extends manifest schema, parsing, validation, and resolution.
- Threads destinations through installation and update logic.
- Adds mapping tests and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schemas/aw_manifest_schema.json |
Defines mapping schema. |
pkg/cli/update_manifest.go |
Maps installed names to sources. |
pkg/cli/spec.go |
Adds destination metadata. |
pkg/cli/compile_repository_manifest.go |
Adapts manifest validation. |
pkg/cli/add_workflow_resolution.go |
Resolves local and remote destinations. |
pkg/cli/add_workflow_resolution_manifest_ref_test.go |
Updates resolution fixtures. |
pkg/cli/add_package_manifest.go |
Parses and validates mappings. |
pkg/cli/add_package_manifest_test.go |
Updates manifest tests. |
pkg/cli/add_package_manifest_mapping_test.go |
Tests mapping behavior. |
pkg/cli/add_command.go |
Installs using destination-derived names. |
docs/src/content/docs/specs/repository-package-manifest-specification.md |
Specifies mappings. |
docs/src/content/docs/reference/aw-yml-package-manifest.md |
Documents mapping usage. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/cli/add_package_manifest.go:1073
- Distinct mappings that reuse one source are silently collapsed because deduplication keys only on
sourcePath. For example, mappingpayload/reviewer.mdto bothreviewer.mdandreviewer-copy.mdinstalls only the first destination, even though these are not duplicate entries. Include the destination in the key so only identical mappings are ignored.
if _, exists := seen[sourcePath]; exists {
continue
}
seen[sourcePath] = struct{}{}
pkg/cli/add_workflow_resolution.go:579
Lstatchecks only the final component and follows symlinked parent directories. A source such aspayload/workflows/reviewer.mdis therefore accepted whenpayload/workflowsis a symlink, including when it points outside the package, violating the local-source containment requirement. Check every path component withLstat(or compare fully resolved package/source paths) before reading the file.
info, err := os.Lstat(absolutePath)
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Balanced
| if _, exists := seen[absolutePath]; exists { | ||
| continue | ||
| } | ||
| seen[absolutePath] = struct{}{} |
| includeInstallablePaths, _, _ := splitManifestIncludePaths(manifest.Includes) | ||
| includeInstallablePaths = append(includeInstallablePaths, manifest.Files...) | ||
| includeInstallablePaths = append(includeInstallablePaths, manifestIncludesFromPaths(manifest.Files)...) | ||
| installationSources := normalizePackageInstallablePaths(includeInstallablePaths, "") |
| if !strings.HasSuffix(strings.ToLower(installable.DestinationPath), ".md") { | ||
| continue | ||
| } | ||
| workflowID := normalizeWorkflowID(filepath.Base(p)) | ||
| byName[workflowID] = p | ||
| workflowID := normalizeWorkflowID(filepath.Base(installable.DestinationPath)) | ||
| byName[workflowID] = installable.SourcePath |
| - `source` or `destination` is empty, absolute, or contains a path-traversal sequence that escapes its root; | ||
| - `source` resolves to a symbolic link or to a path outside the package root; | ||
| - `source` or `destination` does not end in `.md` or `.yml`, or ends in `.lock.yml`; |
| slashed := filepath.ToSlash(p) | ||
| if strings.HasPrefix(slashed, "/") || strings.HasPrefix(slashed, "\\") || filepath.IsAbs(p) || isWindowsDriveRelativePath(slashed) { | ||
| return "", errors.New("absolute paths are not allowed") | ||
| } | ||
| cleaned := path.Clean(slashed) | ||
| if cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") { | ||
| return "", errors.New("path traversal outside the root is not allowed") | ||
| } | ||
| return cleaned, nil |
There was a problem hiding this comment.
Ponytail review: mostly lean, well-tested addition. One duplication spotted.
net: -6 lines possible.
Generated by ✂️ Ponytail Reviewer for #53698 · auto · 74.9 AIC · ⌖ 6.89 AIC · ⊞ 7.3K
Comment /ponytail to run again
|
|
||
| // isWindowsDriveRelativePath reports whether p starts with a Windows drive letter prefix | ||
| // (e.g. "C:/payload"). filepath.IsAbs does not detect these on non-Windows hosts. | ||
| func isWindowsDriveRelativePath(p string) bool { |
There was a problem hiding this comment.
L591: yagni: isWindowsDriveRelativePath reimplements the same drive-letter check as isWindowsDrivePath in docker_args_validation.go. Reuse/extend that helper instead of a second copy.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness gap and three test coverage issues.
📋 Key Themes & Highlights
Key Themes
- Missing validator call in compile path (
compile_repository_manifest.go):validateUniqueManifestInstallDestinationsis called in theaddandupdatepaths but not ingh aw compile, so a manifest with two mapping entries that share adestinationcompiles cleanly on the distribution side and only fails at consumer install time. This is the highest-impact issue. - Test coverage gaps: The duplicate-destination test for
.mdfiresvalidateUniqueManifestWorkflowFilenames(a pre-existing check) rather than the newvalidateUniqueManifestInstallDestinations, leaving the new validator un-exercised for that file type. Afiles+mapping cross-field duplicate scenario is also untested.
Positive Highlights
- ✅ The
resolvedPackageInstallabletype is a clean and well-named abstraction that unifies source and destination paths. - ✅ The separation between
validateUniqueManifestWorkflowFilenames(existing,.md-only, basename-stem keyed) andvalidateUniqueManifestInstallDestinations(new, full-path keyed) is sound. - ✅
cleanManifestRelativePathhandles Windows drive-letter prefixes on non-Windows hosts — good cross-platform defensive coding. - ✅ Error messages consistently include an actionable example with the correct path.
- ✅ The new test file covers the full validation matrix thoroughly for the happy and rejection paths.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 101 AIC · ⌖ 11.5 AIC · ⊞ 7.8K
Comment /matt to run again
| installationSources = packageInstallablesFromSourcePaths(scanned) | ||
| } | ||
|
|
||
| return validateManifestInstallableWorkflowPrivacy(manifestPath, installationSources, func(sourcePath string) ([]byte, error) { |
There was a problem hiding this comment.
[/diagnosing-bugs] validateUniqueManifestInstallDestinations is never called in the compile path, so duplicate-destination mappings pass validation during gh aw compile even though they are rejected by gh aw add.
💡 Suggested fix
Add the call right after validateUniqueManifestWorkflowFilenames (mirrors the pattern in add_package_manifest.go and add_workflow_resolution.go):
if err := validateUniqueManifestWorkflowFilenames(installationSources, manifestPath); err != nil {
return err
}
if err := validateUniqueManifestInstallDestinations(installationSources, manifestPath); err != nil {
return err
}
return validateManifestInstallableWorkflowPrivacy(...)Without this, a manifest with two mapping entries that share a destination will compile cleanly on the distribution side but fail at install time in the consumer's repository.
@copilot please address this.
| } | ||
| seen[path] = struct{}{} | ||
| normalized = append(normalized, path) | ||
| seen[sourcePath] = struct{}{} |
There was a problem hiding this comment.
[/tdd] The normalizePackageInstallablePaths deduplication uses source path as the key, meaning two entries with different sources but the same destination are both emitted. The subsequent validateUniqueManifestInstallDestinations catches this — but only in the add/update paths. Combined with the missing call in compile_repository_manifest.go, a duplicate-destination manifest distributes successfully.
💡 Why this matters
The dedup-by-source here is intentional (avoid re-fetching the same remote file twice), but it means duplicate-destination detection is entirely delegated to validateUniqueManifestInstallDestinations. Once that validator is also added to the compile path, both protections work together as intended.
Consider adding a test in compile_repository_manifest that exercises a manifest with two mapping entries sharing a destination, to pin the compile-time rejection.
@copilot please address this.
| {SourcePath: "factory/payload/workflows/controller.yml", DestinationPath: ".github/workflows/controller.yml"}, | ||
| }) | ||
| assert.Equal(t, map[string]string{ | ||
| "code-reviewer": "factory/payload/workflows/reviewer.md", |
There was a problem hiding this comment.
[/tdd] The test at line 243 (rejects duplicate markdown destinations) asserts "duplicate workflow filename" — this fires validateUniqueManifestWorkflowFilenames, which runs before validateUniqueManifestInstallDestinations. The test therefore never exercises the new validateUniqueManifestInstallDestinations code path for .md files. A test specifically asserting "both install to" for a .md duplicate would pin the new validator.
💡 Suggested addition
Add a sub-case to invalidCases in TestResolveRepositoryPackage_IncludeMappings:
{
name: "rejects duplicate md destinations via install destination check",
includes: ` - source: payload/a/reviewer.md
destination: .github/workflows/reviewer.md
- source: payload/b/other.md
destination: .github/workflows/reviewer.md`,
contains: "both install to \".github/workflows/reviewer.md\"",
},Or alternatively, make the existing rejects duplicate markdown destinations sub-test assert the validateUniqueManifestInstallDestinations error message rather than the filename-dedup one, since duplicate destinations are the root concept being tested.
@copilot please address this.
| func resolveRepositoryPackageInstallablePaths(ctx context.Context, owner, repo, packagePath, ref, host string, manifest *repositoryPackageManifest, manifestPath string) ([]resolvedPackageInstallable, []string, []string, error) { | ||
| includeInstallablePaths, includeSkillDirs, includeAgentFiles := splitManifestIncludePaths(manifest.Includes) | ||
| includeInstallablePaths = append(includeInstallablePaths, manifest.Files...) | ||
| includeInstallablePaths = append(includeInstallablePaths, manifestIncludesFromPaths(manifest.Files)...) |
There was a problem hiding this comment.
[/tdd] There is no test covering the case where a files entry and a mapping entry resolve to the same install destination. The files entries are wrapped as string-form includes with an empty Destination (so defaultPackageInstallDestination is used), which could conflict with a mapping entry pointing at the same basename. This cross-field duplicate scenario isn't exercised by the new test file.
💡 Suggested test scenario
name: Factory
files:
- payload/reviewer.md # installs to .github/workflows/reviewer.md
includes:
- source: payload/other/reviewer.md
destination: .github/workflows/reviewer.mdThis should produce a duplicate-destination error. A test verifying this keeps the semantics explicit and prevents regressions when the files/includes merge logic changes.
@copilot please address this.
Documents the architectural decision to extend the `includes` manifest field with object-form entries that pair a package-relative source path with a consumer-repository-root-relative destination path.
This comment has been minimized.
This comment has been minimized.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (786 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
|
|
@copilot run pr-finisher skill fix https://github.com/github/gh-aw/actions/runs/32150579681/job/95794792893?pr=53698 |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 42c1eef: |
PR Triage
Largest and highest-risk PR in this batch (13 files, 900+ lines) adding source-to-destination mappings for
|
Nested
aw.ymlpackages treat.github/**entries as repository-root-relative (ADR-41790), so a distribution repository cannot keep executable workflow assets inert under a package directory and still install them into the consumer's.github/workflows/.includesaccepted only path strings, with no way to express separate source and destination paths.includesentries may now be an object declaring a package-relativesourceand a repository-relativedestination:The sources stay outside
.github/workflows/in the distribution repository, so they never run there.Schema
includesitems accept the existing string form or an object with requiredsource/destinationand optionalkind(agentic-workflow|action-workflow).Model and resolution
repositoryPackageInclude; resolution yieldsresolvedPackageInstallable{SourcePath, DestinationPath}, replacing bare path strings across package resolution (resolvedRepositoryPackage.InstallationSource)..github/**stays repository-root-relative, everything else stays package-relative. Mappingsourceis always resolved against the package path;destinationis validated independently as a repository-root install path..github/workflows/<basename>), unifying downstream handling.Validation
Mappings are rejected for absolute paths (including Windows drive prefixes),
..traversal, unsupported or.lock.ymlextensions, extension changes between source and destination, destinations outside or nested under.github/workflows/, andkindthat disagrees with the source extension. Local packages additionally reject symlinked, missing, or directory sources. Duplicate destinations are detected before any file is written. No package-provided shell code is executed.Install and update
WorkflowSpec.DestinationPathcarries the resolved install path;WorkflowNameis derived from it, so a mapped entry installs under its destination name..mdcompiles,.ymlis copied verbatim.gh aw updatekeys installed workflows by destination and maps them back to package sources, preserving manifest-scoped source tracking.gh aw add-wizardreusesAddResolvedWorkflows, so semantics are identical acrossadd,add-wizard, andupdate.Docs
Reference and specification (§4.9) updated, including an explicit statement of the differing resolution rules for
.github/**string entries versus mapping sources.Tests
New
add_package_manifest_mapping_test.gocovers nested packages (remote fetch offactory/payload/...installing to.github/workflows/...), root packages, mixed legacy/mapping entries, destination renaming, every rejection case, duplicate destinations for both.mdand.yml, local symlink/missing-source handling, update name→source mapping, and an end-to-end install asserting mapped destinations for agentic and deterministic workflows.