Add repository-root aw.yml package resolution to gh aw add#32117
Conversation
|
@copilot ensure a schema of aw.yml is published in the docs |
|
@copilot the "compile" command should validate aw.yml and its contents |
|
@copilot add a field to describe the version of the aw.yml schema version |
|
@copilot add a min-version field that semver checks that the compiler is compatible with the manifest |
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
aw.yml package resolution to gh aw add
|
@copilot remove docs field, only support readme.md |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add support for subfolder syntax gh aw add org/repo/folder with a nested aw.yml |
|
@copilot use w3c-spec-writer to generate a spec for the aw.yml file format support. Store in docs with other specification files. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 00b84e7. Added
Addressed in 00b84e7. Added |
|
@copilot ensure add-wizard also support the manifest. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 1d45909. I used the W3C specification writer to draft the format and added the spec at
Addressed in 1d45909. Removed |
There was a problem hiding this comment.
Pull request overview
Adds repository-level “package” support to gh aw add by introducing an aw.yml manifest contract (schema + validation) and integrating manifest validation into gh aw compile.
Changes:
- Introduces an embedded JSON schema for
aw.ymland parser-side schema validation utilities. - Extends
gh aw addto resolveowner/repoandowner/repo/<path>as installable packages (manifest + fallback scanning) and surfaces non-fatal warnings. - Adds
gh aw compilevalidation for a repository-rootaw.ymland documents the new manifest format/spec.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/schemas/aw_manifest_schema.json | Adds JSON schema defining the aw.yml manifest contract. |
| pkg/parser/schema_validation.go | Adds manifest validation entrypoint that includes file location for errors. |
| pkg/parser/schema_compiler.go | Embeds/caches compiled schema for aw.yml. |
| pkg/cli/spec.go | Extends RepoSpec and factors out explicit-host detection helper. |
| pkg/cli/compile_repository_manifest.go | Validates local repo-root aw.yml (and README) during compile. |
| pkg/cli/compile_repository_manifest_test.go | Tests compile-time validation behavior and JSON output integration. |
| pkg/cli/compile_orchestrator.go | Wires manifest validation into the compile flow. |
| pkg/cli/add_workflow_resolution.go | Expands repository package specs into concrete workflow specs and collects warnings. |
| pkg/cli/add_wizard_command.go | Updates help text to include repository package specs. |
| pkg/cli/add_package_manifest.go | Implements remote package manifest loading, parsing, validation, and install bundle resolution. |
| pkg/cli/add_package_manifest_test.go | Unit tests for package resolution, validation, and add integration behavior. |
| pkg/cli/add_interactive_workflow.go | Uses resolved workflows to derive workflow names in interactive flows. |
| pkg/cli/add_interactive_orchestrator.go | Adds helpers to derive workflow names from resolved workflows (packages). |
| pkg/cli/add_interactive_orchestrator_test.go | Tests interactive name/file derivation for repository packages. |
| pkg/cli/add_command.go | Updates help text and prints package-resolution warnings during add. |
| pkg/cli/add_command_test.go | Updates validation expectations for invalid specs. |
| docs/src/content/docs/setup/cli.md | Documents repository package usage and compile-time validation. |
| docs/src/content/docs/reference/repository-package-manifest.md | Adds reference documentation for aw.yml. |
| docs/src/content/docs/reference/repository-package-manifest-specification.md | Adds normative manifest specification document. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 19/19 changed files
- Comments generated: 3
| for _, sourceDir := range packageSourceDirectories { | ||
| sourcePath := joinRepositoryPackagePath(packagePath, sourceDir) | ||
| files, err := listPackageWorkflowFiles(owner, repo, ref, sourcePath) | ||
| if err != nil { | ||
| if isRepositoryFileNotFound(err) { | ||
| continue | ||
| } | ||
| return nil, fmt.Errorf("failed to scan %q in %s/%s@%s: %w", sourcePath, owner, repo, ref, err) | ||
| } |
| readmePath := joinRepositoryPackagePath(packagePath, "README.md") | ||
| if _, err := downloadPackageFileFromGitHubForHost(owner, repo, readmePath, ref, host); err == nil { | ||
| return readmePath, nil | ||
| } else if isRepositoryFileNotFound(err) { | ||
| return "", fmt.Errorf("repository %q is not a valid Agentic Workflow package: missing required README.md at %q", owner+"/"+repo, readmePath) | ||
| } else { | ||
| return "", fmt.Errorf("failed to read package README %q from %s/%s@%s: %w", readmePath, owner, repo, ref, err) |
| if err := parser.ValidateRepositoryPackageManifestWithSchemaAndLocation(root, manifestPath); err != nil { | ||
| return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: %w", manifestPath, err) | ||
| } |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — this is a substantial new feature that introduces a manifest-driven install concept with multiple parsing layers.
Positive Highlights
- ✅ Well-scoped test suite: the new
add_package_manifest_test.goandcompile_repository_manifest_test.gocover a healthy range of validation paths (missing name, incompatiblemin-version, unknown fields, nested packages, scan fallback). - ✅ Graceful degradation: falling back from manifest resolution to the old
workflows/path lookup preserves backward compatibility cleanly. - ✅ Spec and docs land together: the normative spec, quick-reference doc, and inline CLI help text all ship in the same PR — a good habit.
- ✅
explicitHostForRepoextraction is a clean refactor that removes duplication inparseWorkflowSpec.
Key Themes
-
Fragile error detection via string matching —
isRepositoryFileNotFoundandisRepositoryPackageManifestNotFoundboth sniff error messages for"404"/"not found"/"no aw.yml manifest found". These are implicit contracts that break silently if the underlying error text changes. Sentinel errors (errors.Is) are safer and self-documenting. (See inline comments) -
Hardcoded
ref = "main"default — repos usingmasteror any other default branch will fail with a confusing "no aw.yml manifest found" error when no version is specified. Worth at minimum a prominent TODO comment; ideally, resolve the default branch from the API. (See inline comment) -
Duplicated
WorkflowSpecconstruction — the identical construction loop inResolveWorkflowsappears twice. A small helper would remove the duplication and make the two paths easier to compare. (See inline comment) -
Missing direct tests for
parseRepositoryPackageSpec— this is the routing function that decides whether a spec triggers package resolution or falls back to the old path. Per/tdd, its individual behaviors (path traversal,.mdexclusion, URL exclusion) deserve their own table-driven test rows, not just indirect coverage. (See inline comment) -
Silent
FindGitRooterror swallowing invalidateRepositoryManifestForCompilation— any git error (not just "not a git repo") causes manifest validation to be silently skipped. (See inline comment) -
Minor: unguarded
Warnings[0]index access in one test — could panic if the warning is not emitted. (See inline comment)
Overall Verdict
The feature is well-conceived and the implementation is solid. The issues above are mostly correctness/robustness concerns rather than design flaws, with the hardcoded "main" default and string-based error detection being the two worth addressing before merge. Marking as COMMENT rather than REQUEST_CHANGES since none of these are blocking for a first iteration, but the sentinel-error pattern in particular will pay dividends as the codebase grows.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 13.7M
| owner := parts[0] | ||
| repo := parts[1] | ||
| ref := repoSpec.Version | ||
| if ref == "" { |
There was a problem hiding this comment.
[/tdd] The ref is hardcoded to "main" when no version is supplied, but many repositories use master, develop, or another default branch name. A user running gh aw add owner/repo against a repo whose default is master will receive a cryptic "no aw.yml manifest found" 404 error.
Consider either:
- Resolving the repo's default branch via the GitHub API before the manifest fetch, or
- Documenting the assumption clearly so users know to supply an explicit version:
owner/repo@master
// TODO: uses "main" as fallback; repos with a different default branch must
// supply an explicit `@version`, e.g. owner/repo@master
ref = "main"| s, ok := value.(string) | ||
| return s, ok | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] isRepositoryFileNotFound (and isRepositoryPackageManifestNotFound below it) detect conditions by string-sniffing the error message. If the underlying download helper changes its error wording, these functions break silently — either treating a real 404 as a hard error, or treating a non-404 as "not found".
Prefer a sentinel error. For example:
var ErrFileNotFound = errors.New("file not found")
// wrap in the download function:
if resp.StatusCode == 404 {
return nil, fmt.Errorf("%w: %s", ErrFileNotFound, path)
}
// detection becomes safe:
func isRepositoryFileNotFound(err error) bool {
return errors.Is(err, ErrFileNotFound)
}This also makes the callsites self-documenting and avoids the implicit coupling to error message text.
| assert.Equal(t, "Repo Assist", pkg.Name) | ||
| assert.Equal(t, "README.md", pkg.DocsPath) | ||
| assert.Equal(t, []string{"workflows/review.md", ".github/workflows/nightly-review.md"}, pkg.InstallationSource) | ||
| assert.Contains(t, pkg.Warnings[0], "Ignoring files entry") |
There was a problem hiding this comment.
[/tdd] Direct index access on pkg.Warnings[0] will panic if Warnings is empty. Per the TDD principle of making tests read like specifications, guard it first:
require.NotEmpty(t, pkg.Warnings, "expected a warning about the ignored README.md files entry")
assert.Contains(t, pkg.Warnings[0], "Ignoring files entry")This also makes the test failure message self-explanatory if the warning is not emitted.
| return strings.HasSuffix(strings.ToLower(path), ".md") && | ||
| (strings.HasPrefix(path, "workflows/") || strings.HasPrefix(path, ".github/workflows/")) | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] The manual name check runs before ValidateRepositoryPackageManifestWithSchemaAndLocation, but the JSON schema presumably also validates name. This creates two error paths for the same constraint. If the schema's name error message is ever improved, the hand-written message here will diverge and confuse users.
Consider either:
- Running schema validation first and relying on it to reject a missing
name, or - Adding a comment explaining why the early manual check is intentional (e.g. to produce a friendlier error message that the schema validator cannot).
Right now the ordering is a silent coupling.
| if isRepositoryFileNotFound(err) { | ||
| continue | ||
| } | ||
| return nil, fmt.Errorf("failed to scan %q in %s/%s@%s: %w", sourcePath, owner, repo, ref, err) |
There was a problem hiding this comment.
[/tdd] parseRepositoryPackageSpec is a key routing function with several distinct behaviors (URL exclusion, .md exclusion, path traversal prevention, identifier validation) but it has no dedicated unit test. These behaviors are exercised transitively via TestResolveWorkflows_*, but a table-driven test for parseRepositoryPackageSpec itself would:
- Make the specification explicit (each row = one behavior)
- Protect the path-traversal check (e.g.
owner/repo/../../etc/passwd) independently of the full resolution pipeline - Catch regressions faster with a more targeted failure message
Suggested cases to cover:
owner/repo→ok=true,PackagePath=""owner/repo/folder→ok=true,PackagePath="folder"owner/repo@main→ok=true,Version="main"owner/repo/../../etc→ok=true, error returnedowner/repo/workflow.md→ok=false(explicit .md path)https://github.com/owner/repo→ok=false(URL)./local.md→ok=false(local path)
Addressed in b6788cd. I did a minimal cleanup pass on the manifest add flow: extracted the repeated repository-package workflow-spec construction into a single helper in |
|
@copilot review all comments , add tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in a27c1a5. Added targeted tests for explicit-host fallback scanning, nested-package README errors, direct
Addressed in a27c1a5. I reviewed the remaining comments and fixed the actionable follow-ups: repository-package fallback scans now use the explicit host, nested package README errors include the full package identifier, and the coverage was tightened with focused regressions. |
|
@copilot review all pull request review comments from github action bot |
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>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 529cb09. I reviewed the remaining GitHub Actions bot feedback and fixed the actionable items: repository packages now try the repo default branch before falling back to |
|
@copilot lint go |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
New Feature
What does this feature do?
gh aw add <owner>/<repo>can now treat a repository as an Agentic Workflow package by resolving a repository-rootaw.ymlmanifest instead of requiring an explicit workflow path. This givesgh aw adda deterministic source of truth for installable files, primary docs, and package-level warnings.Why is this feature needed?
The previous add flow only understood direct workflow paths, so repository-level installs had no stable contract for bundle selection or docs resolution. The new manifest path makes package installs explicit while preserving fallback scanning for simple repositories.
Implementation details
Manifest-driven repository installs
owner/repoaw.yml,agents.yml,agents.yamlParse-valid manifest enforcement
nameto be present and non-emptyInstall bundle resolution
files:when it contains valid installable markdown pathsfiles:entries with inline warningsworkflows/.github/workflows/Docs resolution
docs:when present and markdowndocs/<parameterized-name>.mddocs/<workflow-basename>.mdAdd-flow integration
gh aw addExample manifest now supported: