Add payload-based grader execution command - #58634
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
clients2.google.commtalk.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "clients2.google.com"
- "mtalk.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · claude · sonnet46 · 50.6 AIC · ⌖ 18.2 AIC · ⊞ 7.8K
Comment /smoke-claude to run again
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "gh-aw": minor | |||
There was a problem hiding this comment.
✅ Smoke test review comment #1: Changeset entry looks good — gh-aw: minor bump is appropriate for a new subcommand. Consider documenting any migration steps for users. — Run 33932452774
| /tmp/gh-aw/agent_output.json | ||
| /tmp/gh-aw/safeoutputs.jsonl | ||
| /tmp/gh-aw/agent/graders/grader_manifest.json | ||
| /tmp/gh-aw/agent/graders/grader_payload.json |
There was a problem hiding this comment.
✅ Smoke test review comment #2: Adding /tmp/gh-aw/agent to artifact paths is a good practice for capturing agent outputs. Ensure this path is cleaned up between runs to avoid stale artifacts. — Run 33932452774
|
@copilot test support for running script directly (run: script file.sh) and inline scripts. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added explicit execution coverage for both forms in b18f6dc: an operational-value |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "pypi.org"
- "raw.githubusercontent.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
🏗️ ADR required — draft added for PR #58634I enforced the ADR gate for this PR because the prefetch summary shows 590 added lines in default business logic directories, which is above the 100-line threshold. Evidence used
Gate resultA draft ADR has been added at:
Inferred decision captured in the draft
Next action for the authorPlease review and refine
|
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. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "pypi.org"
- "raw.githubusercontent.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
No blocking issues found in the grader replay changeset. The new graders run path, payload archival, and redaction wiring are internally consistent, and the targeted coverage around the new artifact path and command surface is adequate.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
api.github.comgithub.compypi.orgraw.githubusercontent.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "pypi.org"
- "raw.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 56.7 AIC · ⌖ 7.32 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design to the new graders run payload-execution command — requesting changes for one functional bug.
📋 Key Themes & Highlights
Key Themes
- Bug:
runOperationalValuePayloadnever usesconfig.Repo, callinggetGitHubHostForRepo("")twice — the--repooverride silently has no effect on host resolution for operational-value graders. - Test coverage gap:
graders_run_test.goonly exercises the stdin payload path; the artifact-download branch (RunID != 0) and the--repooverride are completely untested. - Architecture:
graders_run.cjsduplicates the VM sandbox (deepFreeze, context lockdown, safeMath, helpers) already implemented inactions/setup/js/trace_graders_worker.cjs, risking security-hardening drift between the two copies.
Positive Highlights
- ✅ Solid payload validation (
readGraderPayload): size limit, empty check, JSON validity, all with source-aware errors. - ✅ Sensible timeout and stderr handling around the Node subprocess (
runJavaScriptGrader), including a distinct message for deadline exceeded. - ✅ Good CLI ergonomics —
graders rundocs, examples, and integration test coverage for help text and invalid run IDs.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 56.2 AIC · ⌖ 16.1 AIC · ⊞ 10.3K
Comment /matt to run again
| return nil | ||
| } | ||
|
|
||
| func runOperationalValuePayload(ctx context.Context, workflowArg string, payload json.RawMessage, output io.Writer) error { |
There was a problem hiding this comment.
[/diagnosing-bugs] runOperationalValuePayload ignores config.Repo entirely, calling getGitHubHostForRepo("") twice instead of getGitHubHostForRepo(config.workflowArg)-equivalent using the actual repo override. This defeats the --repo flag for the operational-value grader path, since the host is always resolved from empty string (falling back to the default host) regardless of what the user passed.
💡 Suggested fix
func runOperationalValuePayload(ctx context.Context, workflowArg, repo string, payload json.RawMessage, output io.Writer) error {
host := getGitHubHostForRepo(repo)
evaluator, err := loadOperationalValueReportEvaluator(ctx, workflowArg, host)
...
runOperationalValueEvaluatorBash(ctx, "/bin/bash", evaluator.EvaluatorPath,
[]string{evaluator.EvaluatorPath, "--grade-run"}, payload, operationalValueEvaluatorTimeout, host)and update the call site in runGrader to pass config.Repo through. Also worth adding a regression test that sets Repo and asserts the evaluator/bash call receives the expected host, since graders_run_test.go currently never exercises config.Repo at all.
@copilot please address this.
| return "test" | ||
| } | ||
|
|
||
| func TestRunGraderFromStdin(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] No test exercises loadGraderRunPayload's artifact-download branch (config.RunID != 0) or the config.Repo override — only the stdin path is covered. This is the more complex, higher-risk branch (temp dirs, artifact listing/downloading, flattenUnifiedArtifact, file lookup) and it currently has zero regression coverage.
💡 Suggested test
Add a test that fakes/mocks the artifact download (or uses a lightweight seam) to verify: (1) the correct artifact names are requested, (2) findGraderFile locates grader_payload.json after flattenUnifiedArtifact, and (3) an error is returned when the payload file is missing from the artifact — this last case is easy to assert without network access by pre-seeding a temp dir.
@copilot please address this.
| } | ||
| return value; | ||
| } | ||
|
|
There was a problem hiding this comment.
[/codebase-design] This file reimplements the inline-grader VM sandbox (deepFreeze, vm.createContext with codeGeneration lockdown, safeMath, helpers, timeouts) that already exists in actions/setup/js/trace_graders_worker.cjs. Two independently-maintained copies of a security-sensitive sandbox is a risk: a future hardening fix (e.g. new blocked global, stricter timeout) applied to one will silently miss the other.
💡 Suggested improvement
Consider extracting the shared sandbox construction (deepFreeze, context setup, safeMath, helpers) into a common module both trace_graders_worker.cjs and graders_run.cjs can require, or at minimum add a code comment cross-referencing the other file so changes are kept in sync, plus a shared test asserting both sandboxes reject the same set of dangerous globals.
@copilot please address this.
There was a problem hiding this comment.
Two simplifications are worth cutting in the new grader runner: the bespoke Node sandbox and the temp-script subprocess wrapper. net: -20 lines possible.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comgithub.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "github.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58634 · codex · mai10 · 11.6 AIC · ⌖ 24.2 AIC · ⊞ 14K
Comment /ponytail to run again
| return value; | ||
| } | ||
|
|
||
| function runInline(grader, trace) { |
There was a problem hiding this comment.
pkg/cli/graders_run.cjs:L56: yagni: 140-line custom VM sandbox for one-off graders. Keep a small adapter and reuse the workflow runner until a second runtime shape exists.
| return data, nil | ||
| } | ||
|
|
||
| func runJavaScriptGrader(ctx context.Context, grader graderRunDefinition, payload json.RawMessage, output io.Writer) error { |
There was a problem hiding this comment.
pkg/cli/graders_run.go:L174: shrink: temp-file staging plus a subprocess wrapper around every grader run. A single in-process helper or direct node -e entrypoint would remove most of this ceremony.
There was a problem hiding this comment.
Reviewed graders_run.go/graders_run.cjs (new payload-based grader execution). Sandboxing (frozen vm context, disabled require/process/eval), payload size limits, JSON validation, and temp-file permissions all look solid.
One correctness bug found: --repo is ignored for operational-value graders (see inline comment on pkg/cli/graders_run.go:223). Everything else in this diff — CLI wiring, artifact download path, JS grader execution, redaction-step change (now unconditional, matches the fact that built-in payloads also contain trace data) — looks correct and well tested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 76.6 AIC · ⌖ 13.4 AIC · ⊞ 8.3K
| } | ||
|
|
||
| func runOperationalValuePayload(ctx context.Context, workflowArg string, payload json.RawMessage, output io.Writer) error { | ||
| evaluator, err := loadOperationalValueReportEvaluator(ctx, workflowArg, getGitHubHostForRepo("")) |
There was a problem hiding this comment.
--repo is silently ignored when the grader is operational-value. runGrader never threads config.Repo into runOperationalValuePayload, which calls getGitHubHostForRepo("") twice (line 223 and 229) instead of getGitHubHostForRepo(config.Repo). Elsewhere in this file (loadGraderRunPayload) and in the existing graders operational-value command, --repo correctly overrides host/owner/repo resolution. Here, running gh aw graders run my-workflow operational-value 123 --repo other/repo will resolve the evaluator using the default host config instead of the repo the user asked for, which is surprising and inconsistent with the rest of the command.
Fix: pass config.Repo through to runOperationalValuePayload and use getGitHubHostForRepo(config.Repo) for both calls.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the follow-up commit: --repo is normalized to the requested repository host and passed to both evaluator loading and --grade-run execution. The script-file regression test now exercises Repo and verifies GH_HOST.
@copilot Please take the next forward-progress pass on this PR.
|
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>
Fixed in commit |
Adds a
graders runsubcommand for executing one workflow grader against either a completed run’s archived payload or JSON from standard input.CLI
gh aw graders run <workflow-id> <grader-id> [run-id].--repo.Payload replay
Execution
gh aw graders run weekly-research loops 123456789 cat payload.json | gh aw graders run weekly-research custom-score✨ PR Review Safe Output Test - Run 33932452774
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
clients2.google.commtalk.google.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.