Harden benchmark workflow execution - #171
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
75e1ed6 to
1511e44
Compare
|
@bmsaadat the first default-branch dispatch exposed two runtime issues: PR-commenting could stop the job, and the separately configured project name conflicted with the project ID embedded in the benchmark key. This patch removes that duplicate pin, fails closed on all-ungraded arms, and prevents publishing them. I reproduced the original setup failure locally, then reran the same task without the pin: MCP initialized, interception succeeded, verifier rewards were written, and cleanup completed. All checks and BugBot are green. |
bmsaadat
left a comment
There was a problem hiding this comment.
Approve with one ask and a few optional notes.
This is a solid hardening pass. The guard sits before the experiment POST so no orphan experiments, the permission addition looks like the real fix for the comment failures, and the validation (live Harbor run with the pin removed) is convincing.
The one ask: removing KERNEL_PROJECT also removes the only fail-closed check that the benchmark key is actually project-scoped. The server still implements the pin (auth-context.ts, the server_pin test), and projectForOperation enforces whatever scope the connection resolves to. So if the key is ever rotated to an org-scoped credential by mistake, benchmark agents get org-wide tool access with every check green, where the pin previously contained them to the eval project. I like the single-source-of-truth direction, so no need to bring the var back. A cheap preflight in run.sh or the workflow that asserts credential_scope.project_id is non-null before launching trials would keep it fail-closed.
Optional:
- The completeness rule now lives in three places (publish throw, report warning, jq string) plus a test pin. The jq line can't currently be the deciding check since the gate already requires publish success. Since
report.tsalready computesincompleteinternally, serializing acompleteflag and gating on that would keep the semantics in one tested place. - Does Harbor flush
result.jsonon SIGINT? If so, a timed-out arm with some graded trials still publishes a truncated experiment on a red run, since publish never sees exit statuses. - Since one unscored arm now blocks all publication and the workflow uploads no artifacts, an all-infra weekly run leaves little evidence. Uploading the Harbor job dirs when the gate fails would cover that.
Nit: the new ungraded locals in report.ts/publish-braintrust.ts collide with ArmSummary.ungraded, which excludes infra trials (an all-infra arm has scored === 0 but ungraded === 0). unscored would avoid the ambiguity.
1511e44 to
19625f9
Compare
|
@bmsaadat addressed the project-scope ask in |
bmsaadat
left a comment
There was a problem hiding this comment.
Approved, thanks for the quick turnaround. The preflight is exactly what I was hoping for: fail-closed on org-scoped keys, the credential/effective match is a nice extra, and nothing from the response gets printed.
One non-blocking note: zod isn't a direct dependency of this repo (it's only in bun.lock transitively), and the workflow only runs bun install in the harness checkout, so this script will lean on Bun's runtime auto-install when run.sh executes in the candidate/baseline checkouts. It can only fail loudly (aborting the run), so no safety concern, but since it's just two string checks, plain typeof checks would make the script dependency-free; bun add zod works too.
Summary
Validation
bun test(262 tests)bunx tsc --noEmit --incremental falsebun run buildwith required placeholder configurationgit diff --check