Persist evals to evals/* branches and make audit/logs branch-aware#45492
evals/* branches and make audit/logs branch-aware#45492Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
evals/* branches and make audit/logs branch-aware
There was a problem hiding this comment.
Pull request overview
Adds branch-backed eval persistence and fallback lookup for logs and audit.
Changes:
- Adds and wires the
push_evals_stateworkflow job. - Generalizes experiment-state pushing for eval files.
- Adds shared branch-file retrieval and eval hydration.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/jobs.go |
Registers the eval persistence job. |
pkg/workflow/evals_job.go |
Builds the eval branch push job. |
pkg/workflow/compiler_jobs.go |
Wires persistence into compilation and conclusion. |
pkg/workflow/compiler_jobs_test.go |
Tests job generation and dependencies. |
pkg/workflow/compiler_experiments.go |
Shares state-branch naming logic. |
pkg/cli/logs_run_processor.go |
Attempts branch hydration during log processing. |
pkg/cli/logs_orchestrator_filters.go |
Adds branch fallback to eval filtering. |
pkg/cli/experiments_command.go |
Reuses the branch-file reader. |
pkg/cli/evals_branch.go |
Implements eval branch lookup and hydration. |
pkg/cli/branch_file_reader.go |
Adds shared remote branch-file retrieval. |
pkg/cli/audit.go |
Adds branch fallback to audit filtering. |
actions/setup/js/push_experiment_state.cjs |
Generalizes signed state persistence. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Medium
| args := []string{"api", | ||
| "repos/{owner}/{repo}/contents/" + filePath, | ||
| "--field", "ref=" + branchName, | ||
| "--jq", ".content", | ||
| "--repo", repoOverride, | ||
| } |
| return false | ||
| } | ||
|
|
||
| decoded, err := readRemoteRepoBranchFileContext(ctx, repoOverride, branchName, constants.EvalsResultFilename, host) |
| workflowID := workflowIDFromRunPath(run.WorkflowPath) | ||
| if workflowID == "" { | ||
| return false |
| // Apply evals filtering if --evals flag is specified. | ||
| if opts.evalsOnly { | ||
| if !runHasEvals(result.LogsPath, verbose) { | ||
| if !runHasEvals(result.LogsPath, verbose) && !ensureEvalsResultsFromBranch(context.Background(), result.Run, result.LogsPath, "", "", "", verbose) { |
|
@copilot please run the
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Persist evals to evals/* branches
The PR is well-structured and the generalization of push_experiment_state.cjs is clean. However, there are blocking correctness and reliability issues already flagged in the inline comments that need to be addressed before merging.
Blocking issues:
branch_file_reader.goline 24 —--field ref=<branch>causesgh apito send a POST instead of GET to the contents endpoint, breaking all branch reads. Use--raw-fieldor append?ref=<branch>as a query parameter.\n\n2.evals_branch.goline 33 — The branch stores only the latest evals run; fetching it for an older run silently returns wrong data. The branch-backed path needs to be run-scoped or limited to the current run.\n\n3.evals_branch.goline 25 —WorkflowPathis empty for runs discovered viagh run listwithout--json workflowPath, soworkflowIDFromRunPathreturns""and the branch lookup is silently skipped — defeating the feature for those runs.\n\n4.logs_orchestrator_filters.goline 138 —context.Background()discards the caller context; slow API calls won't respect cancellation or timeouts.\n\n5.logs_run_processor.goline 131 — Cache-hit path uses the freshrun.WorkflowPath(empty fromgh run list) instead ofsummary.Run.WorkflowPathfor the branch lookup.
Please address the blocking items — especially (1) which makes the entire feature non-functional — before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 33.2 AIC · ⌖ 5.03 AIC · ⊞ 5K
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (362 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on three issues.
📋 Key Themes & Highlights
Key Themes
- Job condition drops failed evals (line 185,
evals_job.go):push_evals_stateonly fires when evals succeed, so runs with partial failures never have their results persisted to the branch. - Duplicated constant (
evalsBranchPrefix): defined independently inpkg/workflowandpkg/cli; these can drift silently. Move topkg/constants. - Sanitization logic in
workflowIDFromRunPath: manually strips.yml/.yamlafterNormalizeWorkflowName, duplicating work done bySanitizeWorkflowIDForCacheKey. No unit tests cover this path.
Positive Highlights
- ✅ Clean reuse of
push_experiment_state.cjsvia genericGH_AW_STATE_*env vars — minimal new surface area. - ✅ Correct fallback chain (artifact → branch) wired consistently in both
auditandlogspaths. - ✅
WorkflowStateBranchNamecorrectly extracted as a shared utility. - ✅ New compiler tests cover both job creation and conclusion dependency wiring.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 48.6 AIC · ⌖ 4.7 AIC · ⊞ 6.7K
Comment /matt to run again
| BuildStringLiteral("success"), | ||
| ) | ||
| notCancelled := &NotNode{Child: BuildFunctionCall("cancelled")} | ||
| jobCondition := RenderCondition(BuildAnd(BuildAnd(BuildFunctionCall("always"), notCancelled), evalsSucceeded)) |
There was a problem hiding this comment.
[/codebase-design] The push_evals_state job condition requires evals == success, so results from failed evals runs are silently dropped and never persisted to the branch.
💡 Suggested fix
The push job should run whenever the evals job ran to completion (success or failure), not only on success. Consider:
evalsFinished := BuildNotEquals(
BuildPropertyAccess(fmt.Sprintf("needs.%s.result", constants.EvalsJobName)),
BuildStringLiteral("skipped"),
)
jobCondition := RenderCondition(BuildAnd(BuildAnd(BuildFunctionCall("always"), notCancelled), evalsFinished))This mirrors how push_experiments_state is conditioned in the experiments job — check that pattern for the canonical approach.
@copilot please address this.
| if workflowPath == "" { | ||
| return "" | ||
| } | ||
| base := filepath.Base(workflowPath) |
There was a problem hiding this comment.
[/codebase-design] workflowIDFromRunPath manually strips .yml/.yaml extensions after calling NormalizeWorkflowName, but NormalizeWorkflowName already removes .md and .lock.yml. The double-strip suggests this helper duplicates sanitization that is already done elsewhere (e.g. SanitizeWorkflowIDForCacheKey).
💡 Suggested fix
Check whether SanitizeWorkflowIDForCacheKey(filepath.Base(workflowPath)) already gives the correct result. If so, collapse this function:
func workflowIDFromRunPath(workflowPath string) string {
if workflowPath == "" {
return ""
}
return workflow.SanitizeWorkflowIDForCacheKey(filepath.Base(workflowPath))
}At minimum, add a test for .yaml and .yml inputs to document expected behaviour.
@copilot please address this.
|
|
||
| var evalsJobLog = logger.New("workflow:evals_job") | ||
|
|
||
| const evalsBranchPrefix = "evals" |
There was a problem hiding this comment.
[/codebase-design] evalsBranchPrefix = "evals" is defined in both pkg/workflow/evals_job.go and pkg/cli/evals_branch.go. This duplication means the two packages can drift independently.
💡 Suggested fix
Move the canonical constant to pkg/constants (where EvalsArtifactName, EvalsResultFilename, etc. already live) and import it from both packages:
// pkg/constants/constants.go
const EvalsBranchPrefix = "evals"Then in both files:
branchName := workflow.WorkflowStateBranchName(constants.EvalsBranchPrefix, workflowID)@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
i️ Test Details
Verdict
Summary: Both tests verify important design invariants—that push_evals_state jobs are created with the correct configuration and integrated into the workflow compilation pipeline. However, neither test covers error paths or edge cases, which limits their ability to catch unexpected failures. Test inflation is healthy (1.91:1, under 2:1 threshold). Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. 0% implementation tests (threshold: 30%). No violations detected.
Both new tests verify important design invariants for the evals persistence feature: the push_evals_state job is created with correct configuration and integrated into the workflow compilation pipeline. While edge cases and error paths could be more thoroughly tested, the tests provide solid coverage of the behavioral contracts.
There was a problem hiding this comment.
REQUEST_CHANGES — several correctness and durability bugs must be fixed
Five existing comments (from a prior round) plus three new ones below identify blocking issues. The most critical:
### Blocking issues summary
1. triggers POST, not GET () — the entire branch-backed read path is broken out of the box. encodes parameters as a POST body; the Contents endpoint only accepts GET. Use with or embed the in the URL as a query param.
2. Failed evals are never persisted () — the job is gated on , so failed/partial eval runs produce no branch entry. This defeats the durability purpose of the feature. Loosen to .
3. Branch file overwrites per-run evals with all-runs aggregate () — on the branch accumulates results for every run of the workflow. Writing it verbatim into a per-run corrupts per-run metrics and audit reports. Results must be filtered to the current run's ID before writing.
4. leaks in filter path () — passes an unscoped context for network I/O, breaking Ctrl-C/timeout cancellation.
**5. Cache-hit path uses wrong ** () — is empty on cache hits; should be used instead, or branch lookup silently fails and cache is unnecessarily bypassed.
6. Silent error regression () — refactoring removed the decode-error log; non-404 failures are now invisible.
7. poisons base64 decode () — stderr bytes mixed into decoded content cause confusing errors; use .
🔎 Code quality review by PR Code Quality Reviewer · 72.5 AIC · ⌖ 4.73 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/evals_job.go:195
Failed evals are never persisted to the branch: the job condition requires the evals job to succeed, so any run where evals fail or partially fail is silently skipped and no results land on the branch.
<details>
<summary>💡 Suggested fix</summary>
Loosen the condition to only exclude cancelled runs:
notCancelled := &NotNode{Child: BuildFunctionCall("cancelled")}
jobCondition := RenderCondition(BuildAnd(BuildFunctionCall("always"), notCancelled))The push script already handles…
pkg/cli/experiments_command.go:320
Silent error regression: the refactored readRemoteExperimentState silently discards all errors from readRemoteRepoBranchFile, including non-404 failures like network errors or malformed responses. The previous implementation logged decode errors explicitly.
<details>
<summary>💡 Suggested fix</summary>
Distinguish not-found (expected, silent) from unexpected errors:
decoded, err := readRemoteRepoBranchFile(repoOverride, branchName, "state.json", "")
if err != nil {
if !isRe…
</details>
<details><summary>pkg/cli/branch_file_reader.go:29</summary>
**`CombinedOutput()` merges stderr into the content bytes that are then base64-decoded**: if `gh api` writes any warning or diagnostic text to stderr before exiting, those bytes will be prepended to the file content and cause a spurious decode error that hides the real issue.
<details>
<summary>💡 Suggested fix</summary>
Use `cmd.Output()` (stdout only) and capture stderr separately for error reporting:
```go
var stderr bytes.Buffer
cmd.Stderr = &stderr
out, err := cmd.Output()
if err != nil…
</details>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
This change adds durable git-branch storage for eval results and reuses the existing experiments-state push path instead of introducing a separate persistence mechanism. It also updates
audit/logseval filtering to fall back to branch-backedevals.jsonlwhen artifacts are unavailable.Compiler/job graph updates
push_evals_stateas a built-in post-evals job.evals(and activation context), withcontents: write.push_evals_stateso branch persistence completes before finalization.evals/<sanitized-workflow-id>naming, aligned with existing workflow ID sanitization semantics.Refactor + reuse of state push logic
push_experiment_state.cjsviaGH_AW_STATE_*envs (DIR,BRANCH,FILES,LABEL) while preserving experiment aliases for backward compatibility.Branch-backed eval lookup in CLI
gh api repos/.../contents/...@ref.logsandauditeval filtering now hydrate localevals.jsonlfromevals/*branch when local artifact-derived eval files are missing.Targeted compatibility and wiring coverage
push_evals_statecreation and conclusion dependency wiring.