feat: add BinEval evals support#43641
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…n custom RunsOn test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pelikhan
left a comment
pelikhan
left a comment
There was a problem hiding this comment.
@copilot the evals job is essentially the detection job with a different harness, different artifact name and output names. Parameterize the details job function to reuse code. The evals harness must also use awf, agent engine etc...
mark feature as experimental
| "type": "string", | ||
| "description": "Unique identifier for the evaluation question." | ||
| }, | ||
| "question": { |
There was a problem hiding this comment.
@copilot add support for a different model per question
There was a problem hiding this comment.
Added an optional model field to each question item in both the shorthand list and extended object forms. The per-question override takes precedence over the top-level model field.
| } | ||
| } | ||
| }, | ||
| "engine-config": { |
There was a problem hiding this comment.
@copilot remove config, use same config as top level engine config
Add model field to setup default model for all questions
There was a problem hiding this comment.
Removed engine-config and replaced it with a simple model string field at the top level of the extended form (same string type as the engine model field). Done in the schema, EvalsConfig, and the frontmatter parser.
| ) | ||
|
|
||
| // defaultEvalsModel is the default LLM used for evaluations when no engine-config is provided. | ||
| const defaultEvalsModel = "openai/gpt-4o-mini" |
There was a problem hiding this comment.
Changed defaultEvalsModel to "small" so the runtime picks an appropriate small/cheap model.
| const defaultEvalsModel = "openai/gpt-4o-mini" | ||
|
|
||
| // defaultEvalsModelEndpoint is the GitHub Models inference endpoint. | ||
| const defaultEvalsModelEndpoint = "https://models.github.ai/inference/chat/completions" |
There was a problem hiding this comment.
Removed the defaultEvalsModelEndpoint Go constant. The endpoint is now defined as GITHUB_MODELS_ENDPOINT inside run_evals_harness.cjs.
| // buildEvalsHarnessScript returns the JavaScript body that drives the evaluation harness. | ||
| // The script is injected into the github-script action's `script:` field. | ||
| func buildEvalsHarnessScript(endpoint string) string { | ||
| return `const fs = require('fs'); |
There was a problem hiding this comment.
@copilot refactor into a JavaScript file in actions/setup/js
There was a problem hiding this comment.
Extracted to actions/setup/js/run_evals_harness.cjs. The step now requires it via the setup_globals + require() pattern used by other downstream jobs (e.g. setup_threat_detection.cjs).
| fmt.Fprintf(&step, " uses: %s\n", getCachedActionPin("actions/github-script", data)) | ||
| step.WriteString(" with:\n") | ||
| step.WriteString(" script: |\n") | ||
| step.WriteString(" const fs = require('fs');\n") |
There was a problem hiding this comment.
Extracted to actions/setup/js/render_evals_summary.cjs. The step uses the same setup_globals + require() pattern.
🤖 PR Triage
Score breakdown:
Recommendation: Defer — PR is in DRAFT state. Contains significant new feature (+1175/-7 in 11 Go files) adding binary eval support to workflow frontmatter. Hold for author to mark ready and CI to pass.
|
- Mark evals as experimental in schema description - Add per-question model override field to EvalDefinition - Replace engine-config with top-level model field in EvalsConfig/schema - Use $ref: github_actions_runs_on for evals.runs-on schema field - Change default model hint from openai/gpt-4o-mini to small - Remove defaultEvalsModelEndpoint constant - Extract harness JS to actions/setup/js/run_evals_harness.cjs - Extract summary JS to actions/setup/js/render_evals_summary.cjs - Update steps to require CJS modules via setup_globals pattern - Fix lint warnings in new JS files (getErrorMessage, String(), try/catch) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add comment to evalQ.Model field explaining per-question override - Log count of per-question model overrides in parseEvalsFromFrontmatter - Restructure parseAnswer nested ternary into explicit if/else - Add indeterminate field to parseAnswer return to avoid calling it twice - Reuse parseAnswer result instead of calling it twice in main loop Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the actionable parts of this feedback:
The deeper "use AWF/agent engine for execution" and full detection-job code-share refactor would require a more substantial rework of the harness architecture — happy to do that as a follow-up once the feature direction is confirmed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
Hey The PR is focused, well-described, and ships with solid test coverage for both the config parser and the job builder ( This looks ready for review. 🟢
|
Only keep JSON schema changes, compiler support. |
…remove JS harness files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds initial plumbing for “BinEval”-style binary evaluation questions in workflow frontmatter, including schema validation, parsing/validation, and compiler wiring. The implementation currently stops short of actually emitting an evals job (the job builder is a stub), so the feature is effectively configuration-only with an “experimental/pending” warning.
Changes:
- Introduces
evalsfrontmatter support: schema + raw frontmatter field + parsedEvalsConfigonWorkflowData. - Adds
EvalsConfigparsing/validation logic and unit tests. - Adds evals-related job/artifact/branch constants and wires the compiler to attempt building an evals job (currently a no-op).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_data.go | Adds WorkflowData.Evals to hold parsed evals configuration. |
| pkg/workflow/frontmatter_types.go | Adds raw evals frontmatter field (any) with explanatory comment. |
| pkg/workflow/evals_job.go | Adds placeholder buildEvalsJob stub (currently returns nil). |
| pkg/workflow/evals_config.go | Adds EvalsConfig / EvalDefinition types and frontmatter parsing + validation helpers. |
| pkg/workflow/evals_config_test.go | Adds unit tests for evals parsing, validation, and helpers. |
| pkg/workflow/compiler_orchestrator_workflow.go | Parses evals config during orchestration and emits an experimental warning when configured. |
| pkg/workflow/compiler_jobs.go | Wires buildEvalsJob() into job build sequence (currently no-op). |
| pkg/parser/schemas/main_workflow_schema.json | Adds evals schema (shorthand + extended forms) and includes various string/emoji normalization changes. |
| pkg/constants/job_constants.go | Adds evals job name + artifact/filename/branch constants and marks evals as a built-in job name. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 7
- Review effort level: Low
| func evalsBranchName(workflowID string) string { | ||
| return "evals/" + workflowID | ||
| } |
| // EvalsBranchPrefix is the git branch prefix for persisting evaluation results. | ||
| // Results are stored in branches named evals/<workflow-id>/evals.jsonl. | ||
| const EvalsBranchPrefix = "evals" |
| var KnownBuiltInJobNames = map[string]struct{}{ | ||
| string(AgentJobName): {}, | ||
| string(ActivationJobName): {}, | ||
| string(PreActivationJobName): {}, | ||
| string(PreActivationHyphenJobName): {}, | ||
| string(DetectionJobName): {}, | ||
| string(EvalsJobName): {}, | ||
| string(SafeOutputsJobName): {}, | ||
| string(SafeOutputsHyphenJobName): {}, | ||
| string(UploadAssetsJobName): {}, |
| "evals": { | ||
| "description": "⚠️ Experimental. BinEval binary evaluation questions to run after safe-outputs and before the conclusion job. Can be a plain list of questions (shorthand) or an object with questions, model, and runs-on fields.", | ||
| "oneOf": [ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (597 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (20 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness issues that produce misleading errors to users, plus a semantic mismatch (full validation + warning but zero output) that needs a clear policy decision before shipping.
### Issues requiring changes
Correctness — silent failures in parser (2 issues):
questions:with a wrong type (non-list) silently falls through to"at least one question required"— the real cause is completely hidden- Non-string
model:values are silently discarded at both the config and per-question levels, leaving the user on the default model with no diagnostic
Semantic contract (1 issue):
- The compiler validates evals config fully and emits a warning, but
buildEvalsJobis a permanent(nil, nil)stub. A user who resolves all validation errors still gets a compiled workflow with zero evals logic and no further signal. This needs either: (A) skip validation+warning until the job is real, or (B) make it a hard compile error when evals are declared.
Stale documentation (2 issues):
parseEvalsFromFrontmattergodoc showsengine-config:which the parser never reads — users following the example will get a confusing errorFrontmatterConfig.Evalscomment still references the removedengine-configoverride
Low / advisory:
EvalDefinitionyaml struct tags describe an unmarshal path that doesn't exist
🔎 Code quality review by PR Code Quality Reviewer · 143.9 AIC · ⌖ 5.7 AIC · ⊞ 5.4K
Comment /review to run again
| if err != nil { | ||
| return nil, fmt.Errorf("evals.questions: %w", err) | ||
| } | ||
| cfg.Questions = questions |
There was a problem hiding this comment.
Silent type failure for non-list questions value swallows the real error — if questions is present but not a []any (e.g. a string, map, or integer), the inner type assertion fails silently, cfg.Questions stays nil, and the user gets "at least one question is required" with no indication that their questions field has the wrong shape.
💡 Suggested fix
Add an explicit else branch after the inner type assertion:
if questionsRaw, ok := v["questions"]; ok {
if questionsList, ok := questionsRaw.([]any); ok {
questions, err := parseEvalDefinitions(questionsList)
if err != nil {
return nil, fmt.Errorf("evals.questions: %w", err)
}
cfg.Questions = questions
} else {
return nil, fmt.Errorf("evals.questions: must be an array, got %T", questionsRaw)
}
}The current code path reaches validateEvals with an empty questions slice and produces a misleading "at least one question is required" error that completely obscures the root cause.
| // - id: builds | ||
| // question: Does the generated code compile? | ||
| // engine-config: | ||
| // model: gpt-4o-mini |
There was a problem hiding this comment.
Godoc example shows engine-config key that the parser never reads — the comment documents engine-config: model: gpt-4o-mini as valid frontmatter, but the implementation was updated to read model directly at the top level of the extended object. Any user following this example will get "at least one question is required" with no useful error.
💡 Suggested fix
Update the godoc to match the actual schema:
// Extended form:
//
// evals:
// questions:
// - id: builds
// question: Does the generated code compile?
// model: openai/gpt-4o-mini # optional, overrides per-question
// runs-on: ubuntu-latest # optional runner override
Review SummaryReviewed using 📋 Issues to address (4 inline comments)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on 4 focused issues.
📋 Key Themes & Highlights
Key Issues (by priority)
- Doc/schema mismatch on
engine-config— the Go doc comment and PR description showengine-config: { model: ... }but both the schema and the parser use a top-levelmodelkey. Users following the example will silently get no model override. - Constant not used in
evalsBranchName—EvalsBranchPrefixis declared in job_constants.go but the function hardcodes"evals/"instead. - Schema missing
required: ["questions"]on extended form — an emptyevals: { model: gpt-4o }passes JSON Schema but fails Go validation; the schema should catch this earlier. Also missingminItems: 1on both array forms. - Silent data-loss when
questionsis wrong type — ifquestions:is not an array, the parser silently produces 0 questions with no error.
Positive Highlights
- ✅ Comprehensive test suite (211 lines) — well-structured and covers both shorthand and extended forms
- ✅ Clean separation into config parsing, validation, and job assembly phases
- ✅ Nil-safe
HasEvals()method and consistent pointer-receiver pattern - ✅ Whitespace trimming in ID and question fields prevents subtle validation bypasses
- ✅ Good use of experimental warning +
IncrementWarningCount()to signal non-production readiness
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 86.7 AIC · ⌖ 5.72 AIC · ⊞ 6.7K
Comment /matt to run again
| // evalsBranchName returns the git branch name used to persist evaluation results. | ||
| // Format: evals/<workflow-id> | ||
| func evalsBranchName(workflowID string) string { | ||
| return "evals/" + workflowID |
There was a problem hiding this comment.
[/codebase-design] evalsBranchName hardcodes the prefix string instead of using the EvalsBranchPrefix constant defined for this exact purpose — a silent divergence risk if the prefix is ever changed.
💡 Suggested fix
func evalsBranchName(workflowID string) string {
return constants.EvalsBranchPrefix + "/" + workflowID
}Import github.com/github/gh-aw/pkg/constants and reference the constant, so that both places stay in sync automatically.
@copilot please address this.
| // - id: builds | ||
| // question: Does the generated code compile? | ||
| // | ||
| // # Extended — object with questions list and optional engine-config |
There was a problem hiding this comment.
[/grill-with-docs] The doc comment shows engine-config:\n model: gpt-4o-mini but the schema and Go parser both use a top-level model key, not a nested engine-config object. Users following this example will silently get no model override applied.
💡 Suggested fix
Update the doc comment to match the actual schema:
// # Extended — object with questions list and optional config
// evals:
// questions:
// - id: builds
// question: Does the generated code compile?
// model: openai/gpt-4o-mini # top-level, not under engine-config
// runs-on: ubuntu-latestThe PR description also repeats the same engine-config mismatch and should be updated.
@copilot please address this.
| } | ||
|
|
||
| func TestParseEvalsFromFrontmatter_InvalidType(t *testing.T) { | ||
| c := NewCompiler() |
There was a problem hiding this comment.
[/tdd] No test covers the extended form with a missing questions key — e.g. evals: { model: gpt-4o }. The parser silently skips the questions block and then validateEvals returns an error, but the error path is untested.
💡 Suggested test
func TestParseEvalsFromFrontmatter_ExtendedForm_NoQuestions(t *testing.T) {
c := NewCompiler()
_, err := c.parseEvalsFromFrontmatter(map[string]any{
"evals": map[string]any{"model": "openai/gpt-4o"},
})
require.Error(t, err)
assert.Contains(t, err.Error(), "at least one question")
}@copilot please address this.
|
|
||
| case map[string]any: | ||
| // Extended form: object with questions and optional model/runs-on | ||
| if questionsRaw, ok := v["questions"]; ok { |
There was a problem hiding this comment.
[/codebase-design] When questions is present in the extended form but is not a []any (e.g. a single string), the inner type assertion silently falls through and cfg.Questions remains nil — no error is returned. This is a silent data-loss path.
💡 Suggested fix
Add an else-error branch after the inner type assertion:
if questionsList, ok := questionsRaw.([]any); ok {
// ... existing parsing
} else {
return nil, fmt.Errorf("evals.questions: must be an array")
}@copilot please address this.
There was a problem hiding this comment.
BinEval evals support — review
The overall shape of this PR is solid: schema, parser, and validation are well-structured, test coverage is comprehensive, and the experimental stub approach (warn + no-op job builder) is a safe way to land the plumbing without breaking existing workflows.
Four actionable issues found:
-
Doc/schema mismatch (
evals_config.goline 58): theparseEvalsFromFrontmatterdoc comment still showsengine-config: model:as the extended syntax, but both the JSON schema and the parser acceptmodel:at the top level of the extended object. The schema also setsadditionalProperties: false, so the documented form would fail validation silently. -
Silent swallow of wrong
questionstype (evals_config.goline 77): when the extended form'squestionsvalue is not a[]any, the inner type assertion silently falls through and the downstreamvalidateEvalserror is generic. An explicit type check with a targeted message improves debuggability. -
Dead helper
evalsBranchName(evals_config.goline 167): this function is defined but never called — the job stub that would use it is a no-op. Go will not error on an unexported dead function, but it will drift from the final design. Either defer it to the job-implementation PR or anchor it explicitly. -
Unused
yaml:struct tags (evals_config.goline 175):EvalDefinitioncarriesyaml:tags that are never used — the struct is populated by manual map unpacking. These tags create a false impression about how unmarshalling works.
Items 1 and 2 are blocking (correctness / user-facing error quality). Items 3 and 4 are maintainability issues that should be addressed before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 100.8 AIC · ⌖ 6.28 AIC · ⊞ 4.9K
| // - id: builds | ||
| // question: Does the generated code compile? | ||
| // engine-config: | ||
| // model: gpt-4o-mini |
There was a problem hiding this comment.
The doc comment shows engine-config: model: gpt-4o-mini as the extended-form syntax, but the schema and parser both use a top-level model: key directly (not nested under engine-config:). The JSON schema also marks the extended object as additionalProperties: false, so using engine-config: would silently fail schema validation.
Update the comment to match the actual accepted syntax:
// # Extended — object with questions list and optional model/runs-on
// evals:
// questions:
// - id: builds
// question: Does the generated code compile?
// model: gpt-4o-mini
// runs-on: ubuntu-latest@copilot please address this.
|
|
||
| func TestValidateEvals_AcceptsValidQuestions(t *testing.T) { | ||
| cfg := &EvalsConfig{ | ||
| Questions: []EvalDefinition{ |
There was a problem hiding this comment.
TestValidateEvals_RejectsEmptyQuestion sets Question: " " (whitespace-only). The validator checks strings.TrimSpace(q.Question) == "", so this is valid test coverage for the validator path.
However, there is no test for the extended form where questions key is present but contains something other than a list (e.g., map[string]any{"questions": "not-a-list"}). As noted in the implementation comment, this silently falls through and produces a misleading error. A regression test here would pin the expected error message.
@copilot please address this.
|
@copilot please run the Unresolved review feedback to address:
Please also make sure the ADR requirement is satisfied in the PR body before final handoff. Run: https://github.com/github/gh-aw/actions/runs/28764674525
|
…ifactName 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>
Addressed all four items in commits
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/28768103968
|
…upport Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch merged with main ( |
Adds first-class BinEval-style evaluation support: workflows declare atomic binary YES/NO questions in frontmatter, which are evaluated by a small LLM after
safe_outputscompletes, with results stored as JSONL artifacts and persisted to a git branch for historical comparison.Schema
Extended form with model override:
Changes
Types & schema
pkg/constants/job_constants.go—EvalsJobName,EvalsArtifactName,EvalsResultFilename,EvalsBranchPrefixmain_workflow_schema.json—evalsproperty with shorthand (list) and extended (object withengine-config/runs-on) formsFrontmatterConfig—Evals anyraw field;WorkflowData—Evals *EvalsConfigparsed fieldConfig parser (
evals_config.go)EvalDefinition/EvalsConfigstructs;parseEvalsFromFrontmatter()handles both formsevalsBranchName()→evals/<workflow-id>Job builder (
evals_job.go,evals_steps.go)buildEvalsJob()—evalsjob positioned aftersafe_outputs, before conclusion; depends onagent+activation+safe_outputs(when present);ensureConclusionIsLastJob()auto-wires the tail dependencygithub-script→ GitHub Models API (defaultopenai/gpt-4o-mini, configurable) → step summary Markdown table →evalartifact upload → git branch persistence toevals/<workflow-id>/evals.jsonlYES/NOline is captured as rationalePipeline wiring
buildJobs()callsbuildEvalsJob()afterbuildSafeOutputsJobs()extractAdditionalConfigurations()callsparseEvalsFromFrontmatter()