fix(validator): failed re-evaluation inherits the previous run's score from a stale results.json (#249)

[kai392]

Fixes #249

Root cause

evaluate_submission() writes every run of a submission into a workspace keyed by submission id (data/workspaces/submissions/<submission_id>/). Submission rows are reused, not recreated — create_pr_submission() updates the existing row when a miner pushes a new commit to the same PR — so every re-evaluation lands in the same directory, and nothing removed the previous results.json.

A re-run that produced no results therefore never tripped either guard:

if completed != 0 and not local_results_path.exists():   # False - the old file is still there
    raise subprocess.CalledProcessError(...)
...
metrics, score = _prepare_results(local_results_path, settings=settings)   # parses the stale payload

_is_missing_results_payload() (added for #12) never fired either, because the results were not missing — they were from a different run. The run was marked completed and carried the earlier run's score, plus the earlier run's cost_ledger.jsonl.

The remote path had the same hole one level up: _remote_attempt() rsyncs the remote workspace back without --delete, and the remote workspace is also keyed by submission id, so a stale remote results.json would be copied over a freshly cleared local one.

Fix approach

Minimal, two changes in eval_runner.py:

• _clear_previous_run_files() unlinks results.json and cost_ledger.jsonl at the top of evaluate_submission(), before any attempt runs.
• The remote command prologue now rm -fs the remote results.json, right next to the ledger truncation it already did.

No behaviour is changed for a run that actually produces results; the two existing "missing results" guards simply now see the truth.

Impact

The stale score did not stay in the run row — it propagated to submission.latest_score, best_eval_id, the public leaderboard, update_king_score() and the worker's should_promote_submission() merge/close decision. A miner could bank one passing score and have any later broken checkpoint inherit it. The fix also stops broken evaluations from being reported as clean passes.

Tests

• test_reevaluation_does_not_reuse_previous_results — evaluate once with a scoring stub (0.91), then again with a stub that writes nothing and exits non-zero; the second run must be failed with score is None, latest_score is None and cost_usd == 0.0. Fails on main, passes here.
• test_remote_command_clears_stale_remote_results — the remote prologue removes the remote results.json.

Risk / tradeoffs

• Low. The only new failure mode is an OSError from Path.unlink() on an unwritable workspace, which is deliberately not swallowed: if freshness cannot be guaranteed the evaluation must not proceed, and the worker already marks such a job failed.
• Deleting the ledger up front is safe — both eval command templates already truncate it (: > $TRINITY_COST_LEDGER) before running; this only covers the case where the command never starts.
• Provider-route evaluations are keyed by a unique EvaluationRun.id, so their workspace is never reused; left untouched.
• docs/JOURNAL.md entry added as required by AGENTS.md §6.

🤖 Generated with Claude Code

[kai392]

Two CI notes for reviewers:

• CI is action_required — like every fork PR on this repo, the pull_request workflow needs a maintainer to click Approve and run before test-router / test-validator / web will report. Could someone approve the run?
• PR automation / register_submission failing is pre-existing and unrelated to this change. actions/checkout now refuses to check out fork code from a pull_request_target workflow without allow-unsafe-pr-checkout: true, so this job fails on every fork PR (see the identical failures on fix(worker): one-shot run exits 0 after a successful pass #248, fix(eval): cancel queued provider-eval jobs when a standalone evaluation is deleted #247, Skip Verifier turns when selecting the committed answer #246 …). That is exactly what fix(ci): register_submission fails on every fork PR (pull_request_target checkout guard) #233 fixes; nothing in this PR touches the workflow.

What I verified locally, since the validator job can't run on my machine (it needs POSIX pty plus a Postgres service):

• full router suite: python -m pytest tests -q → 186 passed, 3 skipped (the one failure on my box is test_run_remote.py needing a python3 on PATH — environmental, present before this change);
• ruff check clean on both changed files;
• direct exercise of the changed code paths: _clear_previous_run_files() removes a stale results.json/cost_ledger.jsonl and is idempotent; _prepare_results() then returns results_missing instead of the previous run's score; _ledger_cost_report() returns cost_usd == 0.0 instead of billing the previous run; and _build_remote_command() emits rm -f <remote results.json> in the prologue with the command template otherwise unchanged.

Happy to adjust if you would rather keep the remote-side rm -f out of the command prologue and do it as a separate ssh step instead.

[kai392]

CI is green on this branch. I enabled Actions on my fork and ran this repo's unmodified .github/workflows/ci.yml against this branch's exact commit (5401f72) on GitHub-hosted runners, including the real postgres:16 service:

job	result
test-router	✅ success
test-validator (Postgres 16)	✅ success
web (tsc build + lint)	✅ success

Run: https://github.com/kai392/minirouter/actions/runs/30106379606

I also ran the state after both this PR and #252 are merged (JOURNAL conflict resolved by keeping both entries) — all three jobs green: https://github.com/kai392/minirouter/actions/runs/30106669053

So the only red mark left here, PR automation / register_submission, is not something this branch can affect: pull_request_target always runs the workflow definition from base main, so that job fails identically on every fork PR regardless of branch contents, and it can only go green once #233 is merged. The CI check on this PR still shows action_required because a maintainer approval is required to run workflows for a first-time contributor — the run above is the same workflow, same commit, so approving it should reproduce exactly this result.