fix(critics): read instance_id from raw JSON in get_completed_instances

[juanmichelini]

Root-cause half of the swebenchmultimodal resume failure tracked as evaluation#515. The other half is evaluation#576 (revert of a wrapper-script workaround that made things worse); the two PRs together restore working resume for both fresh and stale archives.

Bug

get_completed_instances() calls EvalOutput.model_validate(data) on every line and silently swallows any exception:

try:
    data = json.loads(line.strip())
    output = EvalOutput.model_validate(data)
    completed_instances.add(output.instance_id)
except Exception as e:
    logger.warning(...)

When a resume run loads a partial archive written by an older SDK whose tool/observation event schema has since drifted (e.g. BrowserNavigateTool, BrowserObservation entries that the current pydantic models no longer recognise), every completed row hits the except branch and gets dropped. _get_instances_for_attempt() then computes all_instances - completed_in_attempt and concludes that nothing has been completed, so the iterative loop re-runs every selected instance from scratch.

Fix

Read instance_id directly from the parsed JSON dict. We do not need a fully validated EvalOutput to decide whether an instance was already processed — we just need its id. Malformed JSON, blank lines, and objects missing instance_id are still skipped with a warning.

get_failed_instances() legitimately needs the full model (to hand to critic.evaluate()), so it's left as is. On a stale archive it will find zero failed instances; combined with the now-correct completed set this produces an empty retry list — the conservative "trust the prior completion" choice, which matches what a resume is supposed to do.

Test

Added test_get_completed_instances_tolerates_stale_archive_schema which writes a JSONL with:

1. a stale-schema row with a BrowserNavigateTool history event that fails EvalOutput.model_validate
2. a fresh row written via the current EvalOutput
3. a blank line
4. a malformed JSON line
5. a JSON object missing instance_id

Asserts only rows 1 and 2 land in the returned set. The test fails on main (row 1 gets dropped, set is {"instance_fresh"}) and passes with this PR.

$ uv run pytest tests/test_iterative_resume.py -p no:cacheprovider
======================== 5 passed, 2 warnings in 0.46s ========================

Companion PR

• evaluation#576 — Revert evaluation#520; the --output-dir "$OUTPUT_DIR_PATH" line it added was nesting <dataset>/<model_folder> under an already-leaf path, so the restored archive was sitting at one path while the benchmark was looking at another. That bug also surfaces as fix: cache SDK sdist across image builds #515.

Either PR alone fixes one shape of the bug; landing both fixes the user-visible "swebenchmultimodal resume runs all 67 instances anyway" problem (e.g. eval-monitor run 27027881388) end-to-end.

cc @simonrosenberg @juanmichelini

---

This PR was created by an AI agent (OpenHands) on behalf of @juanmichelini.

@juanmichelini can click here to continue refining the PR

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Code Review

Taste Rating: 🟢 Good taste — Elegant, simple solution

Summary

Clean, well-scoped fix that solves the stale schema problem without adding complexity. The change is minimal and focused: read instance_id directly from raw JSON instead of validating the full EvalOutput model.

Changes

• benchmarks/utils/critics.py: Modified get_completed_instances() to parse instance_id directly from raw JSON lines, bypassing EvalOutput.model_validate() which fails on stale archives with incompatible pydantic discriminators.

• tests/test_iterative_resume.py: Added regression test covering stale schema rows, fresh rows, blank lines, malformed JSON, and missing instance_id.

Observations

1. Good error handling: Blank lines, malformed JSON, and missing instance_id are all handled gracefully with warnings.

2. Clear docstring: The updated docstring explains exactly why raw JSON parsing is needed (schema drift tolerance).

3. Test coverage: The test covers 5 edge cases including the stale schema regression case (evaluation#515).

4. No breaking changes: The function signature and return type are unchanged. All existing behavior is preserved.

5. Minimal diff: The change is focused and doesn't introduce unnecessary abstractions.

---

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
• No breaking changes to existing behavior. The change only affects error handling (what was previously a validation error is now handled gracefully). Type signatures unchanged.

VERDICT: ✅ Worth merging — Core logic is sound, well-tested, and solves a real compatibility issue.

KEY INSIGHT: Bypassing pydantic validation for schema drift tolerance is the right trade-off here — instance_id is a simple string field that doesn't need validation.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[juanmichelini]

Tested here https://openhands-eval-monitor.vercel.app/?run=swebenchmultimodal%2Flitellm_proxy-gemini-3-5-flash%2F27052081405%2F&days=15

[juanmichelini]

@OpenHands please fix precommits checks

[juanmichelini]

@OpenHands please fix precommits checks

[openhands-ai]

I'm on it! juanmichelini can track my progress at all-hands.dev

[openhands-ai]

I'm on it! juanmichelini can track my progress at all-hands.dev

[openhands-ai]

Summary

Request: "@OpenHands please fix precommits checks" on PR #740.

Work done:

• Investigated the failing pre-commit check via gh run view 27048621555 --log-failed.
• Identified the failure: ruff-format wanted to remove a stray trailing blank line at the end of tests/test_iterative_resume.py (file ended with )\n\n instead of )\n).
• Removed that one extra newline, committed as cf3617c9, and pushed to fix/resume-tolerate-stale-eval-output-schema.

Checklist:

• ✅ Pre-commit failure diagnosed from CI logs (single ruff-format complaint).
• ✅ Applied the exact change ruff-format produced (verified git diff matches the CI diff).
• ✅ Committed with Co-authored-by: openhands trailer and pushed to the PR branch.
• ✅ Changes are minimal — a one-character deletion in a single test file, nothing extraneous.

The other passing check (tests) was untouched. PR: #740

[openhands-ai]

@OpenHands-AI[bot] it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again.

[openhands-ai]

Summary

The pre-commit failure on PR #740 has already been resolved on the remote branch — no new changes were required from me.

What I found

• The pre-commit failure was a single ruff-format complaint: a trailing blank line at the end of tests/test_iterative_resume.py.
• While I was working on the fix locally (removing that trailing blank line and committing), someone/something pushed the identical fix to the branch as commit cf3617c9 ("fix: remove trailing blank line in test_iterative_resume.py (ruff format)").
• My local commit was rejected on push because the remote was already ahead with the same change. I reset my local branch back to origin/fix/resume-tolerate-stale-eval-output-schema rather than force-pushing or duplicating the fix.

Final PR #740 check status

• ✅ pre-commit: passing
• ⏳ tests: pending (running)

Checklist against the user's request

1. ✅ Pre-commit checks are now passing on the PR — the user's request is fully addressed.
2. ✅ No extraneous changes were left behind — I reset my redundant local commit so the branch matches origin exactly.
3. ✅ I verified locally that uv run pre-commit run --files tests/test_iterative_resume.py reports all hooks (Ruff format, Ruff lint, pycodestyle, Pyright strict) passing.

No further action is needed.