fix(validator): skip synchronous eval for submissions with no artifact (#143)

docs/JOURNAL.md

@@ -18,6 +18,28 @@ protocol. **Newest entries at the top.** Tag each entry with one or more of:
 
 ---
 
+## 2026-07-13 — /submit sync-eval ran on checkpoint-less PRs, failing them (#143)  #mistake #gotcha
+**Context:** infra/bugfix PRs (no `submissions/final_model/best_theta.npy`) were being marked
+`failed` with `missing_checkpoint`, which closes them before a maintainer can merge (#143). One of
+my own PRs (#117) was auto-closed exactly this way.
+**Expected:** a submission with no uploaded model bundle is never evaluated.
+**Actual:** the async paths already skip such submissions — the pipeline enqueue in `/submit`
+(`routes.py`) and `queue.py` both gate on `submission.submission_artifact_id is not None` — but the
+**synchronous** branch (`if settings.sync_eval_on_submit and not settings.uses_train_pipeline:`) did
+not, so `evaluate_submission` ran on a checkpoint-less submission and hit the
+`does not have a checkpoint to evaluate` failure in `eval_runner.py`.
+**Root cause:** the artifact gate was added to the async paths but the synchronous path was missed —
+the invariant "never evaluate a submission without an artifact" was enforced in two of three places.
+**Fix / decision:** extracted `_should_sync_eval(settings, submission)` and added the same
+`submission_artifact_id is not None` clause, so the sync path matches the async ones. Deliberately
+did **not** touch `eval_runner`'s `missing_checkpoint` failure — that is a legitimate safety net for a
+submission that *has* an artifact row whose checkpoint file is genuinely missing.
+**Follow-up:** this only stops the validator from *generating* the failure; a maintainer still has to
+deploy it, and it cannot reopen already-closed PRs (#117 was re-opened as a fresh PR). Testable
+offline via the pure `_should_sync_eval` helper (no Postgres needed).
+
+---
+
 ## 2026-07-12 — Validator Postgres tests no longer silently skip in CI  #decision #repro
 **Context:** issue #118 flagged that validator DB-backed tests could ``pytest.skip`` whenever Postgres
 was unreachable, including on CI where no database service was provisioned.

validator/src/eval_backend/api/routes.py

@@ -377,6 +377,23 @@ def health() -> HealthResponse:
     return HealthResponse()
 
 
+def _should_sync_eval(settings: Settings, submission: Submission) -> bool:
+    """Whether to run ``evaluate_submission`` inline for a just-registered submission.
+
+    A submission with no ``submission_artifact_id`` has no uploaded model bundle
+    (e.g. an infra/bugfix PR that never ships ``best_theta.npy``). Evaluating it
+    only produces a spurious ``missing_checkpoint`` failure that marks the PR's
+    submission ``failed`` — the behaviour that closes infra PRs (#143). The async
+    enqueue paths already skip such submissions (see ``queue.py`` and the pipeline
+    gate below); this keeps the *synchronous* path consistent with them.
+    """
+    return (
+        settings.sync_eval_on_submit
+        and not settings.uses_train_pipeline
+        and submission.submission_artifact_id is not None
+    )
+
+
 @router.post("/submit", response_model=SubmissionCreateResponse)
 async def submit(
     request: Request,
@@ -449,7 +466,7 @@ async def submit(
         session.flush()
         session.commit()
 
-        if settings.sync_eval_on_submit and not settings.uses_train_pipeline:
+        if _should_sync_eval(settings, submission):
             session.refresh(submission)
             evaluate_submission(session, submission, runtime_settings)
             session.commit()

validator/tests/test_submit_sync_eval_gate.py

@@ -0,0 +1,45 @@
+"""Offline tests for the synchronous-eval gate on ``POST /submit`` (issue #143).
+
+A submission with no ``submission_artifact_id`` has no uploaded model bundle
+(an infra/bugfix PR that never ships ``best_theta.npy``). Running
+``evaluate_submission`` on it only yields a spurious ``missing_checkpoint``
+failure that marks the PR's submission ``failed`` — the behaviour that closes
+infra PRs. The synchronous path must skip such submissions, exactly as the async
+enqueue paths already do.
+
+These tests exercise the pure ``_should_sync_eval`` decision, so they need no
+database and run everywhere (the DB-backed validator tests skip without Postgres).
+"""
+from __future__ import annotations
+
+from types import SimpleNamespace
+
+from eval_backend.api.routes import _should_sync_eval
+from eval_backend.core.config import PIPELINE_TRAIN_EVAL, Settings
+
+
+def _submission(artifact_id):
+    return SimpleNamespace(submission_artifact_id=artifact_id)
+
+
+def test_no_artifact_is_never_sync_evaluated():
+    """The #143 regression: a checkpoint-less submission must not be evaluated."""
+    settings = Settings(sync_eval_on_submit=True)
+    assert settings.uses_train_pipeline is False
+    assert _should_sync_eval(settings, _submission(None)) is False
+
+
+def test_artifact_present_is_sync_evaluated():
+    settings = Settings(sync_eval_on_submit=True)
+    assert _should_sync_eval(settings, _submission("artifact-123")) is True
+
+
+def test_sync_eval_disabled_never_runs_inline():
+    settings = Settings(sync_eval_on_submit=False)
+    assert _should_sync_eval(settings, _submission("artifact-123")) is False
+
+
+def test_train_pipeline_defers_to_async_even_with_artifact():
+    settings = Settings(sync_eval_on_submit=True, pipeline_mode=PIPELINE_TRAIN_EVAL)
+    assert settings.uses_train_pipeline is True
+    assert _should_sync_eval(settings, _submission("artifact-123")) is False