compiler: add evals job for BinEval binary evaluations#43700
Conversation
…ob.go, compiler_evals.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…s, clarify step ID replacement comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR adds compiler features for BinEval evaluations (5 production Go files, 2 production JavaScript files, 1,138 lines added) but includes no test coverage. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
Design Decision Gate — ADR Found, Implementation Largely AlignedADR reviewed: ADR-43641: Add BinEval-Style Evals Support — implementation aligns with the stated decision, with one minor undocumented design choice worth capturing.
Verification SummaryThe ADR-43641 was drafted for the prior scaffolding PR (#43641) that introduced the
One design decision not captured in the ADR: The evals engine step reuses Suggested ADR updateIn the Decision section of
Also consider changing Status from
|
There was a problem hiding this comment.
Pull request overview
Adds compiler support for a new BinEval-style evals workflow feature, generating an evals job to run binary (YES/NO) evaluations on agent output plus a push_evals job to persist evals.jsonl to an evals/<workflowID> branch.
Changes:
- Introduces a new
evalsjob builder and step builders that run an engine in AWF sandbox, parse answers, redact, and upload anevals.jsonlartifact. - Introduces a new
push_evalsjob that downloads the evals artifact and commits it to a dedicatedevals/...branch via a signed-commit path. - Wires
push_evalsinto the compiler’s memory-management job build pass and runtime-features job-name list.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/jobs.go | Adds push_evals to built-in job-name list for runtime feature injection. |
| pkg/workflow/evals_steps.go | Implements evals step generation (prepare/setup/engine/parse/redact/upload) and helper utilities. |
| pkg/workflow/evals_job.go | Implements the top-level evals job builder, including dependencies, condition, permissions, and artifact downloads. |
| pkg/workflow/compiler_jobs.go | Wires push_evals job creation into the compiler job build flow. |
| pkg/workflow/compiler_evals.go | Implements the push_evals job (checkout, download artifact, run push script, job condition). |
| actions/setup/js/run_evals.cjs | Adds the engine-agnostic eval harness (prompt construction + log parsing). |
| actions/setup/js/push_evals.cjs | Adds the branch persistence script (commit + signed push with retries). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 8
- Review effort level: Low
| func (c *Compiler) buildUploadEvalsArtifactStep(data *WorkflowData) []string { | ||
| evalsArtifactName := artifactPrefixExprForAgentDownstreamJob(data) + constants.EvalsArtifactName | ||
| return []string{ |
| // Refresh baseRef and fetch the updated remote history so that | ||
| // pushSignedCommits can resolve the new baseRef in git rev-list. | ||
| try { | ||
| const { stdout: lsOut } = await exec.getExecOutput("git", ["ls-remote", "origin", `refs/heads/${branchName}`], { cwd: workspaceDir }); | ||
| const remoteHead = lsOut.trim().split(/\s+/)[0] || ""; |
| default: | ||
| sb.WriteRune(r) | ||
| } |
| pushExperimentsStateJobName: {}, | ||
| pushRepoMemoryJobName: {}, | ||
| updateCacheMemoryJobName: {}, | ||
| pushEvalsJobName: {}, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on correctness and coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Correctness risk (
evals_steps.go): hand-rolledjsonStringLiteralmisses Unicode control-char escaping, producing invalid JSON for edge-case inputs. - Safety risk (
push_evals.cjs):fs.rmSyncon every workspace-root entry when creating an orphan branch can silently delete runner-injected credentials or caches. - Observability gap (
run_evals.cjs): positional fallback for answer extraction is silent; out-of-order answers from the engine become silently mapped to wrong questions. - Test coverage gap: no compiler-level tests for the new
evals+push_evalsjob pair, unlike the detection job which has formal and samples test suites. - Stale TODO in
compiler_jobs.gostill says the evals job is a no-op — should be removed.
Positive Highlights
- ✅ Excellent reuse of detection infrastructure (
IsDetectionRun,buildCleanFirewallDirsStep,buildPullAWFContainersStep) avoids duplication and keeps both paths consistent. - ✅ Signed-commit push with exponential-backoff retry and baseRef refresh mirrors
push_experiments_statecleanly. - ✅ Two-phase
setup/parsedesign is clean and engine-agnostic. - ✅
continue-on-error: trueon engine execution and parse steps ensures the pipeline always reaches the upload step. - ✅
evals_config_test.goalready covers the frontmatter parsing layer thoroughly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 115.8 AIC · ⌖ 9.45 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/evals_steps.go:385
[/tdd] jsonStringLiteral does not escape Unicode control characters (U+0000–U+001F, excluding \n, \r, \t) — the JSON spec requires \uXXXX escaping for them. A question string with a bare NUL byte produces invalid JSON in GH_AW_EVALS_QUESTIONS, silently breaking all eval results.
<details>
<summary>💡 Suggested fix</summary>
Replace the hand-rolled serialiser with encoding/json:
func jsonStringLiteral(s string) string {
b, _ := json.Marshal(s) // always succeeds for a st…
</details>
<details><summary>pkg/workflow/compiler_jobs.go:217</summary>
**[/codebase-design]** The TODO comment on line 216 says the evals job is still a no-op, but `buildEvalsJob` is now fully implemented. Stale TODO comments become misleading fast — this one will confuse the next reader.
Please remove (or rewrite) the TODO now that the implementation is live.
@copilot please address this.
</details>
<details><summary>pkg/workflow/evals_steps.go:260</summary>
**[/tdd]** `strings.Replace(..., 1)` replaces only the first occurrence of `"id: agentic_execution"` in a line. If a future engine emits two steps with that ID in the same pass, the second one will not be renamed and the compiled YAML will have a duplicate step ID, causing a silent runtime failure.
<details>
<summary>💡 Suggested fix</summary>
Use `strings.ReplaceAll` to be safe:
```go
prefixed := strings.ReplaceAll(line, "id: agentic_execution", "id: evals_agentic_execution")Or assert…
actions/setup/js/run_evals.cjs:141
[/tdd] The positional fallback silently patches over an ID-match failure — if the engine returns answers out of order (Q3 before Q1), the positional index maps the wrong answer to the wrong question, and the mismatch is invisible in logs.
<details>
<summary>💡 Suggested fix</summary>
Log a warning when falling back to positional matching so operators know the ID-based extraction failed:
if (answer === 'UNKNOWN' && i < positionalAnswers.length && positionalAnswers[i]) {
core.warni…
</details>
<details><summary>actions/setup/js/push_evals.cjs:64</summary>
**[/tdd]** `fs.rmSync(..., { recursive: true, force: true })` runs unconditionally on every file in the workspace root when creating an orphan branch, including any live tool cache or credentials that GitHub Actions injects at known paths. This could silently delete `.npmrc`, node_modules caches, or other pre-populated runner state.
<details>
<summary>💡 Safer approach</summary>
Only remove files that were actually part of the sparse-checkout file list, or use `git read-tree --empty` + `git c…
</details>
<details><summary>pkg/workflow/evals_steps.go:308</summary>
**[/codebase-design]** `buildRedactEvalsSecretsStep` calls `redact_secrets.cjs` with no env vars. `redact_secrets.cjs` scans the entire `/tmp/gh-aw` tree by default — it will pick up `evals.jsonl` automatically. However, this implicit coupling means that if the redact script's discovery logic changes, evals results could silently stop being redacted without any compile-time indication.
<details>
<summary>💡 Suggestion</summary>
Either document the implicit coupling in the step's comment, or p…
</details>
<details><summary>pkg/workflow/evals_steps.go:208</summary>
**[/codebase-design]** `evalsData` hardcodes `Tools: map[string]any{"bash": []any{"*"}}` instead of inheriting from the main workflow's tool config. If the workflow uses a different tool set (e.g. only file-write tools), the evals engine will silently get broader bash access than intended.
<details>
<summary>💡 Suggestion</summary>
Document why bash wildcards are explicitly required here (evals needs no MCP tools, only bash to run scripts), or inherit and narrow:
```go
// BinEval tasks only …
</details>
<details><summary>pkg/workflow/evals_job.go:17</summary>
**[/tdd]** There are no compiler-level tests for the new `buildEvalsJob` path. The detection job has `samples_threat_detection_test.go` and `compiler_threat_detection_formal_test.go` that compile golden-file workflows and assert the produced YAML structure. The evals job lacks equivalent coverage, so changes to job dependencies, condition expressions, or step ordering have no safety net.
<details>
<summary>💡 Suggested approach</summary>
Add a `samples_evals_test.go` (or similar) that compile…
</details>
<details><summary>pkg/workflow/compiler_evals.go:93</summary>
**[/codebase-design]** `push_evals` requests `contents: write` but the `checkout` step above it sets `persist-credentials: false`. The signed-commit GraphQL path uses `github.token` passed via the `GH_TOKEN` env var, which is fine. The plain-git fallback in `pushSignedCommits` however relies on credentials in the git remote URL (token embedded in the HTTPS URL). If the fallback path is exercised, the `contents: write` permission is what grants the token that power — which is good — but the perm…
</details>
There was a problem hiding this comment.
Review: Evals Job Implementation
Two bugs will cause correctness failures for non-trivial configurations, plus two medium robustness issues. Needs fixes before merge.
Blocking issues summary
HIGH — push_evals.cjs L169/200: remote set-url origin sets an unauthenticated URL; the ls-remote origin call in the retry loop will always fail on private repos, keeping currentBaseRef stale and making every retry hit the same conflict. The retry mechanism is broken by default.
HIGH — evals_steps.go L144–154: manual EngineConfig copy drops Auth (and ~15 other fields). Workflows using OIDC or Azure auth will have evalsEngineConfig.Auth == nil, silently running inference without credentials. The evals job sets id-token: write permission correctly, but the engine config that drives AWF_AUTH_* env vars is missing.
MEDIUM — run_evals.cjs L79: agent_output.json loaded unbounded into memory then embedded in prompt. Large agent outputs will OOM the github-script runner or exceed model context limits.
MEDIUM — run_evals.cjs L190–193: unknown GH_AW_EVALS_PHASE values silently run setupMain() instead of failing. A misconfigured parse step silently overwrites the prompt.
🔎 Code quality review by PR Code Quality Reviewer · 216.4 AIC · ⌖ 6.95 AIC · ⊞ 5.4K
Comment /review to run again
| // Refresh baseRef and fetch the updated remote history so that | ||
| // pushSignedCommits can resolve the new baseRef in git rev-list. | ||
| try { | ||
| const { stdout: lsOut } = await exec.getExecOutput("git", ["ls-remote", "origin", `refs/heads/${branchName}`], { cwd: workspaceDir }); |
There was a problem hiding this comment.
Stale baseRef on retry: origin is set without a token, so ls-remote always fails silently on private repos, leaving currentBaseRef stale and causing every subsequent retry to fail.
💡 Explanation and fix
At line 169 the remote URL is set without a credential:
execGitSync(["remote", "set-url", "origin",
`https://${serverHost}/${targetRepo}.git`], ...);Immediately after, the retry loop calls ls-remote origin (line 200) against that unauthenticated URL. On any private repository, git will fail with 403/404. The bare catch {} at line ~215 swallows the error and keeps the old currentBaseRef. pushSignedCommits then retries with the same (wrong) base SHA, hitting the same conflict, and eventually calls core.setFailed after all retries are exhausted.
Fix: embed the token in the origin URL (mirroring the repoUrl already constructed above):
const authenticatedOrigin = `(xaccesstoken/redacted):${ghToken}@${serverHost}/${targetRepo}.git`;
execGitSync(["remote", "set-url", "origin", authenticatedOrigin], { stdio: "pipe", cwd: workspaceDir });Alternatively, pass env: getGitAuthEnv(ghToken) to the exec.getExecOutput call.
| APITarget: evalsEngineConfig.APITarget, | ||
| HarnessScript: evalsEngineConfig.HarnessScript, | ||
| Driver: evalsEngineConfig.Driver, | ||
| } |
There was a problem hiding this comment.
Auth field is not copied into evalsEngineConfig, so evals jobs using OIDC or Azure key-based auth will silently run without credentials and fail at inference time.
💡 Missing fields and fix
The manual struct copy (lines 144–154) copies only:
ID, Model, Version, Env, Config, Args, APITarget, HarnessScript, Driver.
It silently drops:
Auth— OIDC/Azure auth config (mapped toAWF_AUTH_*env vars byApplyAuthEnvVars)LLMProvider— inference provider overridePermissionMode,MaxTurns,MaxToolDenials,MaxRuns,MaxTurnCacheMisses,MaxAICreditsUserAgent,Command,Agent,Bare,TokenWeightsInlineProviderAuth,InlineProviderRequest,InlineProviderIDMCPSessionTimeout,MCPToolTimeout,Extensions
The most critical is Auth: buildEvalsJob already sets PermissionIdToken: write when data.EngineConfig.Auth.Type == "github-oidc", but evalsEngineConfig.Auth will be nil, so the AWF proxy sidecar never receives the OIDC config and falls back to anonymous access.
Fix: use a shallow pointer copy instead of a manual field list, or explicitly copy Auth:
copy := *engineConfig // shallow copy of all fields
evalsEngineConfig = ©This is much safer than maintaining a manual field list as EngineConfig grows.
| steps = append(steps, prefixed+"\n") | ||
| // Inject always() condition and continue-on-error after the first line (- name:) | ||
| // so that infrastructure failures do not block the parse step that follows. | ||
| if i == 0 { |
There was a problem hiding this comment.
Injecting if: always() and continue-on-error after i == 0 assumes the first string returned by GetExecutionSteps is always the - name: line; this is a fragile coupling that will silently break if any engine returns a leading comment or blank line.
💡 Details
The code:
if i == 0 {
steps = append(steps, " if: always()\n")
steps = append(steps, " continue-on-error: true\n")
}This inserts YAML fields at position 1 in the step, which is only valid YAML if position 0 (i == 0) is the - name: key. If an engine's GetExecutionSteps returns e.g. [" # comment\n", " - name: run engine\n", ...], the injected if: and continue-on-error: fields are emitted as top-level items in the step block before - name:, producing invalid YAML.
The detection job has the same injection pattern (buildDetectionEngineExecutionStep), but that code was reviewed against a stable contract. Adding a second callsite makes this contract load-bearing.
Safer fix: add a well-known sentinel like id: agentic_execution detection and inject after the - name: line by searching for it, or define a structured interface method WithAlwaysCondition(steps [][]string) [][]string in the engine API.
|
@copilot remove code to push results from this Pr |
| You are evaluating the output of an AI agentic workflow using BinEval (binary evaluation). | ||
| For each question below, answer with exactly YES or NO based on the agent output provided. | ||
|
|
||
| ## Questions |
There was a problem hiding this comment.
@copilot use xml tags to enclose questions and information
There was a problem hiding this comment.
Done. Updated buildEvalPrompt to use XML tags: <questions>, <question number="..." id="...">, <agent_output>, and <instructions> to structure the BinEval prompt sections.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Removed all |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
|
PR Triage
New BinEval evals job support in the compiler (+774/-11, 4 files). Two new emitted jobs: evals + push_evals. Significant new compiler behavior; needs careful human review.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR Finisher Summary
Actions taken:
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or push) before merge to verify the changes on CI infrastructure. Still needed: Human to re-trigger CI workflows to get fresh status checks on the agent's HEAD commit (73f8758). |
|
@copilot please run the Unresolved review feedback to address first:
|
The evals job depends on activation in both branches (safe_outputs + activation, or agent + activation), but not always on agent. When safe_outputs is enabled, needs.agent is not available. Use artifactPrefixExprForDownstreamJob (needs.activation.outputs.artifact_prefix) instead of artifactPrefixExprForAgentDownstreamJob (needs.agent.outputs.artifact_prefix) to fix artifact name resolution in workflow_call runs. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed the unresolved review feedback in commit 94d961f:
All review threads have been addressed. Validation passed:
|
Adds compiler support for a new
evalsjob that runs YES/NO (BinEval) evaluations against agent output. Whenevals:is declared in workflow frontmatter, the compiler emits two new jobs:evals(runs aftersafe_outputs) andpush_evals(persists results to a git branch).New files
actions/setup/js/run_evals.cjs— Engine-agnostic harness dispatching onGH_AW_EVALS_PHASE:setup: builds a multi-question BinEval prompt fromGH_AW_EVALS_QUESTIONS+agent_output.json, writes toaw-prompts/prompt.txtparse: extracts per-question YES/NO from the engine log (ID-match first, positional fallback), writesevals.jsonlactions/setup/js/push_evals.cjs— Commitsevals.jsonltoevals/<workflowID>branch via signed commits (mirrorspush_experiment_state.cjs)pkg/workflow/evals_steps.go— Step builders: reuses detection infrastructure (buildCleanFirewallDirsStep,buildPullAWFContainersStep); engine runs withIsDetectionRun: truefor AWF sandbox + restricted network; handles Codex MCP setup, Pi model normalization, APITarget inheritancepkg/workflow/evals_job.go— Implements previously-stubbedbuildEvalsJob; depends onsafe_outputs+activation(falls back toagent+activation); condition mirrors detection patternpkg/workflow/compiler_evals.go—buildPushEvalsJob; pushes artifact toevals/<sanitizedWorkflowID>branch; condition:always() && !cancelled() && needs.evals.result != 'failure'Modified files
pkg/workflow/jobs.go—pushEvalsJobName = "push_evals"+runtimeFeaturesBuiltInJobNamesentrypkg/workflow/compiler_jobs.go—buildPushEvalsJobAndAddwired intobuildMemoryManagementJobs;ensureConclusionIsLastJobpicks uppush_evalsautomaticallyExample frontmatter
The evals job reuses
IsDetectionRun: trueto avoid introducing a newIsEvalsRunfield — the AWF sandbox behavior and model defaults are identical for binary Q&A tasks.pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/28776499241