Skip to content

feat(results): offload inline base64 payloads to an artifact sink - #138

Open
abhinav-pola wants to merge 3 commits into
mainfrom
devin/1791490205-trajectory-artifacts
Open

abhinav-pola wants to merge 3 commits into
mainfrom
devin/1791490205-trajectory-artifacts

Conversation

@abhinav-pola

@abhinav-pola abhinav-pola commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

TL;DR

Add offloadInlineBase64 and an ArtifactSink interface so a result store can swap every inline base64 payload in sample rows for a plain gs:// pointer before runResultToParquet builds the trajectory and messages columns. PR 1 of ECO-4986.

Show me

makeLocalResultStore({ dir, artifactSink? }).write          src/results/result-store.ts
  offloadInlineBase64(result, artifactSink)                  src/results/artifacts.ts
    replaceBase64(sampleScores)
      "data:<mime>;base64,..."            (any string)
      { data, mimeType }                  (Ori / pi image parts)
      { type: "base64", media_type, data }(Anthropic source)
      input_audio: { data, format }       -> audio/<format>
      file_data (bare)                    -> application/octet-stream
        -> sha256(bytes) -> artifacts/sha256/<hex>.<ext>
        -> sink.uriFor(path) written in place of the base64
    forEach(unique paths, sink.put, retry exponential x4, concurrency 8)
      exhausted -> ArtifactUploadError
  runResultToParquet(result with pointers)                   unchanged
runBenchmarkById                                             src/runner/run-by-id.ts
  resultStore.write fails with ArtifactUploadError -> Either.left (chunk fails)
  other store failures                              -> Right, resultsPath null (unchanged)

Visualization

Artifact: https://openrouter.devinenterprise.com/api/presigned_proxy?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjpudWxsLCJidWNrZXRfbmFtZSI6ImRldmluYXR0YWNobWVudHMiLCJidWNrZXRfa2V5IjoiYXR0YWNobWVudHNfcHJpdmF0ZS9jbGVyay1vcmdfMmhUSzBmbWdTSk80ZncwT041T1NwOUtFZG1iLzY1YjFmZDE4LWM5YzQtNDMzZC1hZGFiLTdkMjI2NmQ0YzVkNyIsImlhdCI6MTc5MTQ5MTY5NSwiZXhwIjoxNzkyMDk2NDk1LCJvcmdfaWQiOiJjbGVyay1vcmdfMmhUSzBmbWdTSk80ZncwT041T1NwOUtFZG1iIiwiZmlsZW5hbWUiOiJ0cmFqZWN0b3J5LWFydGlmYWN0LXBvaW50ZXJzLTIwMjYtMTAtMDguaHRtbCJ9.s3VCqnuRMZu_a7iF1cTfnRGrVXwAKH_IDaQBitpD5b8

Call shape before and after

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 and data fields without a mime type stay as they are. Each occurrence keeps its own pointer (no dedup of agent_end.messages); identical bytes upload once because the object name is the content hash.
  • makeLocalResultStore takes an optional artifactSink. Without one, output is unchanged.
  • ResultStoreService.write error channel is now ArtifactUploadError; runBenchmarkById returns Left for 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, ATIF source.path and 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.

Behavior Unit Integration E2E base head
MMMU data URL image (any case) becomes a gs:// pointer artifacts.test.ts result-store.test.ts writes real parquet, reads back messages and ATIF trajectory n/a (no user flow in harness; native run path is ECO-5005) fail pass
Known bare fields ({data,mimeType}, Anthropic source, input_audio, file_data) offloaded with content type and extension artifacts.test.ts n/a (covered by same serializer path as above) n/a fail pass
Repeated Ori image: pointer at every event, one upload artifacts.test.ts n/a n/a fail pass
Upload retried, then pointer used artifacts.test.ts n/a n/a fail pass
Retries exhausted: ArtifactUploadError, nothing inline artifacts.test.ts run-by-id.test.ts returns Left from a real runBenchmarkById n/a fail pass
Text, hashes, remote URLs, impossible-length base64 untouched artifacts.test.ts n/a n/a fail (module missing) pass
No sink: inline output unchanged n/a result-store.test.ts n/a pass pass
Parquet write throwing still returns the run n/a run-by-id.test.ts n/a fail (file imports new module) pass

