Compare Workshop evals on pull requests - #476
Conversation
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| observedCumulativeChatCostUsd: z.number().nonnegative().optional(), | ||
| }).loose(), | ||
| }).loose(), | ||
| output: z.object({ metrics: MetricsSchema }).loose(), |
There was a problem hiding this comment.
[P1] Reject infrastructure-failed trajectories before comparing
The schema ignores run.errors, so harness exceptions are counted as ordinary failed task trials. This happened in this PR's own workflow run: all 18 trajectories contain AgentError/EvalRunError: Connection error, but every row was marked comparable: true and the neutral check reported 0% vs 0% plus apparent duration improvements. That turns a total eval outage into a normal no-regression result. Parse the harness errors (or another explicit infrastructure outcome) and mark affected cohorts/report as non-comparable instead of calculating pass-rate and performance deltas.
|
Posted 1 actionable inline finding. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| let reason: string | null = null; | ||
| if (base === undefined) reason = "missing baseline"; | ||
| else if (next === undefined) reason = "missing candidate"; | ||
| else if (base.taskVersion !== next.taskVersion) reason = "task version changed"; |
There was a problem hiding this comment.
🟡 Evaluator changes remain comparable
Changes to EVAL_AGENT_BUDGET_MS, resolveEvalModel, openLocalEvalTarget, or measureHistory leave definitionsChanged false. The report then attributes evaluator-induced result deltas to product behavior.
Learn more
The baseline and candidate each execute the evaluator code from their own revision. Several evaluator modules omitted from this path list control time budgets, model resolution, target setup, and reported metrics. A change to any of them can alter pass rates or measurements without changing the product behavior under evaluation. The comparison still marks matching cohorts as comparable because the workflow emits changed=false.
Example: A pull request changes the budget constants from 60 seconds to 30 seconds. Candidate trials time out more often, while baseline trials retain 60 seconds. The check reports a pass-rate regression instead of eval definition changed.
Recommended fix: Treat every evaluator implementation file that can affect outcomes as a definition change. At minimum include budgets.ts, config.ts, target.ts, and transcript.ts; preferably maintain one inclusive evaluator path and explicitly exclude only reducer and test files.
Was this helpful? React with 👍 or 👎 to provide feedback.
79348b1 to
f3b1ef6
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| else if (base.assertions.some(assertion => | ||
| assertion.meta.harness.run.errors.length > 0)) reason = "baseline run errors"; | ||
| else if (next.assertions.some(assertion => | ||
| assertion.meta.harness.run.errors.length > 0)) reason = "candidate run errors"; |
There was a problem hiding this comment.
🟡 Agent failures suppress pass-rate regressions
When a trial records AgentError or AgentTimeout, errors marks its entire cohort incomparable. createEvalHarness stores agent outcomes beside harness failures. Candidate failures then disappear from the pass-rate delta.
Learn more
The harness uses one errors array for two different categories. EvalRunError and EvalCleanupError describe infrastructure failures that invalidate a trial. AgentError and AgentTimeout describe how the evaluated agent ended and already make that trial fail. Rejecting every nonempty array therefore removes genuine agent failures from comparison.
Example: The baseline passes three trials. The candidate passes two trials and times out once. The candidate cohort becomes “candidate run errors” instead of reporting a -33.3 pp pass-rate delta.
Recommended fix: Mark a cohort incomparable only when an error name denotes a harness or cleanup failure. Keep AgentError and AgentTimeout in the comparison as failed model trials.
Was this helpful? React with 👍 or 👎 to provide feedback.
| permissions: | ||
| contents: read | ||
| checks: write | ||
| pull_request: |
There was a problem hiding this comment.
[P2] Preserve the manual eval entry point
This replaces workflow_dispatch outright, so merging the PR removes the existing way to run Workshop evals on demand and removes its models, trials, and reportOnly controls. Those manual runs are actively used (there are recent workflow_dispatch runs on this workflow), and adding PR comparisons does not require dropping them. Keep the existing dispatch/manual job, or put the PR-only flow in a separate workflow.
|
Posted 1 actionable inline finding. |
f3b1ef6 to
2cf0dcb
Compare
2cf0dcb to
3c8d621
Compare
| - revision: baseline | ||
| sha: ${{ github.event.pull_request.base.sha }} | ||
| - revision: candidate | ||
| sha: ${{ github.event.pull_request.head.sha }} |
There was a problem hiding this comment.
[P2] Compare revisions that isolate the PR
base.sha is the current tip of the target branch, but head.sha can be based on an older commit. After main advances, the baseline therefore contains base-only product changes that the candidate does not, and this report attributes their effect to the PR (often as a false regression). The three-dot definition check has the same asymmetry: it ignores base-only evaluator changes even though the baseline executes them. Evaluate the current base against the PR merge revision, or the merge base against the raw head, and run the definition check as a two-revision diff between those exact evaluated revisions.
|
Posted 1 actionable inline finding. |
|
[P2] |
3c8d621 to
cb4d71b
Compare
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, edited] |
There was a problem hiding this comment.
[P2] Keep irrelevant edits out of the eval concurrency group
A title/body edit still triggers this workflow (the PR's changed files satisfy paths) and enters the same concurrency group, even though trust then skips every job. Setting cancel-in-progress to false only preserves a running run: GitHub's default concurrency queue permits one pending run and replaces it with the newest. Thus, if a synchronize/base-change eval is pending while a prior run is being canceled, a subsequent title/body edit can evict that eval and leave no comparison for the latest code. Give non-base edits a distinct/unique concurrency group or otherwise prevent them from entering this workflow.
|
Posted 1 actionable inline finding. |
52f4704 to
31fd93b
Compare
31fd93b to
1a9a280
Compare
| jobs: | ||
| trust: | ||
| name: Verify pull request | ||
| if: >- |
There was a problem hiding this comment.
[P1] Keep the secret authorization gate outside PR-controlled YAML
This condition is loaded from the pull request revision, so a same-repository bot PR can remove or relax it and then run candidate-controlled pnpm evals with WORKSHOP_EVAL_CLOUDFLARE_API_TOKEN. That defeats the explicit bot exclusion and grants a compromised automation account a Cloudflare credential it otherwise cannot read. Run the authorization/secret-bearing flow from the protected default-branch workflow (for example, a carefully gated pull_request_target or workflow_run flow) before checking out and executing the approved candidate SHA.
|
Posted 1 actionable inline finding. |
| EXPECTED_TRIALS: ${{ env.EVAL_TRIALS }} | ||
| EVAL_MODELS: ${{ env.EVAL_MODEL }} | ||
| run: | | ||
| tasks=(packages/workshop-evals/evals/*.eval.ts) |
There was a problem hiding this comment.
[P2] Count the same eval files that Vitest runs
This glob only counts top-level evals/*.eval.ts, while vitest.eval.config.ts runs evals/**/*.eval.ts. As soon as a valid eval is organized in a subdirectory, pnpm evals includes it but expected_cases does not, so every main baseline refresh fails the reusable-baseline check and no new baseline artifact is published. Derive the expected cohorts from the report or use a recursive file count matching Vitest.
|
Posted 1 actionable inline finding. |
1 participant
Run the existing Workshop trajectory suite when agent-relevant code changes. Relevant pushes to
mainrefresh the stored baseline artifact; pull requests reuse the latest unexpiredmainartifact and run the exact base revision only when no baseline is available. Candidate runs use the same pinned Workers AI model and retain three raw trajectories per task.A deterministic reducer compares matching task/model cohorts and publishes one neutral Check. Harness errors, incomplete runs, changed eval definitions, and partially observed costs are reported as non-comparable. The existing manual eval workflow remains unchanged. This adds no gating or eval tasks.
One-time repository setup is required before this is ready:
WORKSHOP_EVAL_CLOUDFLARE_API_TOKEN,WORKSHOP_EVAL_ACCOUNT_ID, andWORKSHOP_EVAL_AI_GATEWAYmust point to a dedicated same-account Gateway without a wildcard Access policy. The current preview/staging policy intercepts Wrangler's remote binding endpoint.