Include grader evaluator scripts in workflow packages#56268
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot shell scripts should already be parsed, checked and added to resources |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 37675b8. Grader evaluator paths now go through the existing validated workflow resource extraction/fetch pipeline; the package-specific parser was removed. Tests cover deduplication, repository-relative installation, restoration, and clean compilation. |
|
Category: bug | Risk: medium | Score: 54 (impact 28 + urgency 16 + quality 10) | Action: batch_review | Batch: workflow-runtime | Note: Workflow package resource regression fix.
|
There was a problem hiding this comment.
Pull request overview
Adds grader evaluator discovery and installation for packaged workflows.
Changes:
- Extracts validated grader evaluator paths as resources.
- Installs evaluators repository-relative and deduplicates paths.
- Adds evaluator installation, restoration, and compilation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/resources.go |
Discovers and installs grader evaluator resources. |
pkg/cli/remote_workflow_test.go |
Tests evaluator extraction and installation. |
Review details
Suppressed comments (1)
pkg/cli/resources.go:226
- When
forceis true for an existing evaluator, the file is overwritten beforetracker.TrackModifiedruns at line 268.TrackModifiedtherefore captures the downloaded bytes rather than the original file, so a later package-install failure cannot roll back the user's previous evaluator. Track existing files beforeos.WriteFile, as the dispatch and call-workflow paths do.
if fileExists && !force && isGraderEvaluator {
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| if graders != nil { | ||
| for _, grader := range graders.Graders { | ||
| if grader != nil && grader.Run != "" { | ||
| paths = append(paths, grader.Run) |
There was a problem hiding this comment.
Addressed in 0c7f570. Grader evaluator paths are now added to resolvedRepositoryPackage.ResourceFiles, so package ownership and syncManifestManagedResources reconcile them, including dedupe and update/restore behavior.
| 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, "..") { |
There was a problem hiding this comment.
Addressed in 0c7f570. Resource writes now use fileutil.ValidatePathWithinBase before writing, including package add/update resource paths, so symlinked parents cannot redirect writes outside the repository.
|
@copilot why is any of these changes needed? |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new grader-evaluator packaging path introduces one filesystem-safety regression and one package-ownership regression that can both break downstream installs and updates.
Blocking themes
- repo-root evaluator writes are guarded only by lexical path checks, so a symlinked
.github/graderspath can still redirect a forced update outside the repository - evaluator files are treated as ad hoc shared assets instead of manifest-managed package resources, so different packages can silently clobber each other's pinned evaluator script on later updates
I left inline comments on the exact sites.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 24.1 AIC · ⌖ 8.3 AIC · ⊞ 4.6K
Comment /review to run again
| 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, "..") { |
There was a problem hiding this comment.
This path check is not sufficient for a force update: filepath.Abs/filepath.Rel stay purely lexical, so an existing .github/graders symlink can still redirect the write outside the repository root and --force will happily overwrite the symlink target.
💡 Why this blocks merge
The new grader-evaluator path now writes to the repo root specifically so gh aw update can restore missing evaluators, but that also means the safety boundary has to be the real filesystem path, not just a cleaned string path.
Today the code does:
absTargetBase, _ := filepath.Abs(targetBaseDir)
absTargetPath, _ := filepath.Abs(targetPath)
rel, _ := filepath.Rel(absTargetBase, absTargetPath)That never resolves symlinks. If .github/graders or one of its parents is a symlink, targetPath still looks like it lives under the repo while the eventual os.WriteFile follows the link and writes somewhere else. This is especially bad on the force=true path because the later conflict checks are skipped entirely.
Please resolve both the base and destination with filepath.EvalSymlinks (or reject symlinked parents outright) before writing, and add a test that plants a symlinked .github/graders directory and proves the write is refused.
There was a problem hiding this comment.
Addressed in 0c7f570. The lexical path check was replaced with the repository symlink-aware path validator, and a symlinked-parent regression test now proves the write is refused.
| } | ||
| continue | ||
| } | ||
| if fileExists && !force && isGraderEvaluator { |
There was a problem hiding this comment.
The duplicate-evaluator shortcut only compares file bytes, so two different packages can silently share the same .github/graders/... path and then overwrite each other on the next update without any ownership/drift check.
💡 Why this blocks merge
This change intentionally treats grader scripts as shared repo-root assets:
if fileExists && !force && isGraderEvaluator {
existingContent, _ := os.ReadFile(targetPath)
if bytes.Equal(existingContent, fileContent) {
continue
}
}That avoids an add-time conflict, but it also means the file is never tied back to the package that installed it. Once package A and package B both reference the same path with identical initial content, a later gh aw update for either package can replace the script with new content under force=true even if the other package still owns that file. The old package will now compile against a different evaluator than the one it pinned, and there is no ownership record to stop or even detect that drift.
This needs package-level ownership for grader evaluators (the same resolvedRepositoryPackage.ResourceFiles / manifest-managed path used for other package resources), or the feature will produce cross-package clobbering bugs as soon as two packages reuse the same evaluator path.
There was a problem hiding this comment.
Addressed in 0c7f570. Evaluators are now package ResourceFiles, so they are installed and reconciled through package ownership instead of ad hoc per-workflow dependency writes.
There was a problem hiding this comment.
One small simplification seems worth cutting. net: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #56268 · codex · mai10 · 7.44 AIC · ⌖ 1.99 AIC · ⊞ 16.7K
Comment /ponytail to run again
| ) | ||
|
|
||
| // extractResources extracts file paths from the top-level "resources" frontmatter field. | ||
| var downloadResourceFileFromGitHub = parser.DownloadFileFromGitHub |
There was a problem hiding this comment.
pkg/cli/resources.go:L21: yagni: package-level downloader seam for one test. Call parser.DownloadFileFromGitHub directly and use a tiny helper only if you really need a seam.
There was a problem hiding this comment.
No code change for this one. I kept the downloader seam because these resource-fetch tests need deterministic no-network content, and this file already follows package-level downloader seam patterns used elsewhere in package resolution tests.
There was a problem hiding this comment.
Good addition overall — the happy-path coverage is solid and the deduplication logic is clean.
Two non-blocking issues worth addressing:
-
Fragile grader-path heuristic —
isGraderEvaluatoris re-derived from a path prefix string at download time. A grader pointing outside.github/graders/silently misbehaves. See inline comment onresources.go. -
Disabled graders still install their scripts — paths from
enabled: falsegraders are collected and downloaded. See inline comment.
The existing review comments from the earlier round (ownership gap, symlink traversal) are also worth addressing before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 30.7 AIC · ⌖ 9.2 AIC · ⊞ 6.2K
| paths = v | ||
| } | ||
|
|
||
| // Reject entries that contain GitHub Actions expression syntax — macros are not allowed. |
There was a problem hiding this comment.
The isGraderEvaluator flag is inferred from a path-prefix string (constants.GithubDir+"graders/") rather than from how the path entered the list. A grader whose run: points outside .github/graders/ silently falls through to workflow-relative path resolution, producing a wrong install path with no warning.
Consider tagging origins at collection time — e.g. a small {path string; isGrader bool} struct — so the flag is structurally guaranteed rather than re-derived from the path string.
@copilot please address this.
| } | ||
| case []string: | ||
| paths = append(paths, v...) | ||
| } |
There was a problem hiding this comment.
Disabled graders (enabled: false) still have their evaluator scripts fetched and installed here. A grader marked disabled signals that it should not run; downloading its script anyway is surprising and may cause unexpected conflicts if another workflow later ships a different version of the same file.
Either skip grader.Run collection when grader.Enabled == false, or add a comment explaining why installing a disabled grader's evaluator is intentional.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. I added an in-code comment explaining that disabled grader evaluator paths are intentionally included so package resources stay complete and gh aw update can restore them if re-enabled later.
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 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR exceeds the 100-line threshold for business logic changes (160 new lines in No existing Architecture Decision Record was found in the PR body, on the branch, or in any linked issue. A draft ADR has been generated and committed to this branch: 📄 What was inferred from this PR
Next steps for
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes for correctness and test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Missing conflict test: The content-conflict branch (
resources.go:226-235) — the most security-sensitive new code path — has no test covering the error case. - Fragile re-classification:
isGraderEvaluatoris re-derived from the path prefix string insidefetchAndSaveRemoteResources, duplicating knowledge that is already authoritative inextractResources. - Variable shadowing risk: The
errvariable is reused in the content-equality branch without a distinct name, which is easy to accidentally break during refactors. - Missing rationale: Disabled graders' evaluators are silently included with no comment explaining why.
Positive Highlights
- ✅ Clean factoring of deduplication via
seenmap inextractResources. - ✅ Good test coverage for the happy path (install + idempotent re-install + restore).
- ✅ Repository-root resolution for grader paths (
FindGitRootFrom) is correct and safe. - ✅ Existing comments from prior review (ResourceFiles registration, symlink safety) are important and should also be addressed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 59.8 AIC · ⌖ 10.3 AIC · ⊞ 7.6K
Comment /matt to run again
| return nil, err | ||
| } | ||
| if graders != nil { | ||
| for _, grader := range graders.Graders { |
There was a problem hiding this comment.
[/tdd] Disabled graders' evaluators are silently included with no in-code rationale — this will surprise reviewers and future maintainers.
The test TestExtractResources_IncludesDisabledGraderEvaluator establishes the behaviour, but there is no comment explaining why disabled evaluators are included (e.g. so that gh aw update can restore them if the grader is later re-enabled).
💡 Suggested comment
// Include evaluator paths even for disabled graders so that
// package resources are complete: gh aw update can restore or
// update the script if the grader is later re-enabled.
for _, grader := range graders.Graders {@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. resources.go now documents why disabled grader evaluators are included in the package resource set.
| // 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/") |
There was a problem hiding this comment.
[/codebase-design] Hardcoded path prefix ".github/graders/" for grader detection is fragile and duplicates knowledge that already lives in GraderDefinition.Run validation.
If the graders config ever allows run paths outside .github/graders/ (or the prefix changes), this check will silently misclassify them. Consider deriving the isGraderEvaluator flag from a set built by extractResources itself when parsing graders, rather than re-inspecting the path string here.
💡 Alternative approach
Build a set of evaluator paths in extractResources and pass it alongside resourcePaths into fetchAndSaveRemoteResources, or return a struct that carries both the paths and their kind. This removes the re-classification by prefix.
@copilot please address this.
| } | ||
| continue | ||
| } | ||
| if fileExists && !force && isGraderEvaluator { |
There was a problem hiding this comment.
[/diagnosing-bugs] The content-equality conflict check reads the existing file after the download is complete, meaning a download error will not be caught before the expensive I/O. More importantly, the err variable from downloadResourceFileFromGitHub is re-used in scope from an outer declaration (var err error is implicitly captured) — a new := inside the branch shadows it correctly, but this is easy to accidentally break during a refactor.
💡 Suggestion
Consider pre-reading the existing file before downloading (when fileExists && !force && isGraderEvaluator) so you can skip the download entirely when the file is present and a fast hash matches. At minimum, give the err variable here a distinct name to make the scope crystal-clear:
existingContent, readErr := os.ReadFile(targetPath)
if readErr != nil {
return fmt.Errorf(...)
}@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. The existing-file read now uses a distinct readErr variable; the content comparison still downloads first because it needs the remote bytes to determine whether the local evaluator is identical.
| require.NoError(t, compiler.CompileWorkflow(workflowPath)) | ||
|
|
||
| require.NoError(t, os.Remove(installedPath)) | ||
| evaluatorContent = []byte("#!/usr/bin/env bash\necho new\n") |
There was a problem hiding this comment.
[/tdd] The test verifies restore (force=true) but never tests the conflict case — when the file exists with different content and force=false. That branch (resources.go:226-235) returns a user-facing error and is the most security-sensitive path added in this PR.
💡 Suggested extra assertion or separate test
// Conflict: existing file differs from remote — should error without force.
require.NoError(t, os.WriteFile(installedPath, []byte("#!/usr/bin/env bash\necho different\n"), 0o644))
evaluatorContent = []byte("#!/usr/bin/env bash\necho new\n")
err := fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)
require.ErrorContains(t, err, "already exists")@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. TestFetchAndSaveRemoteResources_InstallsAndRestoresGraderEvaluator now covers the existing-file/different-content/force=false conflict path and asserts the --force error.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on efficiency and test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Efficiency:
gitutil.FindGitRootFromis called per loop iteration; should be hoisted or lazily initialised once. - Ordering issue: network download happens before the existing-file read in the grader conflict check — the common idempotent case always pays for a network round-trip.
- Fragile detection:
isGraderEvaluatorrelies on a hardcoded path prefix rather than information fromextractResources, creating a maintenance hazard. - Missing test: the conflict path (file exists, different content, force=false → error) has no coverage.
Positive Highlights
- ✅ Clean separation of grader evaluators from ordinary resources in
extractResources. - ✅ Deduplication logic is correct and well-placed.
- ✅ Path-traversal boundary check extended correctly to both
targetDirandgitRoot. - ✅
downloadResourceFileFromGitHubis made injectable for testing — a good testability decision. - ✅ Good integration test that exercises install, idempotent re-install, and force-restore end-to-end.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 51.2 AIC · ⌖ 10.5 AIC · ⊞ 7.6K
Comment /matt to run again
| targetPath := filepath.Join(targetDir, localRelPath) | ||
| targetBaseDir := targetDir | ||
| if isGraderEvaluator { | ||
| targetBaseDir, err = gitutil.FindGitRootFrom(targetDir) |
There was a problem hiding this comment.
[/codebase-design] gitutil.FindGitRootFrom is called on every loop iteration for every grader evaluator, even though targetDir doesn't change. Move this call above the loop (or lazily initialise once) to avoid repeated filesystem traversals — especially relevant when a package includes multiple evaluators.
💡 Suggested fix
// Before the loop:
var gitRoot string
// Inside the loop, replacing the current block:
if isGraderEvaluator {
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)
}@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. The repository root lookup for workspace-relative grader resources is now lazily initialized once and reused across resource iterations.
| } | ||
| continue | ||
| } | ||
| if fileExists && !force && isGraderEvaluator { |
There was a problem hiding this comment.
[/codebase-design] The conflict check downloads the file first (line 219) and only then reads the existing file to compare (line 227). This means a network request always fires even when the file exists and is unchanged — the common case for idempotent re-installs. The comment on line 192 says "conflict handling is deferred", but the download isn't deferred alongside it.
💡 Suggested approach
Read the existing file content before downloading, and skip the download entirely when fileExists && !force && isGraderEvaluator. Proceed to download only when you cannot determine staleness without the remote content — or accept the download but at least document why it cannot be avoided.
At minimum, rename or remove the misleading comment on line 192 so it only covers the write-conflict deferral, not the download.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. I clarified the existing-file branch and renamed the read error variable; the remote download remains necessary before equality comparison because the code must compare against the fetched evaluator bytes.
| 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)) |
There was a problem hiding this comment.
[/tdd] The test covers install, idempotent re-install (same content), and force-restore — but is missing the conflict case: when the file exists with different content and force=false, fetchAndSaveRemoteResources should return an error. That is the key new behaviour added in resources.go lines 226–234, and it has no test coverage.
💡 Suggested test addition
After the idempotent re-install assertion on line 1876, add:
// Conflict: file exists with different content, force=false should error.
evaluatorContent = []byte("#!/usr/bin/env bash\necho conflict\n")
err = fetchAndSaveRemoteResources(t.Context(), content, spec, workflowsDir, false, false, nil)
require.Error(t, err)
require.ErrorContains(t, err, evaluatorPath)
require.ErrorContains(t, err, "--force")Without this, a future refactor could silently remove the conflict guard.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c7f570. The restore test now includes the non-force conflict case and verifies the evaluator path and --force guidance in the error.
| // 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/") |
There was a problem hiding this comment.
[/codebase-design] isGraderEvaluator is detected by a hardcoded path prefix (".github/graders/"). If workflow.ParseGradersFromFrontmatter ever normalises the run path (strips ./, resolves a symlink, or allows absolute paths), paths that are semantically grader evaluators will silently fall through to ordinary resource resolution.
💡 Suggested alternative
Propagate the distinction from extractResources rather than re-inferring it from the path string. For example, return a struct or two slices (regular resources and grader evaluator paths), so fetchAndSaveRemoteResources can use set-membership rather than a fragile prefix check. This also eliminates the coupling between path convention and routing logic.
@copilot please address this.
|
@copilot graders should use a relative to the GitHub workspace root ( |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in a1405fb, with follow-ups in 5a4b99d and 5116cab. |
|
I also attempted a branch refresh where allowed. After addressing the above, please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran
Actions taken: added grader evaluators to package Hand-off: CI must be re-triggered by a maintainer before merge. Still needed: human review/approval and resolving review threads in the GitHub UI. |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
PR #56268 Review SummaryApplied Status: COMMENT — no blocking issues, but 3 issues worth fixing:
Minor: @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /diagnosing-bugs to the highest-impact files: resources.go, add_package_manifest_resolve.go, graders_config.go, and the new test additions.
Overall this is a well-structured fix with good test coverage. A few issues warrant attention before merge: a race condition in test infrastructure, a post-download conflict check that wastes a network round-trip, and a subtle discrepancy between the Go validator and the JSON schema pattern.
📋 Key Themes & Highlights
Key Themes
- Race condition risk:
downloadResourceFileFromGitHubis a package-levelvarmutated by three tests withoutt.Parallel()suppression — if any are run in parallel the mutation is unsafe. - Post-download conflict check: Grader evaluator conflict detection downloads the file first, then compares bytes. The network round-trip is wasted when the file already exists and
--forcewas not passed. ../foo.shpath bypass:IsValidOperationalValueEvaluatorRunPathusesstrings.HasPrefix(part, "..")but does not reject the bare..component, allowing a path like../foo.sh.- Schema vs validator divergence: The JSON schema regex and Go validator encode subtly different sets of acceptable paths.
Positive Highlights
- ✅ Disabled graders still trigger resource discovery — correct and well-reasoned
- ✅
packageResourceDestinationKeynormalises paths case-insensitively across OS conventions - ✅
ValidatePathWithinBasereplaces the oldfilepath.Relboundary check — symlink-aware upgrade is excellent - ✅ Conflict error message for grader evaluators is actionable (
--force)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 95.7 AIC · ⌖ 10.8 AIC · ⊞ 7.6K
Comment /matt to run again
| ) | ||
|
|
||
| // extractResources extracts file paths from the top-level "resources" frontmatter field. | ||
| var downloadResourceFileFromGitHub = parser.DownloadFileFromGitHub |
There was a problem hiding this comment.
[/tdd] downloadResourceFileFromGitHub is a package-level var mutated by three tests; none call t.Parallel(), but this is a fragile contract — any future t.Parallel() addition causes a data race.
💡 Suggestion
Inject the function as a parameter or via a functional option instead of mutating a global:
func fetchAndSaveRemoteResources(ctx context.Context, content string, spec *WorkflowSpec,
targetDir string, verbose bool, force bool, tracker *FileTracker,
download func(ctx context.Context, owner, repo, path, ref string) ([]byte, error),
) error {Tests can then pass a stub directly — no global mutation, fully parallel-safe.
@copilot please address this.
| } | ||
| continue | ||
| } | ||
| if fileExists && !force && resource.isGraderEvaluator { |
There was a problem hiding this comment.
[/diagnosing-bugs] Grader evaluator conflict detection downloads the file before checking for a content mismatch. When fileExists && !force, the download has already happened — the network round-trip is wasted when the existing file differs.
💡 Suggestion
Read the existing file content eagerly (before the download) so you can skip the download entirely when the file already exists and --force is false:
if fileExists && !force && resource.isGraderEvaluator {
existingContent, readErr := os.ReadFile(targetPath)
if readErr == nil {
// file readable — download only to compare; but consider returning early
// with a "use --force" error instead and skipping the download altogether
}
}Alternatively, return the conflict error before downloading, matching the behaviour for non-grader resources.
@copilot please address this.
| return strings.HasSuffix(pathForValidation, ".sh") | ||
| } | ||
|
|
||
| // parseOptionalFloat parses an optional float64 field from a map. |
There was a problem hiding this comment.
[/diagnosing-bugs] IsValidOperationalValueEvaluatorRunPath uses strings.HasPrefix(part, "..") to reject traversal, but a bare .. component satisfies strings.HasPrefix("..", "..") — it is rejected. However, a component like ..foo would also be rejected, which is overly strict (valid directory names can start with ..). More critically, the comment says "components starting with .. are rejected" but the intent seems to be "the component .. exactly". The existing code is safe due to the symlink-aware ValidatePathWithinBase backstop, but the validator comment is misleading.
💡 Suggestion
Use an exact match instead of a prefix check to match the documented intent:
for part := range strings.SplitSeq(pathForValidation, "/") {
if part == "" || part == "." || part == ".." {
return false
}
}This is the same guard used in the old code and avoids silently rejecting legitimate directory names like ..files/.
@copilot please address this.
| } | ||
|
|
||
| func appendPackageGraderEvaluatorResources(ctx context.Context, owner, repo, ref, host, packagePath string, resourceFiles []resolvedPackageResource, installationSources []resolvedPackageInstallable) ([]resolvedPackageResource, error) { | ||
| seen := make(map[string]string, len(resourceFiles)) |
There was a problem hiding this comment.
[/codebase-design] appendPackageGraderEvaluatorResources eagerly downloads every .md source file in installationSources to discover grader run paths. For a package with many workflows this is O(n) network calls during resolution — before a user has even confirmed gh aw add. The ADR acknowledges this cost but doesn't note the user-visible latency impact.
💡 Suggestion
Consider a two-phase approach: parse grader config from the already-downloaded manifest metadata where possible, and defer evaluator content downloading to the install step (where downloads already happen). If downloading at resolution time is intentional and necessary, add a log line so users see progress during slow resolutions:
addPackageManifestLog.Printf("resolving grader evaluators from %d workflow sources", len(installationSources))@copilot please address this.
| run: .github/workflows/graders/example-operational-value.sh | ||
| --- | ||
|
|
||
| # Workflow |
There was a problem hiding this comment.
[/tdd] The three new TestFetchAndSaveRemoteResources_* tests mutate the package-level downloadResourceFileFromGitHub var. They're not marked t.Parallel() which prevents a data race today, but there's no explicit enforcement. A comment or (nolint/redacted):paralleltest is insufficient — a future contributor adding t.Parallel() to these tests won't know about the global dependency.
💡 Suggestion
Add a (nolint/redacted):paralleltest comment with an explicit reason on each test that mutates the global, or better — refactor to inject the dependency (see comment on resources.go:21). At minimum, add an internal comment in each test:
// NOTE: cannot call t.Parallel() — test mutates the downloadResourceFileFromGitHub package global.@copilot please address this.
| // repository-relative run path. | ||
| var remoteFilePath string | ||
| if rest, ok := strings.CutPrefix(resourcePath, "/"); ok { | ||
| isWorkspaceRelativeGraderEvaluator := resource.isGraderEvaluator && !strings.HasPrefix(resourcePath, "./") |
There was a problem hiding this comment.
[/codebase-design] isWorkspaceRelativeGraderEvaluator is determined by two disjoint conditions: resource.isGraderEvaluator and !strings.HasPrefix(resourcePath, "./"). This naming is misleading — ./graders/foo.sh is workspace-relative semantically, but the boolean is false for it. The intended meaning is "grader evaluator that uses a repo-root-anchored path" (as opposed to workflow-dir-relative). The current name makes the conditional block harder to reason about.
💡 Suggestion
Rename to something that captures the actual semantic:
isRepoRootAnchoredGraderEvaluator := resource.isGraderEvaluator && !strings.HasPrefix(resourcePath, "./")And update the inline comment to match.
@copilot please address this.
Packaged workflows omitted evaluator scripts referenced by
graders.*.run, causing installed workflows to fail compilation in clean repositories.Package resolution
runtarget to package resources at its repository-relative path.Package lifecycle
gh aw updateto update or restore missing evaluators.gh aw addomits grader evaluator files from packages #56006Run: https://github.com/github/gh-aw/actions/runs/33081464762