Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 22 additions & 22 deletions pkg/cli/add_package_manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (e packageRemoteNotFoundError) Unwrap() []error {
func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host string) (*resolvedRepositoryPackage, error) {
parts := strings.SplitN(repoSpec.RepoSlug, "/", 2)
if len(parts) != 2 {
return nil, fmt.Errorf("invalid repository slug: %s", repoSpec.RepoSlug)
return nil, fmt.Errorf("repository slug %q is not in 'owner/repo' format. Example: owner/repo", repoSpec.RepoSlug)
}

owner := parts[0]
Expand Down Expand Up @@ -153,7 +153,7 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri
warnings = append(warnings, agentWarnings...)

if len(installationSources) == 0 && len(skillFiles) == 0 && len(agentFiles) == 0 {
return nil, fmt.Errorf("repository %q does not contain any installable workflows, skills, or agents (either explicitly declared or auto-discovered)", repositoryPackageIdentifier(repoSpec.RepoSlug, packagePath))
return nil, fmt.Errorf("repository %q does not contain any installable workflows, skills, or agents (either explicitly declared or auto-discovered). Add workflows under 'workflows/', skills under 'skills/', or agents under 'agents/', or declare them explicitly in aw.yml", repositoryPackageIdentifier(repoSpec.RepoSlug, packagePath))
}

return &resolvedRepositoryPackage{
Expand All @@ -179,12 +179,12 @@ func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, package
content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, manifestPath, ref, host)
if err != nil {
if !isRepositoryFileNotFound(err) {
return "", nil, fmt.Errorf("failed to read manifest %q from %s/%s@%s: %w", manifestPath, owner, repo, ref, err)
return "", nil, fmt.Errorf("failed to read manifest %q from %s/%s@%s (check the repository, ref, and network connectivity): %w", manifestPath, owner, repo, ref, err)
}
if packagePath != "" {
return "", nil, fmt.Errorf("%w: repository %q is not a valid Agentic Workflow package: no aw.yml manifest found in %q; add %s or use an explicit workflow path", errRepositoryPackageManifestNotFound, packageID, packagePath, manifestPath)
return "", nil, fmt.Errorf("%w: repository %q is not a valid Agentic Workflow package: no aw.yml manifest found in %q. Add %s or use an explicit workflow path", errRepositoryPackageManifestNotFound, packageID, packagePath, manifestPath)
}
return "", nil, fmt.Errorf("%w: repository %q is not a valid Agentic Workflow package: no aw.yml manifest found at the repository root; add aw.yml or use an explicit workflow path", errRepositoryPackageManifestNotFound, repoSlug)
return "", nil, fmt.Errorf("%w: repository %q is not a valid Agentic Workflow package: no aw.yml manifest found at the repository root. Add aw.yml or use an explicit workflow path", errRepositoryPackageManifestNotFound, repoSlug)
}

return manifestPath, content, nil
Expand All @@ -207,19 +207,19 @@ type repositoryPackageManifest struct {
func parseRepositoryPackageManifest(manifestPath string, content []byte) (*repositoryPackageManifest, []string, error) {
var raw any
if err := yaml.Unmarshal(content, &raw); err != nil {
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: %s", manifestPath, parser.FormatYAMLError(err, 1, string(content)))
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: %s. Ensure the manifest is valid YAML. Example:\nname: My Package", manifestPath, parser.FormatYAMLError(err, 1, string(content)))
}

root, ok := raw.(map[string]any)
if !ok {
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: top-level document must be a mapping", manifestPath)
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: top-level document must be a mapping, not a list or scalar. Example:\nname: My Package", manifestPath)
}

// Validate name before schema validation to provide a clear error message for
// the most common manifest authoring error (missing or empty name).
name, ok := stringValue(root["name"])
if !ok || strings.TrimSpace(name) == "" {
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: name must be a non-empty string", manifestPath)
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: name must be a non-empty string. Example:\nname: My Package", manifestPath)
}

if err := parser.ValidateRepositoryPackageManifestWithSchemaAndLocation(root, manifestPath); err != nil {
Expand All @@ -240,15 +240,15 @@ func parseRepositoryPackageManifest(manifestPath string, content []byte) (*repos
if minVersion, ok := stringValue(root["min-version"]); ok {
manifest.MinVersion = strings.TrimSpace(minVersion)
if !isSupportedManifestMinVersion(manifest.MinVersion) {
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version must use vMAJOR.minor.patch, got %q", manifestPath, minVersion)
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version must use vMAJOR.minor.patch, got %q. Example:\nmin-version: v1.2.3", manifestPath, minVersion)
}
currentVersion := GetVersion()
if !semverutil.IsValid(currentVersion) {
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version validation requires a semantic-versioned compiler, but the current compiler version %q is not a valid semantic version (this indicates a build issue)", manifestPath, currentVersion)
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version validation requires a semantic-versioned compiler, but the current compiler version %q is not a valid semantic version. This indicates a build issue; rebuild gh-aw with a proper version tag. Example: v1.2.3", manifestPath, currentVersion)
}
currentVersion = semverutil.NormalizeGitDescribeSemver(currentVersion)
if semverutil.Compare(currentVersion, manifest.MinVersion) < 0 {
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version %q requires gh-aw %s or newer (current: %s)", manifestPath, manifest.MinVersion, manifest.MinVersion, currentVersion)
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version %q requires gh-aw %s or newer (current: %s). Upgrade gh-aw, or lower min-version in aw.yml to a version at or below the current one. Example:\nmin-version: %s", manifestPath, manifest.MinVersion, manifest.MinVersion, currentVersion, currentVersion)
}
}

Expand Down Expand Up @@ -597,7 +597,7 @@ func resolvePackageSkillFiles(ctx context.Context, owner, repo, packagePath, ref
warnings = append(warnings, fmt.Sprintf("Skill directory %q is missing required %s marker file", skillDir, packageSkillMarkerFile))
continue
}
return nil, nil, fmt.Errorf("failed to validate skill marker %q: %w", markerPath, err)
return nil, nil, fmt.Errorf("failed to validate skill marker %q (check the repository, ref, and network connectivity): %w", markerPath, err)
}
}
skillName := filepath.Base(skillDir)
Expand All @@ -609,7 +609,7 @@ func resolvePackageSkillFiles(ctx context.Context, owner, repo, packagePath, ref
warnings = append(warnings, fmt.Sprintf("Skill directory %q not found in package, skipping", skillDir))
continue
}
return nil, nil, fmt.Errorf("failed to list files in skill directory %q: %w", skillDir, err)
return nil, nil, fmt.Errorf("failed to list files in skill directory %q (check the repository, ref, and network connectivity): %w", skillDir, err)
}
for _, file := range files {
skillFiles = append(skillFiles, resolvedPackageSkillFile{
Expand Down Expand Up @@ -641,7 +641,7 @@ func resolvePackageAgentFiles(ctx context.Context, owner, repo, packagePath, ref
if isRepositoryFileNotFound(err) {
continue
}
return nil, nil, fmt.Errorf("failed to scan agents directory %q: %w", agentsDir, err)
return nil, nil, fmt.Errorf("failed to scan agents directory %q (check the repository, ref, and network connectivity): %w", agentsDir, err)
}
for _, f := range files {
if strings.HasSuffix(strings.ToLower(f), ".md") {
Expand All @@ -663,7 +663,7 @@ func scanPackageSkillDirs(ctx context.Context, owner, repo, packagePath, ref, ho
if isRepositoryFileNotFound(err) {
continue
}
return nil, fmt.Errorf("failed to scan skills directory %q: %w", skillsDir, err)
return nil, fmt.Errorf("failed to scan skills directory %q (check the repository, ref, and network connectivity): %w", skillsDir, err)
}
for _, subdir := range subdirs {
markerPath := joinRepositoryPackagePath(subdir, packageSkillMarkerFile)
Expand All @@ -686,7 +686,7 @@ func scanRepositoryPackageInstallablePaths(ctx context.Context, owner, repo, pac
if isRepositoryFileNotFound(err) {
continue
}
return nil, fmt.Errorf("failed to scan %q in %s/%s@%s: %w", sourcePath, owner, repo, ref, err)
return nil, fmt.Errorf("failed to scan %q in %s/%s@%s (check the repository, ref, and network connectivity): %w", sourcePath, owner, repo, ref, err)
}

for _, file := range files {
Expand Down Expand Up @@ -719,9 +719,9 @@ func resolveRepositoryPackageDocsPath(ctx context.Context, owner, repo, packageP
if _, err := downloadPackageFileFromGitHubForHost(ctx, 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", packageID, readmePath)
return "", fmt.Errorf("repository %q is not a valid Agentic Workflow package: missing required README.md at %q. Add a README.md describing the package. Example:\n# My Package\n\nDescribe what this package does.", packageID, readmePath)
} else {
return "", fmt.Errorf("failed to read package README %q from %s/%s@%s: %w", readmePath, owner, repo, ref, err)
return "", fmt.Errorf("failed to read package README %q from %s/%s@%s (check the repository, ref, and network connectivity): %w", readmePath, owner, repo, ref, err)
}
}

Expand Down Expand Up @@ -771,7 +771,7 @@ func validateManifestInstallableWorkflowPrivacy(manifestPath string, installatio

privateValue, hasPrivate := ExtractWorkflowPrivateSetting(string(content))
if hasPrivate && privateValue {
return fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added", manifestPath, installationSource)
return fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added. Remove 'private: true' from the workflow frontmatter or exclude it from the manifest. Example:\n---\nprivate: false\n---", manifestPath, installationSource)
}
}

Expand Down Expand Up @@ -829,7 +829,7 @@ func parseRepositoryPackageSpec(spec string) (*RepoSpec, bool, error) {
if cleanedPath == "." {
packagePath = ""
} else if cleanedPath == ".." || strings.HasPrefix(cleanedPath, "../") {
return nil, true, fmt.Errorf("invalid repository package path %q", packagePath)
return nil, true, fmt.Errorf("invalid repository package path %q: path traversal outside the repository is not allowed. Use a path relative to the repository root. Example: packages/my-package", packagePath)
} else {
packagePath = cleanedPath
}
Expand Down Expand Up @@ -883,7 +883,7 @@ func validateUniqueManifestWorkflowFilenames(paths []string, manifestPath string
continue
}
if previous, exists := seen[key]; exists {
return fmt.Errorf("invalid Agentic Workflow manifest %q: duplicate workflow filename %q in files entries %q and %q (filenames must be unique across a package)", manifestPath, filenameWithoutExt, previous, installPath)
return fmt.Errorf("invalid Agentic Workflow manifest %q: duplicate workflow filename %q in files entries %q and %q. Filenames must be unique across a package; rename one of the workflow files. Example:\nfiles:\n - workflows/%s.md\n - workflows/%s-2.md", manifestPath, filenameWithoutExt, previous, installPath, filenameWithoutExt, filenameWithoutExt)
}
seen[key] = installPath
}
Expand Down Expand Up @@ -952,7 +952,7 @@ func resolveRepositoryPackageDefaultBranch(ctx context.Context, repoSlug, host s
if targetHost == "" {
targetHost = "the configured host"
}
return "", fmt.Errorf("repository %s on %s returned an empty default branch; ensure the repository exists and is accessible", repoSlug, targetHost)
return "", fmt.Errorf("repository %s on %s returned an empty default branch. Ensure the repository exists and is accessible", repoSlug, targetHost)
}
return branch, nil
}
Expand Down
28 changes: 14 additions & 14 deletions pkg/cli/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func parseRepoSpec(repoSpec string) (*RepoSpec, error) {
repoURL, err := url.Parse(repo)
if err != nil {
specLog.Printf("Failed to parse GitHub URL: %v", err)
return nil, fmt.Errorf("invalid GitHub URL: %w", err)
return nil, fmt.Errorf("could not parse GitHub URL %q (use a URL like https://github.com/owner/repo): %w", repo, err)
}

// Extract owner/repo from path
Expand All @@ -145,7 +145,7 @@ func parseRepoSpec(repoSpec string) (*RepoSpec, error) {
currentRepo, err := GetCurrentRepoSlug()
if err != nil {
specLog.Printf("Failed to get current repo: %v", err)
return nil, fmt.Errorf("failed to get current repository info: %w", err)
return nil, fmt.Errorf("failed to get current repository info (run this command from inside a git repository with a GitHub remote, or specify 'owner/repo' explicitly): %w", err)
}
repo = currentRepo
specLog.Printf("Resolved current repo: %s", repo)
Expand Down Expand Up @@ -181,15 +181,15 @@ func parseGitHubURL(spec string) (*WorkflowSpec, error) {
parsedURL, err := url.Parse(spec)
if err != nil {
specLog.Printf("Failed to parse URL: %v", err)
return nil, fmt.Errorf("invalid URL: %w", err)
return nil, fmt.Errorf("could not parse URL %q (use a URL like https://github.com/owner/repo/blob/main/workflows/workflow.md): %w", spec, err)
}

if parsedURL.Host == "" {
return nil, fmt.Errorf("URL must include a host: %s", spec)
return nil, fmt.Errorf("URL %q is missing a host. Use a full URL. Example: https://github.com/owner/repo/blob/main/workflows/workflow.md", spec)
}

if !isGitHubHost(parsedURL.Host) {
return nil, fmt.Errorf("URL must be from github.com or a GitHub Enterprise host (*.ghe.com), got %q", parsedURL.Host)
return nil, fmt.Errorf("URL host %q is not supported. Expected github.com or a GitHub Enterprise host (*.ghe.com). Example: https://github.com/owner/repo/blob/main/workflows/workflow.md", parsedURL.Host)
}

owner, repo, ref, filePath, err := parser.ParseRepoFileURL(spec)
Expand All @@ -202,12 +202,12 @@ func parseGitHubURL(spec string) (*WorkflowSpec, error) {

// Ensure the file path ends with .md
if !strings.HasSuffix(filePath, ".md") {
return nil, errors.New("GitHub URL must point to a .md file")
return nil, errors.New("GitHub URL must point to a .md file. Example: https://github.com/owner/repo/blob/main/workflows/workflow.md")
}

// Validate owner and repo
if !parser.IsValidGitHubIdentifier(owner) || !parser.IsValidGitHubRepositoryName(repo) {
return nil, fmt.Errorf("invalid GitHub URL: '%s/%s' does not look like a valid GitHub repository", owner, repo)
return nil, fmt.Errorf("GitHub URL contains '%s/%s', which does not look like a valid GitHub repository. Expected owner and repository names with only letters, numbers, hyphens, and underscores", owner, repo)
}

// For raw.githubusercontent.com content, the API host is github.com.
Expand Down Expand Up @@ -294,10 +294,10 @@ func parseWorkflowSpec(spec string) (*WorkflowSpec, error) {
// Non-GitHub HTTP(S) URL: return a generic URL spec whose content will be
// fetched at resolution time and dispatched on Content-Type.
if urlErr != nil {
return nil, fmt.Errorf("invalid URL %q: %w", spec, urlErr)
return nil, fmt.Errorf("could not parse URL %q (use a fully qualified http(s) URL): %w", spec, urlErr)
}
if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
return nil, fmt.Errorf("unsupported URL scheme %q: only http and https are supported", parsedURL.Scheme)
return nil, fmt.Errorf("URL scheme %q is not supported. Only http and https are supported. Example: https://example.com/workflow.md", parsedURL.Scheme)
}
specLog.Printf("Detected generic import URL: %s", spec)
return &WorkflowSpec{
Expand Down Expand Up @@ -341,7 +341,7 @@ func parseWorkflowSpec(spec string) (*WorkflowSpec, error) {

// Must have at least 3 parts: owner/repo/workflow-path
if len(slashParts) < 3 {
return nil, errors.New("workflow specification must be in format 'owner/repo/workflow-name[@version]'")
return nil, errors.New("workflow specification format is not recognized. Expected 'owner/repo/workflow-name[@version]'. Example: github/gh-aw/ci-doctor")
}

owner := slashParts[0]
Expand All @@ -367,12 +367,12 @@ func parseWorkflowSpec(spec string) (*WorkflowSpec, error) {

// Validate owner and repo parts are not empty
if owner == "" || repo == "" {
return nil, errors.New("invalid workflow specification: owner and repo cannot be empty")
return nil, errors.New("workflow specification is missing owner or repo. Expected 'owner/repo/workflow-name[@version]'. Example: github/gh-aw/ci-doctor")
}

// Basic validation that owner and repo look like GitHub identifiers
if !parser.IsValidGitHubIdentifier(owner) || !parser.IsValidGitHubRepositoryName(repo) {
return nil, fmt.Errorf("invalid workflow specification: '%s/%s' does not look like a valid GitHub repository", owner, repo)
return nil, fmt.Errorf("workflow specification contains '%s/%s', which does not look like a valid GitHub repository. Expected owner and repository names with only letters, numbers, hyphens, and underscores", owner, repo)
}

repoSlug := fmt.Sprintf("%s/%s", owner, repo)
Expand Down Expand Up @@ -407,7 +407,7 @@ func parseWorkflowSpec(spec string) (*WorkflowSpec, error) {
// Four or more parts: owner/repo/workflows/workflow-name or owner/repo/path/to/workflow-name
// Require .md extension to be explicit
if !strings.HasSuffix(workflowPath, ".md") {
return nil, fmt.Errorf("workflow specification with path must end with '.md' extension: %s", workflowPath)
return nil, fmt.Errorf("workflow specification path %q must end with '.md' extension. Example: owner/repo/workflows/ci-doctor.md", workflowPath)
}
}

Expand All @@ -428,7 +428,7 @@ func parseLocalWorkflowSpec(spec string) (*WorkflowSpec, error) {
// Validate that it's a .md file
if !strings.HasSuffix(spec, ".md") {
specLog.Printf("Invalid extension for local workflow: %s", spec)
return nil, fmt.Errorf("local workflow specification must end with '.md' extension: %s", spec)
return nil, fmt.Errorf("local workflow specification %q must end with '.md' extension. Example: ./workflows/ci-doctor.md", spec)
}

specLog.Printf("Parsed local workflow: path=%s", spec)
Expand Down
Loading