Repository navigation
feat(results): offload inline base64 payloads to an artifact sink - #138
abhinav-pola wants to merge 3 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
Original prompt from Abhinav
|
There was a problem hiding this comment.
Note: this review was submitted as a COMMENT instead of an APPROVE — the
maintainer app's approval POST was rejected with 403 on this repo
(pull_requests:write missing), so this verdict does not satisfy branch
protection and a human approval is still required. The assessment and
findings are unchanged.
Perry's Review
Verdict: ✅ LGTM
Well-tested, well-scoped change — making the pointer a pure function of the content path (uriFor(artifact.path)) is the right design and cleanly sidesteps payload-keyed dedup bugs. CI is green (validate, CodeQL, Devin review) and the suite passes on head per the PR's proof matrix.
Details
Risk: 🟡 Medium
Risk assessment:
| Dimension | Severity | Risk | Reasoning |
|---|---|---|---|
| Implementation risk | 🟨🟨 | Medium | Upload-failure path can leave orphan pointers in a surviving result; bare file_data typing is shape-dependent |
| Premise risk | 🟩 | Low | 156 rows over BigQuery's 100 MB row limit from inline images is consistent with the described dataset |
| Estimated impact | 🟨🟨 | Medium | A wrong offload silently corrupts persisted benchmark trajectory rows for every run written after this lands |
| Risk Factor | Severity | Risk | Reasoning |
|---|---|---|---|
| Reversibility | 🟩 | Low | Parquet output is additive; re-running with no sink restores byte-identical inline output |
| Detectability | 🟥🟥 | High | A broken pointer looks like a normal gs:// string until ECO-5007 tries to render it |
| Blast radius | 🟨🟨 | Medium | Every trajectory row written with a sink attached, once ECO-5005 wires the GCS sink in |
| Data integrity | 🟨🟨 | Medium | The persisted trajectory columns are exactly what this change rewrites |
| Financial exposure | None | No billing surface | |
| Security and privacy exposure | None | Contents are benchmark payloads the harness already holds | |
| Propagation | 🟨🟨 | Medium | Mission Control (ECO-5007) will render whatever these columns contain |
| Availability | None | Harness-only write path | |
| Recovery cost | 🟩 | Low | Re-run the affected benchmark to regenerate rows |
| Time to correct | 🟩 | Low | The offload is centralized in one module |
Three findings, all worth resolving before ECO-5005 wires a real GCS sink in — none block this PR. The first should be answered rather than deferred: once ECO-5005 lands, a row written with a pointer to an object that was never uploaded is silent data corruption in the results bucket.
src/results/artifacts.ts:56— a payload that exhausts upload retries has already been replaced by itsgs://pointer in the returned result; the contract "a pointer in a surviving row always resolves" currently holds only viarunBenchmarkByIdturning the failure intoLeft.src/results/artifacts.ts:111— barefile_datais typedapplication/octet-streamunconditionally, and unknown media subtypes flow straight into object extensions; worth verifying against the real request-body shapes this harness produces.src/results/artifacts.ts:44— identical bytes under aliased content-type labels hash to the same object but derive different extensions, so identical bytes upload twice under two paths.
The rest of the risk framing: this is offload-only — scoring runs on the in-memory result before the store, so scores are unaffected, and with no sink the output is unchanged (covered by test). Because runBenchmarkById now returns Left for ArtifactUploadError, a chunk fails rather than persisting inline payloads — that is the intended discipline and the test covers it; the gap is only for callers that would swallow the error in future code paths.
TL;DR
Add
offloadInlineBase64and anArtifactSinkinterface so a result store can swap every inline base64 payload in sample rows for a plaings://pointer beforerunResultToParquetbuilds thetrajectoryandmessagescolumns. PR 1 of ECO-4986.Show me
Visualization
Artifact: https://openrouter.devinenterprise.com/api/presigned_proxy?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjpudWxsLCJidWNrZXRfbmFtZSI6ImRldmluYXR0YWNobWVudHMiLCJidWNrZXRfa2V5IjoiYXR0YWNobWVudHNfcHJpdmF0ZS9jbGVyay1vcmdfMmhUSzBmbWdTSk80ZncwT041T1NwOUtFZG1iLzY1YjFmZDE4LWM5YzQtNDMzZC1hZGFiLTdkMjI2NmQ0YzVkNyIsImlhdCI6MTc5MTQ5MTY5NSwiZXhwIjoxNzkyMDk2NDk1LCJvcmdfaWQiOiJjbGVyay1vcmdfMmhUSzBmbWdTSk80ZncwT041T1NwOUtFZG1iIiwiZmlsZW5hbWUiOiJ0cmFqZWN0b3J5LWFydGlmYWN0LXBvaW50ZXJzLTIwMjYtMTAtMDguaHRtbCJ9.s3VCqnuRMZu_a7iF1cTfnRGrVXwAKH_IDaQBitpD5b8
Before / after
n/a (no user-facing surface in this repo; Mission Control rendering is ECO-5007).
What changed?
src/results/artifacts.ts(new, exported as@openrouter/bench-harness/artifacts):InlineArtifact,ArtifactSink { uriFor, put },ArtifactUploadError,offloadInlineBase64. Only data URLs and the known base64 fields are touched; plain text, hashes, ids, remote URLs anddatafields without a mime type stay as they are. Each occurrence keeps its own pointer (no dedup ofagent_end.messages); identical bytes upload once because the object name is the content hash.makeLocalResultStoretakes an optionalartifactSink. Without one, output is unchanged.ResultStoreService.writeerror channel is nowArtifactUploadError;runBenchmarkByIdreturnsLeftfor it, so the chunk fails instead of persisting inline payloads.Why?
ECO-4986: 156 rows are at or above BigQuery's 100 MB row limit (largest 219 MB), almost all inline images. Pointers stay plain strings, so
image_url.url, ATIFsource.pathand the parquet/BigQuery schemas keep their types.Follow-ups, each its own PR: ECO-5005 GCS sink in openrouter-web
gcs-artifacts.ts(results bucket), ECO-5006 Kepler trials, ECO-5007 Mission Control rendering, ECO-5008 docs and row-size validation.Proof matrix
Same test files run on
origin/main(bd5c6d0) and on this branch.gs://pointerartifacts.test.tsresult-store.test.tswrites real parquet, reads backmessagesand ATIFtrajectory{data,mimeType}, Anthropicsource,input_audio,file_data) offloaded with content type and extensionartifacts.test.tsartifacts.test.tsartifacts.test.tsArtifactUploadError, nothing inlineartifacts.test.tsrun-by-id.test.tsreturnsLeftfrom a realrunBenchmarkByIdartifacts.test.tsresult-store.test.tsrun-by-id.test.tsBase run: 1 pass, 3 fail (
artifacts.test.tsandrun-by-id.test.tsfail to import./artifacts; the sink case inresult-store.test.tsfails ontoHaveLength(1)). Head: 21 pass, 0 fail. Full suite on head: 1653 pass, 0 fail.Thermo-nuclear fix loop
ArtifactSink.uriFor), so one pass replaces and collects; removed the map and second walk.replaceBase64, removed comments the repo lint bans; no other findings.How to test
bun run checkexits 1 onorigin/mainas well (existing warnings); it reports no errors in the changed files.Benchmark impact
None on scores: scoring runs on in-memory results before the store offloads anything. Runs without a sink write the same parquet as before.
Reviewer focus
bareBase64ContentType.runBenchmarkByIdnow returnsLeftonly forArtifactUploadError; other store failures still log and returnresultsPath: null.Checklist
Link to Devin session: https://openrouter.devinenterprise.com/sessions/128fd13a3d394295bd6c4b43a1431298
Open in Devin Desktop: https://openrouter.devinenterprise.com/desktop/session/128fd13a3d394295bd6c4b43a1431298?variant=devin
Requested by: @abhinav-pola