From be9d6f43bf19863af6a14e8f348ad2a7e17c401d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 06:35:11 +0000 Subject: [PATCH 01/13] Initial plan From fe53643cb6ed8f8f60aac8ec6c9b2e758b45f6e8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 06:50:00 +0000 Subject: [PATCH 02/13] Include grader evaluators in package resources Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_package_manifest_resolve.go | 8 ++- pkg/cli/add_package_manifest_resources.go | 64 +++++++++++++++++++++++ pkg/cli/add_package_manifest_test.go | 39 ++++++++++++++ pkg/cli/update_manifest_test.go | 38 ++++++++++++++ 4 files changed, 148 insertions(+), 1 deletion(-) diff --git a/pkg/cli/add_package_manifest_resolve.go b/pkg/cli/add_package_manifest_resolve.go index 1f9faf0ba74..acb25cb6004 100644 --- a/pkg/cli/add_package_manifest_resolve.go +++ b/pkg/cli/add_package_manifest_resolve.go @@ -6,6 +6,7 @@ package cli import ( "context" "fmt" + "path" "strings" ) @@ -32,6 +33,11 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri return nil, err } resourceFiles := normalizePackageResourcePaths(manifest.Resources, packagePath) + graderResources, err := resolveRepositoryPackageGraderResources(ctx, owner, repo, ref, host, installationSources) + if err != nil { + return nil, err + } + resourceFiles = appendUniquePackageResources(resourceFiles, graderResources) docsPath, err := resolveRepositoryPackageDocsPath(ctx, owner, repo, packagePath, ref, host) if err != nil { @@ -173,7 +179,7 @@ func newResolvedRepositoryPackage(manifestPath, ref, docsPath string, manifest * func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, packagePath, ref, host string) (string, []byte, error) { manifestPath := joinRepositoryPackagePath(packagePath, repositoryPackageManifestFileName) - repoSlug := owner + "/" + repo + repoSlug := path.Join(owner, repo) packageID := repositoryPackageIdentifier(repoSlug, packagePath) content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, manifestPath, ref, host) if err != nil { diff --git a/pkg/cli/add_package_manifest_resources.go b/pkg/cli/add_package_manifest_resources.go index f4cd5970fc1..d50cd6c6c0b 100644 --- a/pkg/cli/add_package_manifest_resources.go +++ b/pkg/cli/add_package_manifest_resources.go @@ -1,6 +1,7 @@ package cli import ( + "context" "errors" "fmt" "path" @@ -8,6 +9,8 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/workflow" ) type repositoryPackageResource struct { @@ -105,6 +108,67 @@ func normalizePackageResourcePaths(resources []repositoryPackageResource, packag return normalized } +func resolveRepositoryPackageGraderResources(ctx context.Context, owner, repo, ref, host string, installables []resolvedPackageInstallable) ([]resolvedPackageResource, error) { + var resources []resolvedPackageResource + seen := make(map[string]struct{}) + for _, installable := range installables { + if !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".md") { + continue + } + content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, installable.SourcePath, ref, host) + if err != nil { + if isRepositoryFileNotFound(err) { + continue + } + return nil, fmt.Errorf("failed to inspect packaged workflow %q for grader resources: %w", installable.SourcePath, err) + } + result, err := parser.ExtractFrontmatterFromContent(string(content)) + if err != nil { + return nil, fmt.Errorf("failed to inspect packaged workflow %q for grader resources: %w", installable.SourcePath, err) + } + if result.Frontmatter == nil { + continue + } + graders, err := workflow.ParseGradersFromFrontmatter(result.Frontmatter) + if err != nil { + return nil, fmt.Errorf("failed to inspect packaged workflow %q for grader resources: %w", installable.SourcePath, err) + } + if graders == nil { + continue + } + grader := graders.Graders["operational-value"] + if grader == nil || grader.Run == "" { + continue + } + key := strings.ToLower(grader.Run) + if _, exists := seen[key]; exists { + continue + } + seen[key] = struct{}{} + resources = append(resources, resolvedPackageResource{ + SourcePath: grader.Run, + DestinationPath: grader.Run, + }) + } + return resources, nil +} + +func appendUniquePackageResources(resources, additional []resolvedPackageResource) []resolvedPackageResource { + seen := make(map[string]struct{}, len(resources)+len(additional)) + for _, resource := range resources { + seen[strings.ToLower(filepath.ToSlash(filepath.Clean(resource.DestinationPath)))] = struct{}{} + } + for _, resource := range additional { + key := strings.ToLower(filepath.ToSlash(filepath.Clean(resource.DestinationPath))) + if _, exists := seen[key]; exists { + continue + } + seen[key] = struct{}{} + resources = append(resources, resource) + } + return resources +} + func normalizeLocalPackageResourcePaths(resources []repositoryPackageResource, packageDir string) ([]resolvedPackageResource, error) { normalized := make([]resolvedPackageResource, 0, len(resources)) for _, resource := range resources { diff --git a/pkg/cli/add_package_manifest_test.go b/pkg/cli/add_package_manifest_test.go index 5753b4ae9eb..402d7516e18 100644 --- a/pkg/cli/add_package_manifest_test.go +++ b/pkg/cli/add_package_manifest_test.go @@ -122,6 +122,45 @@ resources: assert.Equal(t, "packages/repo-assist/policy/controls.json", pkg.ResourceFiles[1].SourcePath) assert.Equal(t, ".github/aw/policy/controls.json", pkg.ResourceFiles[1].DestinationPath) }) + t.Run("includes grader evaluator scripts as resources", func(t *testing.T) { + downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { + switch path { + case "aw.yml": + return []byte(`name: Grader Package +files: + - workflows/graded.md +`), nil + case "workflows/graded.md": + return []byte(`--- +on: workflow_dispatch +graders: + operational-value: + run: .github/graders/example-operational-value.sh +--- +# Graded workflow +`), nil + case "README.md": + return []byte("# Grader Package\n"), nil + default: + return nil, createRepositoryPackageNotFoundError(path) + } + } + listPackageWorkflowFilesForHost = func(_ context.Context, owner, repo, ref, workflowPath, host string) ([]string, error) { + t.Fatalf("unexpected scan of %s", workflowPath) + return nil, nil + } + + pkg, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "") + require.NoError(t, err) + require.Len(t, pkg.ResourceFiles, 1) + assert.Equal(t, ".github/graders/example-operational-value.sh", pkg.ResourceFiles[0].SourcePath) + assert.Equal(t, ".github/graders/example-operational-value.sh", pkg.ResourceFiles[0].DestinationPath) + + specs := appendRepositoryPackageWorkflowSpecs(nil, &RepoSpec{RepoSlug: "owner/repo"}, pkg) + require.Len(t, specs, 2) + assert.True(t, specs[1].IsPackageResourceFile) + assert.Equal(t, ".github/graders/example-operational-value.sh", specs[1].DestinationPath) + }) getRepositoryPackageLatestRelease = func(_ context.Context, repoSlug, host string) (string, error) { assert.Equal(t, "owner/repo", repoSlug) assert.Equal(t, "github.com", host) diff --git a/pkg/cli/update_manifest_test.go b/pkg/cli/update_manifest_test.go index c49f5b99264..4bf5ce06fc7 100644 --- a/pkg/cli/update_manifest_test.go +++ b/pkg/cli/update_manifest_test.go @@ -19,6 +19,44 @@ import ( "github.com/stretchr/testify/require" ) +func TestSyncManifestManagedResources_RestoresGraderEvaluator(t *testing.T) { + tmpDir := t.TempDir() + setupMinimalGitRepo(t, tmpDir) + t.Chdir(tmpDir) + + const evaluatorPath = ".github/graders/example-operational-value.sh" + oldContent := []byte("#!/usr/bin/env bash\necho old\n") + newContent := []byte("#!/usr/bin/env bash\necho new\n") + downloadContent := oldContent + + originalDownload := downloadPackageFileFromGitHubForHost + t.Cleanup(func() { downloadPackageFileFromGitHubForHost = originalDownload }) + downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { + require.Equal(t, evaluatorPath, path) + return downloadContent, nil + } + + pkg := &resolvedRepositoryPackage{ + ResourceFiles: []resolvedPackageResource{{ + SourcePath: evaluatorPath, + DestinationPath: evaluatorPath, + }}, + } + repoSpec := &RepoSpec{RepoSlug: "owner/repo"} + require.NoError(t, syncManifestManagedResources(context.Background(), repoSpec, pkg, "v1.0.0", UpdateWorkflowsOptions{})) + installedPath := filepath.Join(tmpDir, filepath.FromSlash(evaluatorPath)) + installed, err := os.ReadFile(installedPath) + require.NoError(t, err) + assert.Equal(t, oldContent, installed) + + require.NoError(t, os.Remove(installedPath)) + downloadContent = newContent + require.NoError(t, syncManifestManagedResources(context.Background(), repoSpec, pkg, "v2.0.0", UpdateWorkflowsOptions{})) + restored, err := os.ReadFile(installedPath) + require.NoError(t, err) + assert.Equal(t, newContent, restored) +} + func TestReconcileManifestManagedAssets_AddsPackageOwnedAssets(t *testing.T) { tmpDir := testutil.TempDir(t, "manifest-assets-*") require.NoError(t, os.Mkdir(filepath.Join(tmpDir, ".git"), 0o755)) From 047b52ff261bf6af4f06419014bb4f89709e502c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 06:53:12 +0000 Subject: [PATCH 03/13] Generalize grader resource discovery Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_package_manifest_resolve.go | 3 +-- pkg/cli/add_package_manifest_resources.go | 28 ++++++++++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/pkg/cli/add_package_manifest_resolve.go b/pkg/cli/add_package_manifest_resolve.go index acb25cb6004..a580abc4704 100644 --- a/pkg/cli/add_package_manifest_resolve.go +++ b/pkg/cli/add_package_manifest_resolve.go @@ -6,7 +6,6 @@ package cli import ( "context" "fmt" - "path" "strings" ) @@ -179,7 +178,7 @@ func newResolvedRepositoryPackage(manifestPath, ref, docsPath string, manifest * func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, packagePath, ref, host string) (string, []byte, error) { manifestPath := joinRepositoryPackagePath(packagePath, repositoryPackageManifestFileName) - repoSlug := path.Join(owner, repo) + repoSlug := strings.Join([]string{owner, repo}, "/") packageID := repositoryPackageIdentifier(repoSlug, packagePath) content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, manifestPath, ref, host) if err != nil { diff --git a/pkg/cli/add_package_manifest_resources.go b/pkg/cli/add_package_manifest_resources.go index d50cd6c6c0b..34c208a4f2d 100644 --- a/pkg/cli/add_package_manifest_resources.go +++ b/pkg/cli/add_package_manifest_resources.go @@ -6,6 +6,7 @@ import ( "fmt" "path" "path/filepath" + "sort" "strings" "github.com/github/gh-aw/pkg/constants" @@ -136,19 +137,24 @@ func resolveRepositoryPackageGraderResources(ctx context.Context, owner, repo, r if graders == nil { continue } - grader := graders.Graders["operational-value"] - if grader == nil || grader.Run == "" { - continue + var runPaths []string + for _, grader := range graders.Graders { + if grader != nil && grader.Run != "" { + runPaths = append(runPaths, grader.Run) + } } - key := strings.ToLower(grader.Run) - if _, exists := seen[key]; exists { - continue + sort.Strings(runPaths) + for _, runPath := range runPaths { + key := strings.ToLower(runPath) + if _, exists := seen[key]; exists { + continue + } + seen[key] = struct{}{} + resources = append(resources, resolvedPackageResource{ + SourcePath: runPath, + DestinationPath: runPath, + }) } - seen[key] = struct{}{} - resources = append(resources, resolvedPackageResource{ - SourcePath: grader.Run, - DestinationPath: grader.Run, - }) } return resources, nil } From 0480bdf4f55a4c5ff3fedfdef405e678a43703e7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 07:22:15 +0000 Subject: [PATCH 04/13] Reuse workflow resources for grader scripts Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_package_manifest_resolve.go | 7 +- pkg/cli/add_package_manifest_resources.go | 70 ------------------ pkg/cli/add_package_manifest_test.go | 39 ---------- pkg/cli/remote_workflow_test.go | 68 ++++++++++++++++++ pkg/cli/resources.go | 87 +++++++++++++++-------- pkg/cli/update_manifest_test.go | 38 ---------- 6 files changed, 128 insertions(+), 181 deletions(-) diff --git a/pkg/cli/add_package_manifest_resolve.go b/pkg/cli/add_package_manifest_resolve.go index a580abc4704..1f9faf0ba74 100644 --- a/pkg/cli/add_package_manifest_resolve.go +++ b/pkg/cli/add_package_manifest_resolve.go @@ -32,11 +32,6 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri return nil, err } resourceFiles := normalizePackageResourcePaths(manifest.Resources, packagePath) - graderResources, err := resolveRepositoryPackageGraderResources(ctx, owner, repo, ref, host, installationSources) - if err != nil { - return nil, err - } - resourceFiles = appendUniquePackageResources(resourceFiles, graderResources) docsPath, err := resolveRepositoryPackageDocsPath(ctx, owner, repo, packagePath, ref, host) if err != nil { @@ -178,7 +173,7 @@ func newResolvedRepositoryPackage(manifestPath, ref, docsPath string, manifest * func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, packagePath, ref, host string) (string, []byte, error) { manifestPath := joinRepositoryPackagePath(packagePath, repositoryPackageManifestFileName) - repoSlug := strings.Join([]string{owner, repo}, "/") + repoSlug := owner + "/" + repo packageID := repositoryPackageIdentifier(repoSlug, packagePath) content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, manifestPath, ref, host) if err != nil { diff --git a/pkg/cli/add_package_manifest_resources.go b/pkg/cli/add_package_manifest_resources.go index 34c208a4f2d..f4cd5970fc1 100644 --- a/pkg/cli/add_package_manifest_resources.go +++ b/pkg/cli/add_package_manifest_resources.go @@ -1,17 +1,13 @@ package cli import ( - "context" "errors" "fmt" "path" "path/filepath" - "sort" "strings" "github.com/github/gh-aw/pkg/constants" - "github.com/github/gh-aw/pkg/parser" - "github.com/github/gh-aw/pkg/workflow" ) type repositoryPackageResource struct { @@ -109,72 +105,6 @@ func normalizePackageResourcePaths(resources []repositoryPackageResource, packag return normalized } -func resolveRepositoryPackageGraderResources(ctx context.Context, owner, repo, ref, host string, installables []resolvedPackageInstallable) ([]resolvedPackageResource, error) { - var resources []resolvedPackageResource - seen := make(map[string]struct{}) - for _, installable := range installables { - if !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".md") { - continue - } - content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, installable.SourcePath, ref, host) - if err != nil { - if isRepositoryFileNotFound(err) { - continue - } - return nil, fmt.Errorf("failed to inspect packaged workflow %q for grader resources: %w", installable.SourcePath, err) - } - result, err := parser.ExtractFrontmatterFromContent(string(content)) - if err != nil { - return nil, fmt.Errorf("failed to inspect packaged workflow %q for grader resources: %w", installable.SourcePath, err) - } - if result.Frontmatter == nil { - continue - } - graders, err := workflow.ParseGradersFromFrontmatter(result.Frontmatter) - if err != nil { - return nil, fmt.Errorf("failed to inspect packaged workflow %q for grader resources: %w", installable.SourcePath, err) - } - if graders == nil { - continue - } - var runPaths []string - for _, grader := range graders.Graders { - if grader != nil && grader.Run != "" { - runPaths = append(runPaths, grader.Run) - } - } - sort.Strings(runPaths) - for _, runPath := range runPaths { - key := strings.ToLower(runPath) - if _, exists := seen[key]; exists { - continue - } - seen[key] = struct{}{} - resources = append(resources, resolvedPackageResource{ - SourcePath: runPath, - DestinationPath: runPath, - }) - } - } - return resources, nil -} - -func appendUniquePackageResources(resources, additional []resolvedPackageResource) []resolvedPackageResource { - seen := make(map[string]struct{}, len(resources)+len(additional)) - for _, resource := range resources { - seen[strings.ToLower(filepath.ToSlash(filepath.Clean(resource.DestinationPath)))] = struct{}{} - } - for _, resource := range additional { - key := strings.ToLower(filepath.ToSlash(filepath.Clean(resource.DestinationPath))) - if _, exists := seen[key]; exists { - continue - } - seen[key] = struct{}{} - resources = append(resources, resource) - } - return resources -} - func normalizeLocalPackageResourcePaths(resources []repositoryPackageResource, packageDir string) ([]resolvedPackageResource, error) { normalized := make([]resolvedPackageResource, 0, len(resources)) for _, resource := range resources { diff --git a/pkg/cli/add_package_manifest_test.go b/pkg/cli/add_package_manifest_test.go index 402d7516e18..5753b4ae9eb 100644 --- a/pkg/cli/add_package_manifest_test.go +++ b/pkg/cli/add_package_manifest_test.go @@ -122,45 +122,6 @@ resources: assert.Equal(t, "packages/repo-assist/policy/controls.json", pkg.ResourceFiles[1].SourcePath) assert.Equal(t, ".github/aw/policy/controls.json", pkg.ResourceFiles[1].DestinationPath) }) - t.Run("includes grader evaluator scripts as resources", func(t *testing.T) { - downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { - switch path { - case "aw.yml": - return []byte(`name: Grader Package -files: - - workflows/graded.md -`), nil - case "workflows/graded.md": - return []byte(`--- -on: workflow_dispatch -graders: - operational-value: - run: .github/graders/example-operational-value.sh ---- -# Graded workflow -`), nil - case "README.md": - return []byte("# Grader Package\n"), nil - default: - return nil, createRepositoryPackageNotFoundError(path) - } - } - listPackageWorkflowFilesForHost = func(_ context.Context, owner, repo, ref, workflowPath, host string) ([]string, error) { - t.Fatalf("unexpected scan of %s", workflowPath) - return nil, nil - } - - pkg, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "") - require.NoError(t, err) - require.Len(t, pkg.ResourceFiles, 1) - assert.Equal(t, ".github/graders/example-operational-value.sh", pkg.ResourceFiles[0].SourcePath) - assert.Equal(t, ".github/graders/example-operational-value.sh", pkg.ResourceFiles[0].DestinationPath) - - specs := appendRepositoryPackageWorkflowSpecs(nil, &RepoSpec{RepoSlug: "owner/repo"}, pkg) - require.Len(t, specs, 2) - assert.True(t, specs[1].IsPackageResourceFile) - assert.Equal(t, ".github/graders/example-operational-value.sh", specs[1].DestinationPath) - }) getRepositoryPackageLatestRelease = func(_ context.Context, repoSlug, host string) (string, error) { assert.Equal(t, "owner/repo", repoSlug) assert.Equal(t, "github.com", host) diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index fb34a0442a9..00484dec61c 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/github/gh-aw/pkg/workflow" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1802,6 +1803,73 @@ resources: assert.Equal(t, []string{"triage-issue.md", "close-stale.md", "my-action.yml"}, resources, "should extract all listed resources") } +func TestExtractResources_IncludesGraderEvaluator(t *testing.T) { + content := `--- +engine: copilot +on: issues +resources: + - .github/graders/example-operational-value.sh +graders: + operational-value: + run: .github/graders/example-operational-value.sh +--- + +# Workflow +` + resources, err := extractResources(content) + require.NoError(t, err) + assert.Equal(t, []string{".github/graders/example-operational-value.sh"}, resources) +} + +func TestFetchAndSaveRemoteResources_InstallsAndRestoresGraderEvaluator(t *testing.T) { + tmpDir := t.TempDir() + setupMinimalGitRepo(t, tmpDir) + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) + + const evaluatorPath = ".github/graders/example-operational-value.sh" + content := `--- +on: workflow_dispatch +graders: + operational-value: + run: .github/graders/example-operational-value.sh +--- + +# Workflow +` + evaluatorContent := []byte("#!/usr/bin/env bash\necho old\n") + originalDownload := downloadResourceFileFromGitHub + t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) + downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { + assert.Equal(t, evaluatorPath, filePath) + return evaluatorContent, nil + } + + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{RepoSlug: "owner/repo", Version: "main"}, + WorkflowPath: "workflows/graded.md", + } + require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) + + installedPath := filepath.Join(tmpDir, filepath.FromSlash(evaluatorPath)) + installed, err := os.ReadFile(installedPath) + require.NoError(t, err) + assert.Equal(t, evaluatorContent, installed) + + workflowPath := filepath.Join(workflowsDir, "graded.md") + require.NoError(t, os.WriteFile(workflowPath, []byte(content), 0o644)) + compiler := workflow.NewCompiler() + compiler.SetNoEmit(true) + require.NoError(t, compiler.CompileWorkflow(workflowPath)) + + require.NoError(t, os.Remove(installedPath)) + evaluatorContent = []byte("#!/usr/bin/env bash\necho new\n") + require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, true, nil)) + restored, err := os.ReadFile(installedPath) + require.NoError(t, err) + assert.Equal(t, evaluatorContent, restored) +} + // TestExtractResources_MacroRejected verifies that an entry with GitHub Actions expression syntax causes an error. func TestExtractResources_MacroRejected(t *testing.T) { content := `--- diff --git a/pkg/cli/resources.go b/pkg/cli/resources.go index 8611500a86f..f538759e817 100644 --- a/pkg/cli/resources.go +++ b/pkg/cli/resources.go @@ -10,12 +10,17 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/fileutil" + "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/workflow" ) -// extractResources extracts file paths from the top-level "resources" frontmatter field. +var downloadResourceFileFromGitHub = parser.DownloadFileFromGitHub + +// extractResources extracts file paths from the top-level "resources" frontmatter field +// and validated grader evaluator paths. // Returns an error if any entry contains GitHub Actions expression syntax (e.g. "${{"), // since macros are not permitted in resource paths. func extractResources(content string) ([]string, error) { @@ -28,31 +33,47 @@ func extractResources(content string) ([]string, error) { return nil, nil } - resourcesField, exists := result.Frontmatter["resources"] - if !exists { - return nil, nil + var paths []string + if resourcesField, exists := result.Frontmatter["resources"]; exists { + switch v := resourcesField.(type) { + case []any: + for _, item := range v { + if s, ok := item.(string); ok { + paths = append(paths, s) + } + } + case []string: + paths = append(paths, v...) + } } - var paths []string - switch v := resourcesField.(type) { - case []any: - for _, item := range v { - if s, ok := item.(string); ok { - paths = append(paths, s) + graders, err := workflow.ParseGradersFromFrontmatter(result.Frontmatter) + if err != nil { + return nil, err + } + if graders != nil { + for _, grader := range graders.Graders { + if grader != nil && (grader.Enabled == nil || *grader.Enabled) && grader.Run != "" { + paths = append(paths, grader.Run) } } - case []string: - paths = v } // Reject entries that contain GitHub Actions expression syntax — macros are not allowed. + unique := make([]string, 0, len(paths)) + seen := make(map[string]struct{}, len(paths)) for _, p := range paths { if strings.Contains(p, "${{") { return nil, fmt.Errorf("resources entry %q contains GitHub Actions expression syntax (${{) which is not allowed; use static paths only", p) } + if _, exists := seen[p]; exists { + continue + } + seen[p] = struct{}{} + unique = append(unique, p) } - return paths, nil + return unique, nil } // fetchAndSaveRemoteResources fetches files listed in the top-level "resources" frontmatter @@ -67,7 +88,7 @@ func extractResources(content string) ([]string, error) { // from the same source are silently skipped. // For non-Markdown resource files: if the target already exists and force is false, an error // is returned regardless of origin (non-markdown files have no source tracking). -func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { +func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { //nolint:largefunc // Keep resource conflict, download, and tracking behavior together. if spec.RepoSlug == "" { return nil } @@ -100,13 +121,6 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work // Resources are resolved relative to the source workflow's directory in the remote repo. workflowBaseDir := getParentDir(spec.WorkflowPath) - // Pre-compute the absolute target directory for path-traversal boundary checks. - absTargetDir, err := filepath.Abs(targetDir) - if err != nil { - remoteWorkflowLog.Printf("Failed to resolve absolute path for target directory %s: %v", targetDir, err) - return nil - } - for _, resourcePath := range resourcePaths { // Early rejection of path traversal patterns. This is a fast first-pass check; // the filepath.Rel boundary check below is the authoritative security control. @@ -117,9 +131,13 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work continue } - // Resolve the remote file path + // Resolve the remote file path. Grader evaluators are repository-relative; + // ordinary resources remain relative to the source workflow directory. var remoteFilePath string - if rest, ok := strings.CutPrefix(resourcePath, "/"); ok { + isGraderEvaluator := strings.HasPrefix(resourcePath, constants.GithubDir+"graders/") + if isGraderEvaluator { + remoteFilePath = resourcePath + } else if rest, ok := strings.CutPrefix(resourcePath, "/"); ok { remoteFilePath = rest } else if workflowBaseDir != "" { remoteFilePath = path.Join(workflowBaseDir, resourcePath) @@ -138,15 +156,28 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work if localRelPath == "" || localRelPath == "." { continue } - targetPath := filepath.Join(targetDir, localRelPath) + targetBaseDir := targetDir + if isGraderEvaluator { + targetBaseDir, err = gitutil.FindGitRootFrom(targetDir) + if err != nil { + return fmt.Errorf("failed to resolve repository root for grader resource %q: %w", resourcePath, err) + } + localRelPath = filepath.FromSlash(resourcePath) + } + targetPath := filepath.Join(targetBaseDir, localRelPath) - // Belt-and-suspenders: verify the resolved path stays inside targetDir + // Belt-and-suspenders: verify the resolved path stays inside its target base. + absTargetBase, absErr := filepath.Abs(targetBaseDir) + if absErr != nil { + remoteWorkflowLog.Printf("Failed to resolve absolute resource target directory %s: %v", targetBaseDir, absErr) + continue + } absTargetPath, absErr := filepath.Abs(targetPath) if absErr != nil { remoteWorkflowLog.Printf("Failed to resolve absolute path for resource %s: %v", resourcePath, absErr) continue } - if rel, relErr := filepath.Rel(absTargetDir, absTargetPath); relErr != nil || strings.HasPrefix(rel, "..") { + if rel, relErr := filepath.Rel(absTargetBase, absTargetPath); relErr != nil || strings.HasPrefix(rel, "..") { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Refusing to write resource outside target directory: %q", resourcePath))) } @@ -182,7 +213,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work } // Download from source repository - fileContent, err := parser.DownloadFileFromGitHub(ctx, owner, repo, remoteFilePath, ref) + fileContent, err := downloadResourceFileFromGitHub(ctx, owner, repo, remoteFilePath, ref) if err != nil { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch resource %s: %v", remoteFilePath, err))) @@ -192,7 +223,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work // For markdown resources, embed the source field for future conflict detection. if strings.HasSuffix(strings.ToLower(remoteFilePath), ".md") { - depSourceString := spec.RepoSlug + "/" + remoteFilePath + "@" + ref + depSourceString := path.Join(spec.RepoSlug, remoteFilePath) + "@" + ref if updated, srcErr := addSourceToWorkflow(string(fileContent), depSourceString); srcErr == nil { fileContent = []byte(updated) } diff --git a/pkg/cli/update_manifest_test.go b/pkg/cli/update_manifest_test.go index 4bf5ce06fc7..c49f5b99264 100644 --- a/pkg/cli/update_manifest_test.go +++ b/pkg/cli/update_manifest_test.go @@ -19,44 +19,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestSyncManifestManagedResources_RestoresGraderEvaluator(t *testing.T) { - tmpDir := t.TempDir() - setupMinimalGitRepo(t, tmpDir) - t.Chdir(tmpDir) - - const evaluatorPath = ".github/graders/example-operational-value.sh" - oldContent := []byte("#!/usr/bin/env bash\necho old\n") - newContent := []byte("#!/usr/bin/env bash\necho new\n") - downloadContent := oldContent - - originalDownload := downloadPackageFileFromGitHubForHost - t.Cleanup(func() { downloadPackageFileFromGitHubForHost = originalDownload }) - downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { - require.Equal(t, evaluatorPath, path) - return downloadContent, nil - } - - pkg := &resolvedRepositoryPackage{ - ResourceFiles: []resolvedPackageResource{{ - SourcePath: evaluatorPath, - DestinationPath: evaluatorPath, - }}, - } - repoSpec := &RepoSpec{RepoSlug: "owner/repo"} - require.NoError(t, syncManifestManagedResources(context.Background(), repoSpec, pkg, "v1.0.0", UpdateWorkflowsOptions{})) - installedPath := filepath.Join(tmpDir, filepath.FromSlash(evaluatorPath)) - installed, err := os.ReadFile(installedPath) - require.NoError(t, err) - assert.Equal(t, oldContent, installed) - - require.NoError(t, os.Remove(installedPath)) - downloadContent = newContent - require.NoError(t, syncManifestManagedResources(context.Background(), repoSpec, pkg, "v2.0.0", UpdateWorkflowsOptions{})) - restored, err := os.ReadFile(installedPath) - require.NoError(t, err) - assert.Equal(t, newContent, restored) -} - func TestReconcileManifestManagedAssets_AddsPackageOwnedAssets(t *testing.T) { tmpDir := testutil.TempDir(t, "manifest-assets-*") require.NoError(t, os.Mkdir(filepath.Join(tmpDir, ".git"), 0o755)) From 1e2d4dca07a7e311d35ccc2e88c9bf90b397878d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 07:25:41 +0000 Subject: [PATCH 05/13] Deduplicate shared grader evaluator resources Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/remote_workflow_test.go | 1 + pkg/cli/resources.go | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 00484dec61c..85a71671686 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -1855,6 +1855,7 @@ graders: installed, err := os.ReadFile(installedPath) require.NoError(t, err) assert.Equal(t, evaluatorContent, installed) + require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) workflowPath := filepath.Join(workflowsDir, "graded.md") require.NoError(t, os.WriteFile(workflowPath, []byte(content), 0o644)) diff --git a/pkg/cli/resources.go b/pkg/cli/resources.go index f538759e817..f51eadb2212 100644 --- a/pkg/cli/resources.go +++ b/pkg/cli/resources.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "context" "fmt" "os" @@ -188,7 +189,9 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work fileExists := false if fileutil.FileExists(targetPath) { fileExists = true - if !force { + // Shared evaluators may be referenced by multiple workflows in one package. + // Their conflict handling is deferred until the source content can be compared. + if !force && !isGraderEvaluator { isMarkdown := strings.HasSuffix(strings.ToLower(targetPath), ".md") if isMarkdown { // For markdown files, allow same-source overwrites. @@ -220,6 +223,16 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work } continue } + if fileExists && !force && isGraderEvaluator { + existingContent, err := os.ReadFile(targetPath) + if err != nil { + return fmt.Errorf("failed to read existing grader resource %q: %w", targetPath, err) + } + if bytes.Equal(existingContent, fileContent) { + continue + } + return fmt.Errorf("resource %q already exists at %s; remove the file or use --force to overwrite", resourcePath, targetPath) + } // For markdown resources, embed the source field for future conflict detection. if strings.HasSuffix(strings.ToLower(remoteFilePath), ".md") { From 37675b87f1cc6b61e9f6ed1ef0af5815b10e51c4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 07:29:18 +0000 Subject: [PATCH 06/13] Include every declared grader evaluator Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/remote_workflow_test.go | 18 ++++++++++++++++++ pkg/cli/resources.go | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 85a71671686..9930bf1236d 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -1821,6 +1821,24 @@ graders: assert.Equal(t, []string{".github/graders/example-operational-value.sh"}, resources) } +func TestExtractResources_IncludesDisabledGraderEvaluator(t *testing.T) { + content := `--- +on: issues +graders: + operational-value: + enabled: false + run: .github/graders/example-operational-value.sh + retries: + enabled: true +--- + +# Workflow +` + resources, err := extractResources(content) + require.NoError(t, err) + assert.Equal(t, []string{".github/graders/example-operational-value.sh"}, resources) +} + func TestFetchAndSaveRemoteResources_InstallsAndRestoresGraderEvaluator(t *testing.T) { tmpDir := t.TempDir() setupMinimalGitRepo(t, tmpDir) diff --git a/pkg/cli/resources.go b/pkg/cli/resources.go index f51eadb2212..3be881cfd12 100644 --- a/pkg/cli/resources.go +++ b/pkg/cli/resources.go @@ -54,7 +54,7 @@ func extractResources(content string) ([]string, error) { } if graders != nil { for _, grader := range graders.Graders { - if grader != nil && (grader.Enabled == nil || *grader.Enabled) && grader.Run != "" { + if grader != nil && grader.Run != "" { paths = append(paths, grader.Run) } } From d6c2111ceeae79ad2fc685331ff0eb892792cbb9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:35:02 +0000 Subject: [PATCH 07/13] docs(adr): draft ADR-56268 for grader evaluator script packaging Adds a design decision record explaining the decision to extend the workflow package resource resolver to automatically include grader evaluator scripts referenced in graders.*.run frontmatter. Co-Authored-By: Claude Sonnet 4.6 --- ...-evaluator-scripts-in-workflow-packages.md | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 docs/adr/56268-include-grader-evaluator-scripts-in-workflow-packages.md diff --git a/docs/adr/56268-include-grader-evaluator-scripts-in-workflow-packages.md b/docs/adr/56268-include-grader-evaluator-scripts-in-workflow-packages.md new file mode 100644 index 00000000000..05eeae119c4 --- /dev/null +++ b/docs/adr/56268-include-grader-evaluator-scripts-in-workflow-packages.md @@ -0,0 +1,45 @@ +# ADR-56268: Include Grader Evaluator Scripts in Workflow Packages + +**Date**: 2026-08-27 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +When users install a packaged workflow via `gh aw` that declares grader evaluators under `graders.*.run`, the evaluator shell scripts referenced by those keys were not co-installed alongside the workflow files. This caused workflow compilation to fail immediately in clean repositories because the compiler expects those scripts to be present. The root cause was that the package resource resolver (`extractResources`) only scanned the `resources:` frontmatter field and had no awareness of the `graders:` stanza. Evaluator scripts live at repository-root-relative paths (under `.github/graders/`) rather than relative to the workflow file, and the same evaluator may be shared across multiple workflows in one package. + +### Decision + +We will extend `extractResources` to also parse grader frontmatter via `workflow.ParseGradersFromFrontmatter` and append each non-empty `run` path to the resource list, then deduplicate the combined set before returning it. Within `fetchAndSaveRemoteResources` we will introduce an `isGraderEvaluator` branch that (a) resolves evaluator paths repository-root-relative rather than workflow-directory-relative, (b) installs them relative to the git repository root rather than the workflows target directory, and (c) allows silent no-op re-installs when the existing file content matches the incoming download, blocking only on content divergence. This routes evaluator scripts through the existing resource installation and ownership infrastructure so that `gh aw update` can restore or update them without additional code paths. + +### Alternatives Considered + +#### Alternative 1: Require explicit listing in `resources:` field + +Workflow authors would be required to manually duplicate the evaluator path in both `graders.*.run` and `resources:`. This was the implicit status-quo before this fix. It is rejected because it creates a footgun: authors writing a `graders:` stanza naturally expect the referenced script to be packaged, and the silent omission produces a compilation failure that is hard to diagnose in clean repositories. + +#### Alternative 2: Make missing evaluators a non-fatal warning at compile time + +The compiler could treat a missing evaluator as a warning rather than an error, allowing workflows to install without the script. This is rejected because evaluators are required for grader execution — silencing the error would allow users to install broken workflows that fail at runtime rather than at installation/compilation time, which is a worse developer experience. + +### Consequences + +#### Positive +- Workflows with grader evaluators install successfully in clean repositories without any manual `resources:` duplication. +- `gh aw update` automatically restores or updates missing evaluator scripts using the existing resource lifecycle. +- Evaluator scripts shared across multiple packaged workflows are deduplicated, avoiding redundant downloads. + +#### Negative +- `fetchAndSaveRemoteResources` now contains two distinct path-resolution conventions (workflow-dir-relative for ordinary resources, repo-root-relative for grader evaluators), increasing the function's complexity. A `//nolint:largefunc` suppression is needed. +- Content-based conflict detection for grader evaluators (byte-by-byte comparison before allowing overwrite) is deferred until after the download, adding a network round-trip even when the file would ultimately be skipped. Ordinary resources use a pre-download existence check. +- The `isGraderEvaluator` heuristic relies on the `.github/graders/` path prefix convention; evaluators stored elsewhere would not be detected and would still require manual `resources:` listing. + +#### Neutral +- The `downloadResourceFileFromGitHub` function is extracted as a package-level variable to allow test injection, which is a minor testability refactor with no production behavior change. +- The `absTargetDir` pre-computation is moved from the function entry point into the per-resource loop body to support per-resource target base switching. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From a1405fb43e2cb8f06757299aca405974845ce1ad Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:30:32 +0000 Subject: [PATCH 08/13] Relax grader evaluator path handling Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- ...pile_pipeline_shellcheck_resources_test.go | 6 +- pkg/cli/remote_workflow_test.go | 85 +++++++++++++++-- pkg/cli/resources.go | 92 +++++++++++-------- pkg/parser/schema_test.go | 27 ++++-- pkg/parser/schemas/main_workflow_schema.json | 4 +- pkg/workflow/graders_config.go | 28 +++--- pkg/workflow/graders_config_test.go | 72 ++++++++------- pkg/workflow/graders_operational_value.go | 14 ++- .../graders_operational_value_test.go | 40 ++++++-- 9 files changed, 255 insertions(+), 113 deletions(-) diff --git a/pkg/cli/compile_pipeline_shellcheck_resources_test.go b/pkg/cli/compile_pipeline_shellcheck_resources_test.go index dd302d72b71..fda5ce5bcf1 100644 --- a/pkg/cli/compile_pipeline_shellcheck_resources_test.go +++ b/pkg/cli/compile_pipeline_shellcheck_resources_test.go @@ -21,7 +21,7 @@ func TestCompileWorkflows_ShellScriptResourcesIncludeOperationalValueGrader(t *t tmpDir := t.TempDir() require.NoError(t, initTestGitRepo(tmpDir)) - gradersDir := filepath.Join(tmpDir, ".github", "graders") + gradersDir := filepath.Join(tmpDir, ".github", "workflows", "graders") require.NoError(t, os.MkdirAll(gradersDir, 0o755)) evaluatorScript := "#!/usr/bin/env bash\nset -euo pipefail\necho '{}'\n" @@ -45,7 +45,7 @@ mcp-scripts: git status --short graders: operational-value: - run: .github/graders/example-operational-value.sh + run: ./graders/example-operational-value.sh --- # Test Workflow @@ -78,7 +78,7 @@ This is a test workflow exercising frontmatter shell script resources. case "graders.operational-value": gradersFound = true assert.Equal(t, evaluatorScript, resource.Script) - assert.Equal(t, ".github/graders/example-operational-value.sh", resource.Source) + assert.Equal(t, "./graders/example-operational-value.sh", resource.Source) assert.Equal(t, "bash", resource.Shell) case "mcp-scripts.inspect": mcpScriptsFound = true diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 9930bf1236d..5686961bbb5 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "testing" "time" @@ -1808,17 +1809,17 @@ func TestExtractResources_IncludesGraderEvaluator(t *testing.T) { engine: copilot on: issues resources: - - .github/graders/example-operational-value.sh + - .github/workflows/graders/example-operational-value.sh graders: operational-value: - run: .github/graders/example-operational-value.sh + run: .github/workflows/graders/example-operational-value.sh --- # Workflow ` resources, err := extractResources(content) require.NoError(t, err) - assert.Equal(t, []string{".github/graders/example-operational-value.sh"}, resources) + assert.Equal(t, []string{".github/workflows/graders/example-operational-value.sh"}, resources) } func TestExtractResources_IncludesDisabledGraderEvaluator(t *testing.T) { @@ -1827,7 +1828,7 @@ on: issues graders: operational-value: enabled: false - run: .github/graders/example-operational-value.sh + run: ./graders/example-operational-value.sh retries: enabled: true --- @@ -1836,7 +1837,7 @@ graders: ` resources, err := extractResources(content) require.NoError(t, err) - assert.Equal(t, []string{".github/graders/example-operational-value.sh"}, resources) + assert.Equal(t, []string{"./graders/example-operational-value.sh"}, resources) } func TestFetchAndSaveRemoteResources_InstallsAndRestoresGraderEvaluator(t *testing.T) { @@ -1845,12 +1846,12 @@ func TestFetchAndSaveRemoteResources_InstallsAndRestoresGraderEvaluator(t *testi workflowsDir := filepath.Join(tmpDir, ".github", "workflows") require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) - const evaluatorPath = ".github/graders/example-operational-value.sh" + const evaluatorPath = ".github/workflows/graders/example-operational-value.sh" content := `--- on: workflow_dispatch graders: operational-value: - run: .github/graders/example-operational-value.sh + run: .github/workflows/graders/example-operational-value.sh --- # Workflow @@ -1859,7 +1860,7 @@ graders: originalDownload := downloadResourceFileFromGitHub t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { - assert.Equal(t, evaluatorPath, filePath) + assert.Equal(t, "workflows/graders/example-operational-value.sh", filePath) return evaluatorContent, nil } @@ -1889,6 +1890,74 @@ graders: assert.Equal(t, evaluatorContent, restored) } +func TestFetchAndSaveRemoteResources_InstallsLocalDotGraderEvaluator(t *testing.T) { + tmpDir := t.TempDir() + setupMinimalGitRepo(t, tmpDir) + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) + + content := `--- +on: workflow_dispatch +graders: + operational-value: + run: ./scripts/example-operational-value.sh +--- + +# Workflow +` + evaluatorContent := []byte("#!/usr/bin/env bash\necho local\n") + originalDownload := downloadResourceFileFromGitHub + t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) + downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { + assert.Equal(t, "workflows/scripts/example-operational-value.sh", filePath) + return evaluatorContent, nil + } + + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{RepoSlug: "owner/repo", Version: "main"}, + WorkflowPath: "workflows/graded.md", + } + require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) + + installed, err := os.ReadFile(filepath.Join(workflowsDir, "scripts", "example-operational-value.sh")) + require.NoError(t, err) + assert.Equal(t, evaluatorContent, installed) +} + +func TestFetchAndSaveRemoteResources_RejectsGraderEvaluatorSymlinkedParent(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation requires additional privileges on Windows") + } + tmpDir := t.TempDir() + setupMinimalGitRepo(t, tmpDir) + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) + outsideDir := t.TempDir() + require.NoError(t, os.Symlink(outsideDir, filepath.Join(workflowsDir, "scripts"))) + + content := `--- +on: workflow_dispatch +graders: + operational-value: + run: ./scripts/example-operational-value.sh +--- + +# Workflow +` + originalDownload := downloadResourceFileFromGitHub + t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) + downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { + return []byte("#!/usr/bin/env bash\necho unsafe\n"), nil + } + + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{RepoSlug: "owner/repo", Version: "main"}, + WorkflowPath: "workflows/graded.md", + } + require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, true, nil)) + assert.NoFileExists(t, filepath.Join(outsideDir, "example-operational-value.sh")) +} + // TestExtractResources_MacroRejected verifies that an entry with GitHub Actions expression syntax causes an error. func TestExtractResources_MacroRejected(t *testing.T) { content := `--- diff --git a/pkg/cli/resources.go b/pkg/cli/resources.go index 3be881cfd12..84856d0212f 100644 --- a/pkg/cli/resources.go +++ b/pkg/cli/resources.go @@ -20,11 +20,16 @@ import ( var downloadResourceFileFromGitHub = parser.DownloadFileFromGitHub -// extractResources extracts file paths from the top-level "resources" frontmatter field -// and validated grader evaluator paths. +type extractedResource struct { + path string + isGraderEvaluator bool +} + +// extractResourceEntries extracts file paths from the top-level "resources" frontmatter +// field and validated grader evaluator paths. // Returns an error if any entry contains GitHub Actions expression syntax (e.g. "${{"), // since macros are not permitted in resource paths. -func extractResources(content string) ([]string, error) { +func extractResourceEntries(content string) ([]extractedResource, error) { result, err := parser.ExtractFrontmatterFromContent(content) if err != nil { remoteWorkflowLog.Printf("Failed to extract frontmatter for resources: %v", err) @@ -34,17 +39,19 @@ func extractResources(content string) ([]string, error) { return nil, nil } - var paths []string + var resources []extractedResource if resourcesField, exists := result.Frontmatter["resources"]; exists { switch v := resourcesField.(type) { case []any: for _, item := range v { if s, ok := item.(string); ok { - paths = append(paths, s) + resources = append(resources, extractedResource{path: s}) } } case []string: - paths = append(paths, v...) + for _, s := range v { + resources = append(resources, extractedResource{path: s}) + } } } @@ -55,28 +62,43 @@ func extractResources(content string) ([]string, error) { if graders != nil { for _, grader := range graders.Graders { if grader != nil && grader.Run != "" { - paths = append(paths, grader.Run) + resources = append(resources, extractedResource{path: grader.Run, isGraderEvaluator: true}) } } } // Reject entries that contain GitHub Actions expression syntax — macros are not allowed. - unique := make([]string, 0, len(paths)) - seen := make(map[string]struct{}, len(paths)) - for _, p := range paths { + unique := make([]extractedResource, 0, len(resources)) + seen := make(map[string]int, len(resources)) + for _, resource := range resources { + p := resource.path if strings.Contains(p, "${{") { return nil, fmt.Errorf("resources entry %q contains GitHub Actions expression syntax (${{) which is not allowed; use static paths only", p) } - if _, exists := seen[p]; exists { + if existingIndex, exists := seen[p]; exists { + unique[existingIndex].isGraderEvaluator = unique[existingIndex].isGraderEvaluator || resource.isGraderEvaluator continue } - seen[p] = struct{}{} - unique = append(unique, p) + seen[p] = len(unique) + unique = append(unique, resource) } return unique, nil } +// extractResources returns the extracted resource paths in declaration order. +func extractResources(content string) ([]string, error) { + entries, err := extractResourceEntries(content) + if err != nil { + return nil, err + } + paths := make([]string, 0, len(entries)) + for _, entry := range entries { + paths = append(paths, entry.path) + } + return paths, nil +} + // fetchAndSaveRemoteResources fetches files listed in the top-level "resources" frontmatter // field from the same remote repository and saves them locally. Resources are resolved as // relative paths from the same directory as the source workflow in the remote repo. @@ -111,7 +133,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work spec.Version = ref } - resourcePaths, err := extractResources(content) + resourcePaths, err := extractResourceEntries(content) if err != nil { return err } @@ -122,9 +144,10 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work // Resources are resolved relative to the source workflow's directory in the remote repo. workflowBaseDir := getParentDir(spec.WorkflowPath) - for _, resourcePath := range resourcePaths { + for _, resource := range resourcePaths { + resourcePath := resource.path // Early rejection of path traversal patterns. This is a fast first-pass check; - // the filepath.Rel boundary check below is the authoritative security control. + // the symlink-aware path validation below is the authoritative security control. if strings.Contains(resourcePath, "..") { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Skipping resource with unsafe path: %q", resourcePath))) @@ -132,18 +155,24 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work continue } - // Resolve the remote file path. Grader evaluators are repository-relative; - // ordinary resources remain relative to the source workflow directory. + // Resolve the remote file path. Explicitly local grader evaluators follow ordinary + // resource behavior; workspace-relative grader evaluators install at their exact + // repository-relative run path. var remoteFilePath string - isGraderEvaluator := strings.HasPrefix(resourcePath, constants.GithubDir+"graders/") - if isGraderEvaluator { + isWorkspaceRelativeGraderEvaluator := resource.isGraderEvaluator && !strings.HasPrefix(resourcePath, "./") + if isWorkspaceRelativeGraderEvaluator { remoteFilePath = resourcePath + if strings.HasPrefix(remoteFilePath, constants.WorkflowsDirSlash) && workflowBaseDir != "" { + remoteFilePath = path.Join(workflowBaseDir, strings.TrimPrefix(remoteFilePath, constants.WorkflowsDirSlash)) + } else if spec.PackagePath != "" { + remoteFilePath = joinRepositoryPackagePath(spec.PackagePath, remoteFilePath) + } } else if rest, ok := strings.CutPrefix(resourcePath, "/"); ok { remoteFilePath = rest } else if workflowBaseDir != "" { - remoteFilePath = path.Join(workflowBaseDir, resourcePath) + remoteFilePath = path.Join(workflowBaseDir, strings.TrimPrefix(resourcePath, "./")) } else { - remoteFilePath = resourcePath + remoteFilePath = strings.TrimPrefix(resourcePath, "./") } remoteFilePath = path.Clean(remoteFilePath) @@ -158,7 +187,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work continue } targetBaseDir := targetDir - if isGraderEvaluator { + if isWorkspaceRelativeGraderEvaluator { targetBaseDir, err = gitutil.FindGitRootFrom(targetDir) if err != nil { return fmt.Errorf("failed to resolve repository root for grader resource %q: %w", resourcePath, err) @@ -167,18 +196,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work } targetPath := filepath.Join(targetBaseDir, localRelPath) - // Belt-and-suspenders: verify the resolved path stays inside its target base. - absTargetBase, absErr := filepath.Abs(targetBaseDir) - if absErr != nil { - remoteWorkflowLog.Printf("Failed to resolve absolute resource target directory %s: %v", targetBaseDir, absErr) - continue - } - absTargetPath, absErr := filepath.Abs(targetPath) - if absErr != nil { - remoteWorkflowLog.Printf("Failed to resolve absolute path for resource %s: %v", resourcePath, absErr) - continue - } - if rel, relErr := filepath.Rel(absTargetBase, absTargetPath); relErr != nil || strings.HasPrefix(rel, "..") { + if err := fileutil.ValidatePathWithinBase(targetBaseDir, targetPath); err != nil { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Refusing to write resource outside target directory: %q", resourcePath))) } @@ -191,7 +209,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work fileExists = true // Shared evaluators may be referenced by multiple workflows in one package. // Their conflict handling is deferred until the source content can be compared. - if !force && !isGraderEvaluator { + if !force && !resource.isGraderEvaluator { isMarkdown := strings.HasSuffix(strings.ToLower(targetPath), ".md") if isMarkdown { // For markdown files, allow same-source overwrites. @@ -223,7 +241,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work } continue } - if fileExists && !force && isGraderEvaluator { + if fileExists && !force && resource.isGraderEvaluator { existingContent, err := os.ReadFile(targetPath) if err != nil { return fmt.Errorf("failed to read existing grader resource %q: %w", targetPath, err) diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index 40748f9b058..e26bbec976d 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -1124,17 +1124,24 @@ func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_SandboxAgentPlatfo func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_OperationalValueGrader(t *testing.T) { t.Parallel() - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "graders": map[string]any{ - "operational-value": map[string]any{ - "run": ".github/graders/example-operational-value.sh", - }, - }, - } + for _, runPath := range []string{ + ".github/workflows/graders/example-operational-value.sh", + "./graders/example-operational-value.sh", + } { + t.Run(runPath, func(t *testing.T) { + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "graders": map[string]any{ + "operational-value": map[string]any{ + "run": runPath, + }, + }, + } - if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/operational-value-grader-test.md"); err != nil { - t.Fatalf("expected operational-value evaluator to pass schema validation, got: %v", err) + if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/operational-value-grader-test.md"); err != nil { + t.Fatalf("expected operational-value evaluator to pass schema validation, got: %v", err) + } + }) } } diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 6f879808cd1..3199c574a5f 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12898,8 +12898,8 @@ }, "run": { "type": "string", - "pattern": "^\\.github/graders/.+\\.sh$", - "description": "Repository-relative Bash script for the operational-value evaluator. Supported only for the reserved operational-value grader ID." + "pattern": "^(?:\\./)?(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*|\\.\\.[^/\\\\]+)(?:/(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*|\\.\\.[^/\\\\]+))*\\.sh$", + "description": "Workspace-relative or ./ workflow-local Bash script for the operational-value evaluator. Supported only for the reserved operational-value grader ID." } } } diff --git a/pkg/workflow/graders_config.go b/pkg/workflow/graders_config.go index d01cee44ba7..94b49d9c95f 100644 --- a/pkg/workflow/graders_config.go +++ b/pkg/workflow/graders_config.go @@ -74,7 +74,7 @@ type GraderDefinition struct { Threshold *float64 // quality threshold (pass/fail boundary) Max *float64 // theoretical maximum Min *float64 // theoretical minimum - Run string // repository-relative operational-value evaluator script + Run string // operational-value evaluator script path Script string // inline JS body for trusted custom graders (built-ins leave empty) Config map[string]any // arbitrary config passed to grader at runtime evaluatorContent string @@ -190,7 +190,7 @@ var graderIDPattern = regexp.MustCompile(`^[a-z][a-z0-9-]{0,63}$`) // return { value: trace.toolCalls.length } // unit: count // direction: lower_is_better -func (c *Compiler) parseGradersFromFrontmatter(frontmatter map[string]any) (*GradersConfig, error) { +func (c *Compiler) parseGradersFromFrontmatter(frontmatter map[string]any) (*GradersConfig, error) { //nolint:largefunc raw, exists := frontmatter["graders"] if !exists || raw == nil { return nil, nil @@ -329,7 +329,7 @@ func builtinDefFromMeta(meta *BuiltinGraderMeta) *GraderDefinition { } // parseGraderEntryFields parses individual fields from a grader entry map into the definition. -func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id string, isBuiltin bool) error { +func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id string, isBuiltin bool) error { //nolint:largefunc if v, ok := entry["enabled"]; ok { b, ok := v.(bool) if !ok { @@ -396,8 +396,8 @@ func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id stri if id != "operational-value" { return fmt.Errorf("graders.%s.run is only supported by the operational-value grader", id) } - if !isValidOperationalValueEvaluatorPath(runPath) { - return fmt.Errorf("graders.operational-value.run must be a repository-relative .sh file under .github/graders, got %q", runPath) + if !IsValidOperationalValueEvaluatorRunPath(runPath) { + return fmt.Errorf("graders.operational-value.run must be a workspace-relative or ./ local .sh file, got %q", runPath) } def.Run = runPath } @@ -433,20 +433,26 @@ func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id stri return nil } -func isValidOperationalValueEvaluatorPath(evaluatorPath string) bool { - if evaluatorPath == "" || strings.Contains(evaluatorPath, "\\") { +// IsValidOperationalValueEvaluatorRunPath reports whether evaluatorPath is a +// safe shell script path. Paths may be repository-root-relative, or explicitly +// local to the workflow file when they start with "./". +func IsValidOperationalValueEvaluatorRunPath(evaluatorPath string) bool { + if evaluatorPath == "" || strings.Contains(evaluatorPath, "\\") || strings.HasPrefix(evaluatorPath, "/") { return false } - parts := strings.Split(evaluatorPath, "/") - if len(parts) < 3 || parts[0] != ".github" || parts[1] != "graders" { + pathForValidation := evaluatorPath + if trimmed, ok := strings.CutPrefix(pathForValidation, "./"); ok { + pathForValidation = trimmed + } + if pathForValidation == "" || strings.HasPrefix(pathForValidation, "/") { return false } - for _, part := range parts { + for part := range strings.SplitSeq(pathForValidation, "/") { if part == "" || part == "." || part == ".." { return false } } - return strings.HasSuffix(evaluatorPath, ".sh") + return strings.HasSuffix(pathForValidation, ".sh") } // parseOptionalFloat parses an optional float64 field from a map. diff --git a/pkg/workflow/graders_config_test.go b/pkg/workflow/graders_config_test.go index 2f5079b44c0..b8e8cf9706e 100644 --- a/pkg/workflow/graders_config_test.go +++ b/pkg/workflow/graders_config_test.go @@ -125,25 +125,33 @@ func TestParseGradersFromFrontmatter_CustomGrader(t *testing.T) { func TestParseGradersFromFrontmatter_OperationalValueGrader(t *testing.T) { var c Compiler - cfg, err := c.parseGradersFromFrontmatter(map[string]any{ - "graders": map[string]any{ - "operational-value": map[string]any{ - "run": ".github/graders/example-operational-value.sh", - }, - }, - }) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - grader := cfg.Graders["operational-value"] - if grader.Run != ".github/graders/example-operational-value.sh" { - t.Fatalf("unexpected operational-value run path: %q", grader.Run) - } - if grader.Unit != "ratio" || grader.Direction != "higher_is_better" { - t.Fatalf("unexpected operational-value defaults: unit=%q direction=%q", grader.Unit, grader.Direction) - } - if grader.Min == nil || *grader.Min != 0 || grader.Max == nil || *grader.Max != 1 { - t.Fatalf("expected operational-value range [0,1], got min=%v max=%v", grader.Min, grader.Max) + for _, runPath := range []string{ + ".github/workflows/graders/example-operational-value.sh", + "./graders/example-operational-value.sh", + "scripts/example-operational-value.sh", + } { + t.Run(runPath, func(t *testing.T) { + cfg, err := c.parseGradersFromFrontmatter(map[string]any{ + "graders": map[string]any{ + "operational-value": map[string]any{ + "run": runPath, + }, + }, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + grader := cfg.Graders["operational-value"] + if grader.Run != runPath { + t.Fatalf("unexpected operational-value run path: %q", grader.Run) + } + if grader.Unit != "ratio" || grader.Direction != "higher_is_better" { + t.Fatalf("unexpected operational-value defaults: unit=%q direction=%q", grader.Unit, grader.Direction) + } + if grader.Min == nil || *grader.Min != 0 || grader.Max == nil || *grader.Max != 1 { + t.Fatalf("expected operational-value range [0,1], got min=%v max=%v", grader.Min, grader.Max) + } + }) } } @@ -155,15 +163,15 @@ func TestParseGradersFromFrontmatter_OperationalValueGraderValidation(t *testing errText string }{ {name: "missing run", entry: map[string]any{}, errText: "requires a 'run' field"}, - {name: "path traversal", entry: map[string]any{"run": ".github/graders/../secret.sh"}, errText: "repository-relative"}, - {name: "wrong directory", entry: map[string]any{"run": "scripts/operational-value.sh"}, errText: "repository-relative"}, - {name: "wrong extension", entry: map[string]any{"run": ".github/graders/operational-value.js"}, errText: "repository-relative"}, - {name: "inline script", entry: map[string]any{"run": ".github/graders/operational-value.sh", "script": "return 1"}, errText: "cannot have an inline script"}, - {name: "direction", entry: map[string]any{"run": ".github/graders/operational-value.sh", "direction": "lower_is_better"}, errText: "direction must be 'higher_is_better'"}, - {name: "minimum", entry: map[string]any{"run": ".github/graders/operational-value.sh", "min": 0.1}, errText: "range must be min: 0 and max: 1"}, - {name: "maximum", entry: map[string]any{"run": ".github/graders/operational-value.sh", "max": 2.0}, errText: "range must be min: 0 and max: 1"}, - {name: "threshold below range", entry: map[string]any{"run": ".github/graders/operational-value.sh", "threshold": -0.1}, errText: "threshold must be between 0 and 1"}, - {name: "threshold above range", entry: map[string]any{"run": ".github/graders/operational-value.sh", "threshold": 1.1}, errText: "threshold must be between 0 and 1"}, + {name: "path traversal", entry: map[string]any{"run": ".github/workflows/graders/../secret.sh"}, errText: "workspace-relative"}, + {name: "absolute path", entry: map[string]any{"run": "/tmp/operational-value.sh"}, errText: "workspace-relative"}, + {name: "wrong extension", entry: map[string]any{"run": ".github/workflows/graders/operational-value.js"}, errText: "workspace-relative"}, + {name: "inline script", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "script": "return 1"}, errText: "cannot have an inline script"}, + {name: "direction", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "direction": "lower_is_better"}, errText: "direction must be 'higher_is_better'"}, + {name: "minimum", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "min": 0.1}, errText: "range must be min: 0 and max: 1"}, + {name: "maximum", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "max": 2.0}, errText: "range must be min: 0 and max: 1"}, + {name: "threshold below range", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "threshold": -0.1}, errText: "threshold must be between 0 and 1"}, + {name: "threshold above range", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "threshold": 1.1}, errText: "threshold must be between 0 and 1"}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -182,7 +190,7 @@ func TestParseGradersFromFrontmatter_RunRejectedForOtherGraders(t *testing.T) { _, err := c.parseGradersFromFrontmatter(map[string]any{ "graders": map[string]any{ "custom": map[string]any{ - "run": ".github/graders/operational-value.sh", + "run": ".github/workflows/graders/operational-value.sh", }, }, }) @@ -516,7 +524,7 @@ func TestBuildGraderManifest(t *testing.T) { func TestBuildGraderManifest_OperationalValueGrader(t *testing.T) { grader := &GraderDefinition{ ID: "operational-value", - Run: ".github/graders/example-operational-value.sh", + Run: ".github/workflows/graders/example-operational-value.sh", } grader.evaluatorContent = "#!/usr/bin/env bash\necho '{}'\n" cfg := &GradersConfig{Graders: map[string]*GraderDefinition{"operational-value": grader}} @@ -535,7 +543,7 @@ func TestBuildGraderManifest_OperationalValueGrader(t *testing.T) { if err != nil { t.Fatalf("marshal operational-value manifest: %v", err) } - if !strings.Contains(string(manifestJSON), `"run":".github/graders/example-operational-value.sh"`) || strings.Contains(string(manifestJSON), `"evaluator"`) { + if !strings.Contains(string(manifestJSON), `"run":".github/workflows/graders/example-operational-value.sh"`) || strings.Contains(string(manifestJSON), `"evaluator"`) { t.Fatalf("expected manifest to use run field, got %s", manifestJSON) } @@ -604,7 +612,7 @@ func TestGenerateGradersStep_OperationalValueUsesActivationRunMetadata(t *testin c := &Compiler{} initActionPinCacheForTest(c) var yaml strings.Builder - data := operationalValueGraderWorkflowData(".github/graders/example-operational-value.sh") + data := operationalValueGraderWorkflowData(".github/workflows/graders/example-operational-value.sh") c.generateGradersStep(&yaml, data) diff --git a/pkg/workflow/graders_operational_value.go b/pkg/workflow/graders_operational_value.go index 98d30a36960..6c380e548dc 100644 --- a/pkg/workflow/graders_operational_value.go +++ b/pkg/workflow/graders_operational_value.go @@ -15,7 +15,7 @@ import ( const maxOperationalValueEvaluatorSize = 64 * 1024 -func (c *Compiler) prepareOperationalValueGrader(data *WorkflowData, markdownPath string) error { +func (c *Compiler) prepareOperationalValueGrader(data *WorkflowData, markdownPath string) error { //nolint:largefunc if data == nil || data.Graders == nil { return nil } @@ -31,7 +31,7 @@ func (c *Compiler) prepareOperationalValueGrader(data *WorkflowData, markdownPat if err != nil { return fmt.Errorf("cannot resolve graders.operational-value.run %q: workflow is not inside a Git repository", grader.Run) } - evaluatorPath := filepath.Join(repoRoot, filepath.FromSlash(grader.Run)) + evaluatorPath := ResolveOperationalValueEvaluatorPath(repoRoot, markdownPath, grader.Run) if err := fileutil.ValidatePathWithinBase(repoRoot, evaluatorPath); err != nil { return fmt.Errorf("graders.operational-value.run %q escapes the Git repository", grader.Run) } @@ -81,3 +81,13 @@ func (c *Compiler) prepareOperationalValueGrader(data *WorkflowData, markdownPat grader.evaluatorContent = evaluatorContent return nil } + +// ResolveOperationalValueEvaluatorPath resolves a validated operational-value +// evaluator run path. Paths starting with "./" are local to the workflow file's +// directory; all other paths are relative to the repository root. +func ResolveOperationalValueEvaluatorPath(repoRoot, markdownPath, runPath string) string { + if localPath, ok := strings.CutPrefix(runPath, "./"); ok { + return filepath.Join(filepath.Dir(markdownPath), filepath.FromSlash(localPath)) + } + return filepath.Join(repoRoot, filepath.FromSlash(runPath)) +} diff --git a/pkg/workflow/graders_operational_value_test.go b/pkg/workflow/graders_operational_value_test.go index 5cbd16fc4a4..13680280118 100644 --- a/pkg/workflow/graders_operational_value_test.go +++ b/pkg/workflow/graders_operational_value_test.go @@ -14,7 +14,7 @@ func TestPrepareOperationalValueGrader(t *testing.T) { t.Fatal(err) } workflowPath := filepath.Join(repoRoot, ".github", "workflows", "example.md") - evaluatorPath := filepath.Join(repoRoot, ".github", "graders", "example-operational-value.sh") + evaluatorPath := filepath.Join(repoRoot, ".github", "workflows", "graders", "example-operational-value.sh") if err := os.MkdirAll(filepath.Dir(workflowPath), 0o755); err != nil { t.Fatal(err) } @@ -25,7 +25,7 @@ func TestPrepareOperationalValueGrader(t *testing.T) { if err := os.WriteFile(evaluatorPath, []byte(content), 0o755); err != nil { t.Fatal(err) } - data := operationalValueGraderWorkflowData(".github/graders/example-operational-value.sh") + data := operationalValueGraderWorkflowData(".github/workflows/graders/example-operational-value.sh") if err := (&Compiler{}).prepareOperationalValueGrader(data, workflowPath); err != nil { t.Fatalf("unexpected error: %v", err) @@ -39,6 +39,30 @@ func TestPrepareOperationalValueGrader(t *testing.T) { } } +func TestPrepareOperationalValueGraderResolvesLocalDotPath(t *testing.T) { + repoRoot := t.TempDir() + if err := os.Mkdir(filepath.Join(repoRoot, ".git"), 0o755); err != nil { + t.Fatal(err) + } + workflowPath := filepath.Join(repoRoot, ".github", "workflows", "example.md") + evaluatorPath := filepath.Join(repoRoot, ".github", "workflows", "graders", "example-operational-value.sh") + if err := os.MkdirAll(filepath.Dir(evaluatorPath), 0o755); err != nil { + t.Fatal(err) + } + content := "#!/usr/bin/env bash\nset -euo pipefail\n" + if err := os.WriteFile(evaluatorPath, []byte(content), 0o755); err != nil { + t.Fatal(err) + } + data := operationalValueGraderWorkflowData("./graders/example-operational-value.sh") + + if err := (&Compiler{}).prepareOperationalValueGrader(data, workflowPath); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if data.Graders.Graders["operational-value"].evaluatorContent != content { + t.Fatal("expected ./ evaluator path to resolve relative to the workflow") + } +} + func TestPrepareOperationalValueGraderRejectsInvalidFiles(t *testing.T) { tests := []struct { name string @@ -57,7 +81,7 @@ func TestPrepareOperationalValueGraderRejectsInvalidFiles(t *testing.T) { t.Fatal(err) } workflowPath := filepath.Join(repoRoot, ".github", "workflows", "example.md") - evaluatorPath := filepath.Join(repoRoot, ".github", "graders", "example-operational-value.sh") + evaluatorPath := filepath.Join(repoRoot, ".github", "workflows", "graders", "example-operational-value.sh") if err := os.MkdirAll(filepath.Dir(workflowPath), 0o755); err != nil { t.Fatal(err) } @@ -70,7 +94,7 @@ func TestPrepareOperationalValueGraderRejectsInvalidFiles(t *testing.T) { } } - err := (&Compiler{}).prepareOperationalValueGrader(operationalValueGraderWorkflowData(".github/graders/example-operational-value.sh"), workflowPath) + err := (&Compiler{}).prepareOperationalValueGrader(operationalValueGraderWorkflowData(".github/workflows/graders/example-operational-value.sh"), workflowPath) if err == nil || !strings.Contains(err.Error(), test.errText) { t.Fatalf("expected error containing %q, got %v", test.errText, err) } @@ -91,7 +115,7 @@ func TestPrepareOperationalValueGraderRejectsSymlinkEscape(t *testing.T) { t.Fatal(err) } workflowPath := filepath.Join(repoRoot, ".github", "workflows", "example.md") - evaluatorPath := filepath.Join(repoRoot, ".github", "graders", "example-operational-value.sh") + evaluatorPath := filepath.Join(repoRoot, ".github", "workflows", "graders", "example-operational-value.sh") if err := os.MkdirAll(filepath.Dir(workflowPath), 0o755); err != nil { t.Fatal(err) } @@ -102,7 +126,7 @@ func TestPrepareOperationalValueGraderRejectsSymlinkEscape(t *testing.T) { t.Fatal(err) } - err := (&Compiler{}).prepareOperationalValueGrader(operationalValueGraderWorkflowData(".github/graders/example-operational-value.sh"), workflowPath) + err := (&Compiler{}).prepareOperationalValueGrader(operationalValueGraderWorkflowData(".github/workflows/graders/example-operational-value.sh"), workflowPath) if err == nil || !strings.Contains(err.Error(), "escapes") { t.Fatalf("expected symlink escape error, got %v", err) } @@ -117,7 +141,7 @@ func TestPrepareOperationalValueGraderRejectsRepositorySymlink(t *testing.T) { t.Fatal(err) } workflowPath := filepath.Join(repoRoot, ".github", "workflows", "example.md") - gradersDir := filepath.Join(repoRoot, ".github", "graders") + gradersDir := filepath.Join(repoRoot, ".github", "workflows", "graders") targetPath := filepath.Join(gradersDir, "target.sh") evaluatorPath := filepath.Join(gradersDir, "example-operational-value.sh") if err := os.MkdirAll(filepath.Dir(workflowPath), 0o755); err != nil { @@ -133,7 +157,7 @@ func TestPrepareOperationalValueGraderRejectsRepositorySymlink(t *testing.T) { t.Fatal(err) } - err := (&Compiler{}).prepareOperationalValueGrader(operationalValueGraderWorkflowData(".github/graders/example-operational-value.sh"), workflowPath) + err := (&Compiler{}).prepareOperationalValueGrader(operationalValueGraderWorkflowData(".github/workflows/graders/example-operational-value.sh"), workflowPath) if err == nil || !strings.Contains(err.Error(), "must not be a symbolic link") { t.Fatalf("expected symlink rejection error, got %v", err) } From 5a4b99d176b22d86cd5fbd94abfc2b22ac30020a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:35:10 +0000 Subject: [PATCH 09/13] Tighten grader evaluator path pattern Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/schemas/main_workflow_schema.json | 2 +- pkg/workflow/graders_config.go | 2 +- pkg/workflow/graders_config_test.go | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 3199c574a5f..acc1fdb57fc 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12898,7 +12898,7 @@ }, "run": { "type": "string", - "pattern": "^(?:\\./)?(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*|\\.\\.[^/\\\\]+)(?:/(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*|\\.\\.[^/\\\\]+))*\\.sh$", + "pattern": "^(?:\\./)?(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*)(?:/(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*))*\\.sh$", "description": "Workspace-relative or ./ workflow-local Bash script for the operational-value evaluator. Supported only for the reserved operational-value grader ID." } } diff --git a/pkg/workflow/graders_config.go b/pkg/workflow/graders_config.go index 94b49d9c95f..865a396da2b 100644 --- a/pkg/workflow/graders_config.go +++ b/pkg/workflow/graders_config.go @@ -448,7 +448,7 @@ func IsValidOperationalValueEvaluatorRunPath(evaluatorPath string) bool { return false } for part := range strings.SplitSeq(pathForValidation, "/") { - if part == "" || part == "." || part == ".." { + if part == "" || part == "." || strings.HasPrefix(part, "..") { return false } } diff --git a/pkg/workflow/graders_config_test.go b/pkg/workflow/graders_config_test.go index b8e8cf9706e..227fe5b8129 100644 --- a/pkg/workflow/graders_config_test.go +++ b/pkg/workflow/graders_config_test.go @@ -164,6 +164,7 @@ func TestParseGradersFromFrontmatter_OperationalValueGraderValidation(t *testing }{ {name: "missing run", entry: map[string]any{}, errText: "requires a 'run' field"}, {name: "path traversal", entry: map[string]any{"run": ".github/workflows/graders/../secret.sh"}, errText: "workspace-relative"}, + {name: "dot dot prefix", entry: map[string]any{"run": ".github/workflows/graders/..secret.sh"}, errText: "workspace-relative"}, {name: "absolute path", entry: map[string]any{"run": "/tmp/operational-value.sh"}, errText: "workspace-relative"}, {name: "wrong extension", entry: map[string]any{"run": ".github/workflows/graders/operational-value.js"}, errText: "workspace-relative"}, {name: "inline script", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "script": "return 1"}, errText: "cannot have an inline script"}, From 5116cab9ef683ca662d0187a8a29a28070dbcb4e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:39:25 +0000 Subject: [PATCH 10/13] Document grader path dot-prefix rejection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/graders_config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/workflow/graders_config.go b/pkg/workflow/graders_config.go index 865a396da2b..ac582bcaf94 100644 --- a/pkg/workflow/graders_config.go +++ b/pkg/workflow/graders_config.go @@ -435,7 +435,8 @@ func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id stri // IsValidOperationalValueEvaluatorRunPath reports whether evaluatorPath is a // safe shell script path. Paths may be repository-root-relative, or explicitly -// local to the workflow file when they start with "./". +// local to the workflow file when they start with "./". Empty components, ".", +// and components starting with ".." are rejected to avoid traversal ambiguity. func IsValidOperationalValueEvaluatorRunPath(evaluatorPath string) bool { if evaluatorPath == "" || strings.Contains(evaluatorPath, "\\") || strings.HasPrefix(evaluatorPath, "/") { return false From 0c7f5703e88d0628880347764ce0ac741c1c8446 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:44:20 +0000 Subject: [PATCH 11/13] Add grader evaluators to package resources Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/add_command_resources.go | 6 +- pkg/cli/add_package_manifest_resolve.go | 72 ++++++++++++++++++++++- pkg/cli/add_package_manifest_resources.go | 5 ++ pkg/cli/add_package_manifest_test.go | 47 +++++++++++++++ pkg/cli/add_package_ownership.go | 8 ++- pkg/cli/remote_workflow_test.go | 5 ++ pkg/cli/resources.go | 18 ++++-- 7 files changed, 148 insertions(+), 13 deletions(-) diff --git a/pkg/cli/add_command_resources.go b/pkg/cli/add_command_resources.go index d1621ed3eea..f9d00d0659f 100644 --- a/pkg/cli/add_command_resources.go +++ b/pkg/cli/add_command_resources.go @@ -42,13 +42,13 @@ func addResourceFileWithTracking(resolved *ResolvedWorkflow, tracker *FileTracke return fmt.Errorf("resource destination %q is invalid", resolved.Spec.DestinationPath) } destFile := filepath.Join(gitRoot, destination) + if err := fileutil.ValidatePathWithinBase(gitRoot, destFile); err != nil { + return fmt.Errorf("failed to validate resource destination %q: %w", resolved.Spec.DestinationPath, err) + } rel, err := filepath.Rel(gitRoot, destFile) if err != nil { return fmt.Errorf("failed to validate resource destination %q: %w", resolved.Spec.DestinationPath, err) } - if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - return fmt.Errorf("resource destination %q escapes repository root", resolved.Spec.DestinationPath) - } fileExists := fileutil.FileExists(destFile) if fileExists && !opts.Force { diff --git a/pkg/cli/add_package_manifest_resolve.go b/pkg/cli/add_package_manifest_resolve.go index 1f9faf0ba74..d4a0cd74546 100644 --- a/pkg/cli/add_package_manifest_resolve.go +++ b/pkg/cli/add_package_manifest_resolve.go @@ -6,7 +6,10 @@ package cli import ( "context" "fmt" + "path" "strings" + + "github.com/github/gh-aw/pkg/constants" ) func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host string) (*resolvedRepositoryPackage, error) { @@ -31,7 +34,10 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri if err != nil { return nil, err } - resourceFiles := normalizePackageResourcePaths(manifest.Resources, packagePath) + resourceFiles, err := resolveRepositoryPackageResourceFiles(ctx, owner, repo, packagePath, ref, host, manifest, installationSources) + if err != nil { + return nil, err + } docsPath, err := resolveRepositoryPackageDocsPath(ctx, owner, repo, packagePath, ref, host) if err != nil { @@ -60,6 +66,68 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri return newResolvedRepositoryPackage(manifestPath, ref, docsPath, manifest, installationSources, resourceFiles, extensionFiles, warnings), nil } +func resolveRepositoryPackageResourceFiles(ctx context.Context, owner, repo, packagePath, ref, host string, manifest *repositoryPackageManifest, installationSources []resolvedPackageInstallable) ([]resolvedPackageResource, error) { + resourceFiles := normalizePackageResourcePaths(manifest.Resources, packagePath) + return appendPackageGraderEvaluatorResources(ctx, owner, repo, ref, host, packagePath, resourceFiles, installationSources) +} + +func appendPackageGraderEvaluatorResources(ctx context.Context, owner, repo, ref, host, packagePath string, resourceFiles []resolvedPackageResource, installationSources []resolvedPackageInstallable) ([]resolvedPackageResource, error) { + seen := make(map[string]string, len(resourceFiles)) + for _, resource := range resourceFiles { + seen[packageResourceDestinationKey(resource.DestinationPath)] = resource.SourcePath + } + for _, installable := range installationSources { + if !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".md") { + continue + } + content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, installable.SourcePath, ref, host) + if err != nil { + if isRepositoryFileNotFound(err) { + addPackageManifestLog.Printf("skipping grader evaluator resource discovery for unavailable package workflow %q: %v", installable.SourcePath, err) + continue + } + return nil, fmt.Errorf("failed to read package workflow %q while resolving grader evaluator resources: %w", installable.SourcePath, err) + } + entries, err := extractResourceEntries(string(content)) + if err != nil { + return nil, fmt.Errorf("failed to parse package workflow %q grader resources: %w", installable.SourcePath, err) + } + for _, entry := range entries { + if !entry.isGraderEvaluator { + continue + } + resource := packageGraderEvaluatorResource(installable, entry.path, packagePath) + key := packageResourceDestinationKey(resource.DestinationPath) + if previousSource, exists := seen[key]; exists { + if previousSource != resource.SourcePath { + return nil, fmt.Errorf("package workflows reference multiple grader evaluator resources for %q: %q and %q", resource.DestinationPath, previousSource, resource.SourcePath) + } + continue + } + seen[key] = resource.SourcePath + resourceFiles = append(resourceFiles, resource) + } + } + return resourceFiles, nil +} + +func packageGraderEvaluatorResource(installable resolvedPackageInstallable, runPath, packagePath string) resolvedPackageResource { + if localPath, ok := strings.CutPrefix(runPath, "./"); ok { + return resolvedPackageResource{ + SourcePath: path.Join(path.Dir(installable.SourcePath), localPath), + DestinationPath: path.Join(path.Dir(installable.DestinationPath), localPath), + } + } + sourcePath := joinRepositoryPackagePath(packagePath, runPath) + if localWorkflowsPath, ok := strings.CutPrefix(runPath, constants.WorkflowsDirSlash); ok { + sourcePath = path.Join(path.Dir(installable.SourcePath), localWorkflowsPath) + } + return resolvedPackageResource{ + SourcePath: sourcePath, + DestinationPath: runPath, + } +} + func splitRepositoryPackageSlug(repoSlug string) (string, string, error) { parts := strings.SplitN(repoSlug, "/", 2) if len(parts) != 2 { @@ -173,7 +241,7 @@ func newResolvedRepositoryPackage(manifestPath, ref, docsPath string, manifest * func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, packagePath, ref, host string) (string, []byte, error) { manifestPath := joinRepositoryPackagePath(packagePath, repositoryPackageManifestFileName) - repoSlug := owner + "/" + repo + repoSlug := path.Join(owner, repo) packageID := repositoryPackageIdentifier(repoSlug, packagePath) content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, manifestPath, ref, host) if err != nil { diff --git a/pkg/cli/add_package_manifest_resources.go b/pkg/cli/add_package_manifest_resources.go index f4cd5970fc1..5152ecd33a3 100644 --- a/pkg/cli/add_package_manifest_resources.go +++ b/pkg/cli/add_package_manifest_resources.go @@ -102,9 +102,14 @@ func normalizePackageResourcePaths(resources []repositoryPackageResource, packag DestinationPath: resource.Destination, }) } + return normalized } +func packageResourceDestinationKey(destination string) string { + return strings.ToLower(filepath.ToSlash(filepath.Clean(destination))) +} + func normalizeLocalPackageResourcePaths(resources []repositoryPackageResource, packageDir string) ([]resolvedPackageResource, error) { normalized := make([]resolvedPackageResource, 0, len(resources)) for _, resource := range resources { diff --git a/pkg/cli/add_package_manifest_test.go b/pkg/cli/add_package_manifest_test.go index 5753b4ae9eb..6d44200f439 100644 --- a/pkg/cli/add_package_manifest_test.go +++ b/pkg/cli/add_package_manifest_test.go @@ -122,6 +122,53 @@ resources: assert.Equal(t, "packages/repo-assist/policy/controls.json", pkg.ResourceFiles[1].SourcePath) assert.Equal(t, ".github/aw/policy/controls.json", pkg.ResourceFiles[1].DestinationPath) }) + + t.Run("adds grader evaluators to package resources", func(t *testing.T) { + downloadPackageFileFromGitHubForHost = func(_ context.Context, owner, repo, path, ref, host string) ([]byte, error) { + switch path { + case "packages/repo-assist/aw.yml": + return []byte(`name: Repo Assist +files: + - workflows/review.md + - workflows/triage.md +`), nil + case "packages/repo-assist/README.md": + return []byte("# Repo Assist\n"), nil + case "packages/repo-assist/workflows/review.md": + return []byte(`--- +on: pull_request +graders: + operational-value: + run: .github/workflows/graders/shared-operational-value.sh +--- +# Review +`), nil + case "packages/repo-assist/workflows/triage.md": + return []byte(`--- +on: issues +graders: + operational-value: + run: ./graders/triage-operational-value.sh +--- +# Triage +`), nil + default: + return nil, createRepositoryPackageNotFoundError(path) + } + } + listPackageWorkflowFilesForHost = func(_ context.Context, owner, repo, ref, workflowPath, host string) ([]string, error) { + t.Fatalf("unexpected scan of %s", workflowPath) + return nil, nil + } + + pkg, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo", PackagePath: "packages/repo-assist"}, "") + require.NoError(t, err) + require.Len(t, pkg.ResourceFiles, 2) + assert.Equal(t, "packages/repo-assist/workflows/graders/shared-operational-value.sh", pkg.ResourceFiles[0].SourcePath) + assert.Equal(t, ".github/workflows/graders/shared-operational-value.sh", pkg.ResourceFiles[0].DestinationPath) + assert.Equal(t, "packages/repo-assist/workflows/graders/triage-operational-value.sh", pkg.ResourceFiles[1].SourcePath) + assert.Equal(t, ".github/workflows/graders/triage-operational-value.sh", pkg.ResourceFiles[1].DestinationPath) + }) getRepositoryPackageLatestRelease = func(_ context.Context, repoSlug, host string) (string, error) { assert.Equal(t, "owner/repo", repoSlug) assert.Equal(t, "github.com", host) diff --git a/pkg/cli/add_package_ownership.go b/pkg/cli/add_package_ownership.go index 6f4e46ee1dd..de67ba83182 100644 --- a/pkg/cli/add_package_ownership.go +++ b/pkg/cli/add_package_ownership.go @@ -243,7 +243,7 @@ func readPackageOwnershipRecords(gitRoot string) ([]packageOwnershipRecord, erro return records, nil } -func syncManifestManagedResources(ctx context.Context, repoSpec *RepoSpec, pkg *resolvedRepositoryPackage, ref string, opts UpdateWorkflowsOptions) error { +func syncManifestManagedResources(ctx context.Context, repoSpec *RepoSpec, pkg *resolvedRepositoryPackage, ref string, opts UpdateWorkflowsOptions) error { //nolint:largefunc if pkg == nil || repoSpec == nil { return nil } @@ -286,6 +286,9 @@ func syncManifestManagedResources(ctx context.Context, repoSpec *RepoSpec, pkg * for _, resource := range pkg.ResourceFiles { destination := filepath.ToSlash(filepath.Clean(resource.DestinationPath)) destPath := filepath.Join(gitRoot, filepath.FromSlash(destination)) + if err := fileutil.ValidatePathWithinBase(gitRoot, destPath); err != nil { + return fmt.Errorf("resource %q escapes repository root: %w", destination, err) + } if fileutil.FileExists(destPath) && !opts.Force { if owned, drifted := packageOwnershipAllowsOverwrite(gitRoot, destination, packageBase); !owned || drifted { if owned { @@ -442,7 +445,8 @@ func upsertPackageOwnershipFile(entries []packageOwnershipFileEntry, next packag func isPackageResourceDestination(destination string) bool { return strings.EqualFold(destination, constants.GithubDir+"CODEOWNERS") || strings.HasPrefix(destination, constants.GithubDir+"ISSUE_TEMPLATE/") || - strings.HasPrefix(destination, constants.GithubDir+"aw/") + strings.HasPrefix(destination, constants.GithubDir+"aw/") || + workflow.IsValidOperationalValueEvaluatorRunPath(destination) } func removePackageOwnedFilesIfUnused(packageBase string) error { diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 5686961bbb5..cf6682816b0 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -1876,6 +1876,11 @@ graders: assert.Equal(t, evaluatorContent, installed) require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) + evaluatorContent = []byte("#!/usr/bin/env bash\necho conflict\n") + err = fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil) + require.ErrorContains(t, err, evaluatorPath) + require.ErrorContains(t, err, "--force") + workflowPath := filepath.Join(workflowsDir, "graded.md") require.NoError(t, os.WriteFile(workflowPath, []byte(content), 0o644)) compiler := workflow.NewCompiler() diff --git a/pkg/cli/resources.go b/pkg/cli/resources.go index 84856d0212f..b84ac1af2dd 100644 --- a/pkg/cli/resources.go +++ b/pkg/cli/resources.go @@ -60,6 +60,8 @@ func extractResourceEntries(content string) ([]extractedResource, error) { return nil, err } if graders != nil { + // Include evaluator paths even for disabled graders so package resources stay + // complete and gh aw update can restore them if the grader is later re-enabled. for _, grader := range graders.Graders { if grader != nil && grader.Run != "" { resources = append(resources, extractedResource{path: grader.Run, isGraderEvaluator: true}) @@ -143,6 +145,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work // Resources are resolved relative to the source workflow's directory in the remote repo. workflowBaseDir := getParentDir(spec.WorkflowPath) + var gitRoot string for _, resource := range resourcePaths { resourcePath := resource.path @@ -188,10 +191,13 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work } targetBaseDir := targetDir if isWorkspaceRelativeGraderEvaluator { - targetBaseDir, err = gitutil.FindGitRootFrom(targetDir) - if err != nil { - return fmt.Errorf("failed to resolve repository root for grader resource %q: %w", resourcePath, err) + if gitRoot == "" { + gitRoot, err = gitutil.FindGitRootFrom(targetDir) + if err != nil { + return fmt.Errorf("failed to resolve repository root for grader resource %q: %w", resourcePath, err) + } } + targetBaseDir = gitRoot localRelPath = filepath.FromSlash(resourcePath) } targetPath := filepath.Join(targetBaseDir, localRelPath) @@ -242,9 +248,9 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work continue } if fileExists && !force && resource.isGraderEvaluator { - existingContent, err := os.ReadFile(targetPath) - if err != nil { - return fmt.Errorf("failed to read existing grader resource %q: %w", targetPath, err) + existingContent, readErr := os.ReadFile(targetPath) + if readErr != nil { + return fmt.Errorf("failed to read existing grader resource %q: %w", targetPath, readErr) } if bytes.Equal(existingContent, fileContent) { continue From 8c75bc0cf962365ad50910e983ed20dbcfa3e558 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:47:11 +0000 Subject: [PATCH 12/13] Address package resource review nits Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/add_package_manifest_resolve.go | 2 +- pkg/cli/add_package_manifest_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cli/add_package_manifest_resolve.go b/pkg/cli/add_package_manifest_resolve.go index d4a0cd74546..2a77ce4d9f5 100644 --- a/pkg/cli/add_package_manifest_resolve.go +++ b/pkg/cli/add_package_manifest_resolve.go @@ -241,7 +241,7 @@ func newResolvedRepositoryPackage(manifestPath, ref, docsPath string, manifest * func loadRepositoryPackageManifestFile(ctx context.Context, owner, repo, packagePath, ref, host string) (string, []byte, error) { manifestPath := joinRepositoryPackagePath(packagePath, repositoryPackageManifestFileName) - repoSlug := path.Join(owner, repo) + repoSlug := fmt.Sprintf("%s/%s", owner, repo) packageID := repositoryPackageIdentifier(repoSlug, packagePath) content, err := downloadPackageFileFromGitHubForHost(ctx, owner, repo, manifestPath, ref, host) if err != nil { diff --git a/pkg/cli/add_package_manifest_test.go b/pkg/cli/add_package_manifest_test.go index 6d44200f439..d88959c7ef2 100644 --- a/pkg/cli/add_package_manifest_test.go +++ b/pkg/cli/add_package_manifest_test.go @@ -168,6 +168,7 @@ graders: assert.Equal(t, ".github/workflows/graders/shared-operational-value.sh", pkg.ResourceFiles[0].DestinationPath) assert.Equal(t, "packages/repo-assist/workflows/graders/triage-operational-value.sh", pkg.ResourceFiles[1].SourcePath) assert.Equal(t, ".github/workflows/graders/triage-operational-value.sh", pkg.ResourceFiles[1].DestinationPath) + assert.True(t, isPackageResourceDestination(pkg.ResourceFiles[1].DestinationPath)) }) getRepositoryPackageLatestRelease = func(_ context.Context, repoSlug, host string) (string, error) { assert.Equal(t, "owner/repo", repoSlug) From 453e3ab8a64fa960c079d174f5c703d1f3080dbb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:52:46 +0000 Subject: [PATCH 13/13] Address grader packaging review follow-ups Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_package_manifest_resolve.go | 1 + pkg/cli/remote_workflow_test.go | 24 ++++++++------------ pkg/cli/resources.go | 18 +++++++++------ pkg/parser/schema_test.go | 1 + pkg/parser/schemas/main_workflow_schema.json | 2 +- pkg/workflow/graders_config.go | 4 ++-- pkg/workflow/graders_config_test.go | 2 +- 7 files changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/cli/add_package_manifest_resolve.go b/pkg/cli/add_package_manifest_resolve.go index 2a77ce4d9f5..8af1b268aa2 100644 --- a/pkg/cli/add_package_manifest_resolve.go +++ b/pkg/cli/add_package_manifest_resolve.go @@ -76,6 +76,7 @@ func appendPackageGraderEvaluatorResources(ctx context.Context, owner, repo, ref for _, resource := range resourceFiles { seen[packageResourceDestinationKey(resource.DestinationPath)] = resource.SourcePath } + addPackageManifestLog.Printf("resolving grader evaluators from %d installable package source(s)", len(installationSources)) for _, installable := range installationSources { if !strings.HasSuffix(strings.ToLower(installable.SourcePath), ".md") { continue diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index cf6682816b0..78caaf0d089 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -1857,9 +1857,7 @@ graders: # Workflow ` evaluatorContent := []byte("#!/usr/bin/env bash\necho old\n") - originalDownload := downloadResourceFileFromGitHub - t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) - downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { + download := func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { assert.Equal(t, "workflows/graders/example-operational-value.sh", filePath) return evaluatorContent, nil } @@ -1868,16 +1866,16 @@ graders: RepoSpec: RepoSpec{RepoSlug: "owner/repo", Version: "main"}, WorkflowPath: "workflows/graded.md", } - require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) + require.NoError(t, fetchAndSaveRemoteResourcesWithDownloader(t.Context(), content, spec, workflowsDir, false, false, nil, download)) installedPath := filepath.Join(tmpDir, filepath.FromSlash(evaluatorPath)) installed, err := os.ReadFile(installedPath) require.NoError(t, err) assert.Equal(t, evaluatorContent, installed) - require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) + require.NoError(t, fetchAndSaveRemoteResourcesWithDownloader(t.Context(), content, spec, workflowsDir, false, false, nil, download)) evaluatorContent = []byte("#!/usr/bin/env bash\necho conflict\n") - err = fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil) + err = fetchAndSaveRemoteResourcesWithDownloader(t.Context(), content, spec, workflowsDir, false, false, nil, download) require.ErrorContains(t, err, evaluatorPath) require.ErrorContains(t, err, "--force") @@ -1889,7 +1887,7 @@ graders: require.NoError(t, os.Remove(installedPath)) evaluatorContent = []byte("#!/usr/bin/env bash\necho new\n") - require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, true, nil)) + require.NoError(t, fetchAndSaveRemoteResourcesWithDownloader(t.Context(), content, spec, workflowsDir, false, true, nil, download)) restored, err := os.ReadFile(installedPath) require.NoError(t, err) assert.Equal(t, evaluatorContent, restored) @@ -1911,9 +1909,7 @@ graders: # Workflow ` evaluatorContent := []byte("#!/usr/bin/env bash\necho local\n") - originalDownload := downloadResourceFileFromGitHub - t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) - downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { + download := func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { assert.Equal(t, "workflows/scripts/example-operational-value.sh", filePath) return evaluatorContent, nil } @@ -1922,7 +1918,7 @@ graders: RepoSpec: RepoSpec{RepoSlug: "owner/repo", Version: "main"}, WorkflowPath: "workflows/graded.md", } - require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)) + require.NoError(t, fetchAndSaveRemoteResourcesWithDownloader(t.Context(), content, spec, workflowsDir, false, false, nil, download)) installed, err := os.ReadFile(filepath.Join(workflowsDir, "scripts", "example-operational-value.sh")) require.NoError(t, err) @@ -1949,9 +1945,7 @@ graders: # Workflow ` - originalDownload := downloadResourceFileFromGitHub - t.Cleanup(func() { downloadResourceFileFromGitHub = originalDownload }) - downloadResourceFileFromGitHub = func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { + download := func(_ context.Context, owner, repo, filePath, ref string) ([]byte, error) { return []byte("#!/usr/bin/env bash\necho unsafe\n"), nil } @@ -1959,7 +1953,7 @@ graders: RepoSpec: RepoSpec{RepoSlug: "owner/repo", Version: "main"}, WorkflowPath: "workflows/graded.md", } - require.NoError(t, fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, true, nil)) + require.NoError(t, fetchAndSaveRemoteResourcesWithDownloader(t.Context(), content, spec, workflowsDir, false, true, nil, download)) assert.NoFileExists(t, filepath.Join(outsideDir, "example-operational-value.sh")) } diff --git a/pkg/cli/resources.go b/pkg/cli/resources.go index b84ac1af2dd..9e93f81b575 100644 --- a/pkg/cli/resources.go +++ b/pkg/cli/resources.go @@ -18,13 +18,13 @@ import ( "github.com/github/gh-aw/pkg/workflow" ) -var downloadResourceFileFromGitHub = parser.DownloadFileFromGitHub - type extractedResource struct { path string isGraderEvaluator bool } +type resourceDownloader func(ctx context.Context, owner, repo, filePath, ref string) ([]byte, error) + // extractResourceEntries extracts file paths from the top-level "resources" frontmatter // field and validated grader evaluator paths. // Returns an error if any entry contains GitHub Actions expression syntax (e.g. "${{"), @@ -113,7 +113,11 @@ func extractResources(content string) ([]string, error) { // from the same source are silently skipped. // For non-Markdown resource files: if the target already exists and force is false, an error // is returned regardless of origin (non-markdown files have no source tracking). -func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { //nolint:largefunc // Keep resource conflict, download, and tracking behavior together. +func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { + return fetchAndSaveRemoteResourcesWithDownloader(ctx, content, spec, targetDir, verbose, force, tracker, parser.DownloadFileFromGitHub) +} + +func fetchAndSaveRemoteResourcesWithDownloader(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker, download resourceDownloader) error { //nolint:largefunc // Keep resource conflict, download, and tracking behavior together. if spec.RepoSlug == "" { return nil } @@ -162,8 +166,8 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work // resource behavior; workspace-relative grader evaluators install at their exact // repository-relative run path. var remoteFilePath string - isWorkspaceRelativeGraderEvaluator := resource.isGraderEvaluator && !strings.HasPrefix(resourcePath, "./") - if isWorkspaceRelativeGraderEvaluator { + isRepoRootAnchoredGraderEvaluator := resource.isGraderEvaluator && !strings.HasPrefix(resourcePath, "./") + if isRepoRootAnchoredGraderEvaluator { remoteFilePath = resourcePath if strings.HasPrefix(remoteFilePath, constants.WorkflowsDirSlash) && workflowBaseDir != "" { remoteFilePath = path.Join(workflowBaseDir, strings.TrimPrefix(remoteFilePath, constants.WorkflowsDirSlash)) @@ -190,7 +194,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work continue } targetBaseDir := targetDir - if isWorkspaceRelativeGraderEvaluator { + if isRepoRootAnchoredGraderEvaluator { if gitRoot == "" { gitRoot, err = gitutil.FindGitRootFrom(targetDir) if err != nil { @@ -240,7 +244,7 @@ func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *Work } // Download from source repository - fileContent, err := downloadResourceFileFromGitHub(ctx, owner, repo, remoteFilePath, ref) + fileContent, err := download(ctx, owner, repo, remoteFilePath, ref) if err != nil { if verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch resource %s: %v", remoteFilePath, err))) diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index e26bbec976d..f57b44879bb 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -1126,6 +1126,7 @@ func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_OperationalValueGr for _, runPath := range []string{ ".github/workflows/graders/example-operational-value.sh", + ".github/workflows/graders/..secret.sh", "./graders/example-operational-value.sh", } { t.Run(runPath, func(t *testing.T) { diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index acc1fdb57fc..3199c574a5f 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12898,7 +12898,7 @@ }, "run": { "type": "string", - "pattern": "^(?:\\./)?(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*)(?:/(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*))*\\.sh$", + "pattern": "^(?:\\./)?(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*|\\.\\.[^/\\\\]+)(?:/(?:[^./\\\\][^/\\\\]*|\\.[^./\\\\][^/\\\\]*|\\.\\.[^/\\\\]+))*\\.sh$", "description": "Workspace-relative or ./ workflow-local Bash script for the operational-value evaluator. Supported only for the reserved operational-value grader ID." } } diff --git a/pkg/workflow/graders_config.go b/pkg/workflow/graders_config.go index ac582bcaf94..1465b83c6be 100644 --- a/pkg/workflow/graders_config.go +++ b/pkg/workflow/graders_config.go @@ -436,7 +436,7 @@ func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id stri // IsValidOperationalValueEvaluatorRunPath reports whether evaluatorPath is a // safe shell script path. Paths may be repository-root-relative, or explicitly // local to the workflow file when they start with "./". Empty components, ".", -// and components starting with ".." are rejected to avoid traversal ambiguity. +// and ".." are rejected to avoid traversal. func IsValidOperationalValueEvaluatorRunPath(evaluatorPath string) bool { if evaluatorPath == "" || strings.Contains(evaluatorPath, "\\") || strings.HasPrefix(evaluatorPath, "/") { return false @@ -449,7 +449,7 @@ func IsValidOperationalValueEvaluatorRunPath(evaluatorPath string) bool { return false } for part := range strings.SplitSeq(pathForValidation, "/") { - if part == "" || part == "." || strings.HasPrefix(part, "..") { + if part == "" || part == "." || part == ".." { return false } } diff --git a/pkg/workflow/graders_config_test.go b/pkg/workflow/graders_config_test.go index 227fe5b8129..b52bff2c9f5 100644 --- a/pkg/workflow/graders_config_test.go +++ b/pkg/workflow/graders_config_test.go @@ -127,6 +127,7 @@ func TestParseGradersFromFrontmatter_OperationalValueGrader(t *testing.T) { var c Compiler for _, runPath := range []string{ ".github/workflows/graders/example-operational-value.sh", + ".github/workflows/graders/..secret.sh", "./graders/example-operational-value.sh", "scripts/example-operational-value.sh", } { @@ -164,7 +165,6 @@ func TestParseGradersFromFrontmatter_OperationalValueGraderValidation(t *testing }{ {name: "missing run", entry: map[string]any{}, errText: "requires a 'run' field"}, {name: "path traversal", entry: map[string]any{"run": ".github/workflows/graders/../secret.sh"}, errText: "workspace-relative"}, - {name: "dot dot prefix", entry: map[string]any{"run": ".github/workflows/graders/..secret.sh"}, errText: "workspace-relative"}, {name: "absolute path", entry: map[string]any{"run": "/tmp/operational-value.sh"}, errText: "workspace-relative"}, {name: "wrong extension", entry: map[string]any{"run": ".github/workflows/graders/operational-value.js"}, errText: "workspace-relative"}, {name: "inline script", entry: map[string]any{"run": ".github/workflows/graders/operational-value.sh", "script": "return 1"}, errText: "cannot have an inline script"},