Base run: 1 pass, 3 fail (artifacts.test.ts and run-by-id.test.ts fail to import ./artifacts; the sink case in result-store.test.ts fails on toHaveLength(1)). Head: 21 pass, 0 fail. Full suite on head: 1653 pass, 0 fail.

Thermo-nuclear fix loop

  • Round 1: two passes over the tree plus a payload-keyed map to look up sink-returned URIs. Fixed by making the pointer a function of the content path (ArtifactSink.uriFor), so one pass replaces and collects; removed the map and second walk.
  • Round 2: clean. Deslop: renamed the walker to replaceBase64, removed comments the repo lint bans; no other findings.

How to test

bun test src/results/artifacts.test.ts src/results/result-store.test.ts src/runner/run-by-id.test.ts
bun run format:check && bun run typecheck && bun test && bun run build

bun run check exits 1 on origin/main as 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

  • The set of fields treated as base64 in bareBase64ContentType.
  • runBenchmarkById now returns Left only for ArtifactUploadError; other store failures still log and return resultsPath: null.

Checklist

  • Tests cover changed behavior
  • Public API or configuration changes are backward compatible, or the break is documented
  • Benchmark changes document dataset provenance and licensing
  • No credentials, private results, or restricted dataset contents are included
  • Documentation is updated where needed

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


Devin Review

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

  • Disable automatic comment, CI, and merge conflict monitoring

Original prompt from Abhinav

SYSTEM:
<latest_message>
abhinav-intern (U090K0G7JF3) [ts=1791490041.175479]: @Devin !ship_with_proof implement <https://linear.app/openrouter/issue/ECO-4986%7CECO-4986|https://linear.app/openrouter/issue/ECO-4986|ECO-4986>: store benchmark trajectory base64 payloads as GCS artifacts and keep only a pointer in the sample row

scoping is done in &lt;<https://openrouter.devinenterprise.com/sessions/f14e2399047545baa5f06372aa10576f%7Cthis|https://openrouter.devinenterprise.com/sessions/f14e2399047545baa5f06372aa10576f|this> session&gt;, go with its plan (a plain gs:// string in place of the base64, hash-named files in the results bucket under the run prefix, mission control renders them through the existing gcs proxy)

decisions:
• just the base64 strings (data: urls + the known base64 fields, audio/file parts included). no dedup of agent_end.messages, no large-text offload, no backfill
• retry the upload, then fail the chunk. never fall back to inline
• results bucket, not argo-artifacts (90 day ttl)

start with PR 1 (benchmark-harness upstream). PR 1 uses ECO-4986; create a separate linear ticket for each later PR, assigned to me. open PRs ready for review, not draft, and assign abhinav-pola
</latest_message>

=== BEGIN THREAD HISTORY (in #brain-abhinav) ===
abhinav-intern (U090K0G7JF3) [ts=1791490041.175479]: @Devin !ship_with_proof implement <https://linear.app/openrouter/issue/ECO-4986%7CECO-4986|https://linear.app/openrouter/issue/ECO-4986|ECO-4986>: store benchmark trajectory base64 payloads as GCS artifacts and keep only a pointer in the sample row

scoping is done in &lt;<https://openrouter.devinenterprise.com/sessions/f14e2399047545baa5f06372aa10576f%7Cthis|https://openrouter.devinenterprise.com/sessions/f14e2399047545baa5f06372aa10576f|this> session&gt;, go with its plan (a plain gs:// string in place of the base64, hash-named files in the results bucket under the run prefix, mission control renders them through the existing gcs proxy)

decisions:
• just the... (1120 chars truncated...)

devin-ai-integration[bot]

This comment was marked as resolved.

@perry-the-pr-reviewer perry-the-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. src/results/artifacts.ts:56 — a payload that exhausts upload retries has already been replaced by its gs:// pointer in the returned result; the contract "a pointer in a surviving row always resolves" currently holds only via runBenchmarkById turning the failure into Left.
  2. src/results/artifacts.ts:111 — bare file_data is typed application/octet-stream unconditionally, and unknown media subtypes flow straight into object extensions; worth verifying against the real request-body shapes this harness produces.
  3. 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.

Comment thread src/results/artifacts.ts
Comment thread src/results/artifacts.ts
Comment thread src/results/artifacts.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant