Close the review gaps: locked hash re-check, sticky QA cancel, validator parity

Fixes from an independent review of the fix batch.

The audit import checked its pinned content hash in its own short
session, then wrote findings later without re-checking: an in-place
overwrite committing in between still landed old-bytes findings as
SUCCESS on the new bytes, and the stale SUCCESS blocked the None->QUEUED
CAS that would audit them. sync_pre_trial_to_task_version now takes the
expected hash and re-checks it under the version row lock on every
write; the unlocked early check remains only to spare the artifact read.

Cancelling QA during the new admission-deferral window (task RUNNING,
agents settled, audit live) did not stick: cancel only settled
VERDICT_PENDING tasks, so the sweep's advance backstop re-entered
admission minutes later and started a QA run the user had cancelled,
against a brief whose findings the cancel wiped. cancel_task_qa_core now
settles a RUNNING task with no active agent trials the same way.

The shared validator skipped ActionItem's optional fields (id, links_to,
exploit_evidence, exploited, causal), which the importer's parser still
type-checks -- a wrong-typed value passed the sandbox and then failed
import terminally with no retry. The validator now type-checks them.

The with_verdict=False QA brief said "set verdict null" while its output
template still showed the verdict object and schema; with the strict
verifier that contradiction would burn every attempt. The template now
renders null and omits the schema when no verdict is requested. Also
hides the drawer Retry button on qa/audit rows, which could only ever
render the server's 400.

Claude-Session: https://claude.ai/code/session_01ACF6SUXdbLFarpj3qwF1Ki

Author: kyle-compute

frontend/src/components/trial-detail-panel.tsx

@@ -66,6 +66,7 @@ import { QaAssessmentReport } from "@/components/qa-report/qa-assessment-report"
 import { TimingBreakdownBar } from "@/components/timing-breakdown-bar";
 import { CodeBlock } from "@/components/code-block";
 import type { Trial, Task } from "@/lib/types";
+import { isAgentTrial } from "@/lib/types";
 import {
   costEstimateMarks,
   formatCostUsd,
@@ -1068,8 +1069,12 @@ export function TrialDetailPanel({
     artifactsLines,
   ]);
 
+  // Agent rows only: the generic retry endpoint refuses qa/audit kinds, so
+  // offering the button on their drawers would only ever render its 400.
   const showRetry =
-    allowRetry && (trial?.status === "failed" || trial?.status === "success");
+    allowRetry &&
+    Boolean(trial && isAgentTrial(trial)) &&
+    (trial?.status === "failed" || trial?.status === "success");
   const canRetry = actionsReady && showRetry;
   const showDelete = allowDelete && Boolean(onDelete) && Boolean(trial);
   const canDelete = actionsReady && showDelete;

oddish/src/oddish/core/endpoints/qa.py

@@ -186,6 +186,21 @@ async def cancel_task_qa_core(
                 else TaskStatus.FAILED
             )
             task.finished_at = now_value
+        elif task.status == TaskStatus.RUNNING and not await _count_active_trials(
+            session, task_id=task.id, task_version_id=task.current_version_id
+        ):
+            # QA admission holds a task in RUNNING with every agent trial
+            # settled while its audit runs. Cancelling that audit must
+            # settle the task too: left RUNNING, the sweep's advance
+            # backstop re-enters admission minutes later and starts a QA
+            # run the user just cancelled -- against a brief whose audit
+            # findings this cancel wiped.
+            task.status = (
+                TaskStatus.COMPLETED
+                if task.verdict_status == VerdictStatus.SUCCESS
+                else TaskStatus.FAILED
+            )
+            task.finished_at = now_value
     # A pre-trial status left QUEUED/RUNNING with nothing behind it would
     # keep the card in a running state forever, so cancel always clears it.
     # An audit trial pins the version it audits, and that version can be

oddish/src/oddish/core/verdict_sync.py

@@ -150,22 +150,41 @@ async def sync_pre_trial_to_task_version(
     *,
     payload: dict | None,
     error: BaseException | str | None,
+    expected_content_hash: str | None = None,
 ) -> str | None:
     """Write the pre-trial columns on the audited task version. Unlike
     :func:`sync_verdict_to_task`, this never completes the task and never
     touches a verdict column -- pre-trial is a per-version source audit that
     runs independently of trial classification.
 
+    ``expected_content_hash`` pins the source bytes the audit actually read:
+    the check runs here, under the version row lock, because an in-place
+    overwrite can replace the bytes between any earlier unlocked check and
+    this write. On a mismatch nothing is written -- the overwrite already
+    reset the pre-trial state, so a fresh audit of the new bytes can still
+    be enqueued.
+
     Returns the terminal ``VerdictStatus`` value written, or ``None`` when
-    the write was skipped (version gone) so the caller can release its claim
-    on the version.
+    the write was skipped (version gone, or overwritten bytes) so the caller
+    can release its claim on the version.
     """
     async with get_session() as session:
         version = await session.get(
             TaskVersionModel, task_version_id, with_for_update=True
         )
         if version is None:
             return None
+        if (
+            expected_content_hash is not None
+            and version.content_hash is not None
+            and version.content_hash != expected_content_hash
+        ):
+            logger.warning(
+                "pre-trial write for version %s skipped: content hash changed "
+                "since the audit started (in-place overwrite)",
+                task_version_id,
+            )
+            return None
 
         if error is None:
             version.pre_trial = payload

oddish/src/oddish/worker/analysis_result_check.py

@@ -71,6 +71,14 @@ def _check_action_item(item: object, expected: dict, where: str) -> list[str]:
     for key in ("title", "detail", "recommendation"):
         if _missing(item.get(key)):
             errors.append(f"{where}.{key} must be a non-empty string")
+    # Optional fields still fail the importer's parser when wrong-typed, so
+    # they must fail here first, where failing buys a retry.
+    for key in ("id", "links_to", "exploit_evidence"):
+        if key in item and item[key] is not None and not isinstance(item[key], str):
+            errors.append(f"{where}.{key} must be a string or null")
+    for key in ("exploited", "causal"):
+        if key in item and not isinstance(item[key], bool):
+            errors.append(f"{where}.{key} must be a boolean")
     return errors
 
 

oddish/src/oddish/workers/analysis_trials.py

@@ -351,6 +351,16 @@ def build_qa_brief(
         if with_verdict
         else '== TASK VERDICT ==\nDo NOT produce a verdict for this task: there are too few trials to judge it. Set "verdict": null in the output.\n'
     )
+    # The output template must agree with the section above: showing the
+    # verdict object shape while the prose says null would make the model
+    # fail the (strict) verifier on every attempt.
+    verdict_value = "<object matching this JSON schema>" if with_verdict else "null"
+    verdict_schema = (
+        "Verdict JSON schema:\n"
+        f"{json.dumps(TaskVerdictModel.model_json_schema(), indent=1)}\n\n"
+        if with_verdict
+        else ""
+    )
     return f"""You are the QA auditor for the task `{task_name}`. You are in a clean analysis sandbox, not the task's own environment. The task source, each trial's logs, and each trial's trajectory come from the oddish-query CLI. Do not solve the task.
 
 Audit these trials:
@@ -389,13 +399,10 @@ def build_qa_brief(
       "trajectory_summary": <object with the exact shape given in the trajectory summary section>
     }}
   ],
-  "verdict": <object matching this JSON schema>
+  "verdict": {verdict_value}
 }}
 
-Verdict JSON schema:
-{json.dumps(TaskVerdictModel.model_json_schema(), indent=1)}
-
-Every trial listed above must appear in "trials". The file must be valid JSON. Do not write anything else to /logs."""
+{verdict_schema}Every trial listed above must appear in "trials". The file must be valid JSON. Do not write anything else to /logs."""
 
 
 def build_audit_brief(*, task_name: str) -> str:
@@ -877,6 +884,7 @@ async def _import_audit_result(trial: TrialModel) -> None:
             version_id,
             payload=None,
             error=RuntimeError(error),
+            expected_content_hash=pinned_hash,
         )
         return
     items: list[ActionItem] = []
@@ -892,15 +900,22 @@ async def _import_audit_result(trial: TrialModel) -> None:
             )
             logger.warning("audit import for version %s failed: %s", version_id, error)
             await sync_pre_trial_to_task_version(
-                version_id, payload=None, error=RuntimeError(error)
+                version_id,
+                payload=None,
+                error=RuntimeError(error),
+                expected_content_hash=pinned_hash,
             )
             return
+    # The early check above spared the artifact read, but only this locked
+    # re-check (inside sync) closes the race with an in-place overwrite
+    # committing between that check and this write.
     await sync_pre_trial_to_task_version(
         version_id,
         payload=build_pre_trial_payload(
             items, cost_usd=trial.cost_usd, block_id=trial.id
         ),
         error=None,
+        expected_content_hash=pinned_hash,
     )
     logger.info(
         "audit trial %s: stored %d findings for version %s",

oddish/tests/test_analysis_trials.py

@@ -67,6 +67,21 @@ def test_the_audit_brief_names_its_output_file():
     assert "Do not solve the task" in brief
 
 
+def test_the_no_verdict_brief_does_not_contradict_itself():
+    """with_verdict=False must not show the verdict-object template or its
+    schema: the strict verifier requires null there, and a template
+    contradicting the prose would burn every retry attempt."""
+    brief = build_qa_brief(
+        task_name="demo",
+        trial_ids=["t-1"],
+        pre_trial_items=None,
+        with_verdict=False,
+    )
+    assert '"verdict": null' in brief
+    assert "Verdict JSON schema" not in brief
+    assert "<object matching this JSON schema>" not in brief
+
+
 def _qa_check_payload(trial_ids: list[str], *, with_verdict: bool = False) -> dict:
     from oddish.workers.analysis_trials import analysis_check_payload
 
@@ -716,6 +731,131 @@ async def read_clean(trial, filename):
         assert version.pre_trial is not None
 
 
+@pytest.mark.asyncio
+async def test_pre_trial_write_rechecks_the_hash_under_the_lock():
+    """Needs a database. The audit import's early hash check runs unlocked,
+    so an in-place overwrite can commit between it and the write. The write
+    itself must re-check the pin under the version row lock and skip -- or
+    old-bytes findings land as SUCCESS on the new bytes and block the
+    None->QUEUED CAS that would audit them."""
+    if not URL:
+        pytest.skip("ODDISH_DATABASE_URL not set")
+    from oddish.core.verdict_sync import sync_pre_trial_to_task_version
+    from oddish.db import VerdictStatus, get_session, init_db
+    from oddish.db.models import TaskModel, TaskVersionModel
+
+    await init_db()
+    run = uuid.uuid4().hex[:8]
+    task_id = f"qa-hash-lock-{run}"
+    version_id = f"{task_id}-v1"
+    async with get_session() as session:
+        session.add(
+            TaskModel(
+                id=task_id, name=task_id, user="u", task_path="p", run_analysis=True
+            )
+        )
+        await session.flush()
+        session.add(
+            TaskVersionModel(
+                id=version_id,
+                task_id=task_id,
+                version=1,
+                task_path="p",
+                content_hash="new-bytes",
+            )
+        )
+        await session.commit()
+
+    stale = await sync_pre_trial_to_task_version(
+        version_id,
+        payload={"items": []},
+        error=None,
+        expected_content_hash="old-bytes",
+    )
+    assert stale is None
+    async with get_session() as session:
+        version = await session.get(TaskVersionModel, version_id)
+        assert version.pre_trial is None
+        assert version.pre_trial_status is None
+
+    current = await sync_pre_trial_to_task_version(
+        version_id,
+        payload={"items": []},
+        error=None,
+        expected_content_hash="new-bytes",
+    )
+    assert current == VerdictStatus.SUCCESS.value
+    async with get_session() as session:
+        version = await session.get(TaskVersionModel, version_id)
+        assert version.pre_trial_status == VerdictStatus.SUCCESS
+
+
+@pytest.mark.asyncio
+async def test_cancelling_qa_in_the_deferral_window_settles_the_task():
+    """Needs a database. With every agent trial settled and the audit still
+    live, admission holds the task in RUNNING. Cancelling QA there must
+    settle the task too: left RUNNING, the sweep's advance backstop would
+    re-enter admission minutes later and start a QA run the user just
+    cancelled, against a brief whose audit findings the cancel wiped."""
+    if not URL:
+        pytest.skip("ODDISH_DATABASE_URL not set")
+    from oddish.core.endpoints.qa import cancel_task_qa_core
+    from oddish.db import TaskStatus, TrialStatus, get_session, init_db
+    from oddish.db.models import ExperimentModel, TaskModel
+
+    await init_db()
+    run = uuid.uuid4().hex[:8]
+    task_id = f"qa-cancel-window-{run}"
+    agent_id = f"{task_id}-1"
+    audit_id = f"{task_id}-2"
+    async with get_session() as session:
+        experiment = ExperimentModel(name=f"exp-{run}")
+        session.add(experiment)
+        session.add(
+            TaskModel(
+                id=task_id,
+                name=task_id,
+                user="u",
+                task_path="p",
+                status=TaskStatus.RUNNING,
+                run_analysis=True,
+            )
+        )
+        await session.flush()
+        for trial_id, kind, status in (
+            (agent_id, "agent", TrialStatus.SUCCESS),
+            (audit_id, "audit", TrialStatus.RUNNING),
+        ):
+            session.add(
+                TrialModel(
+                    id=trial_id,
+                    name=trial_id,
+                    task_id=task_id,
+                    experiment_id=experiment.id,
+                    agent="claude-code",
+                    provider="local",
+                    queue_key="q",
+                    kind=kind,
+                    status=status,
+                    attempts=1,
+                    max_attempts=3,
+                )
+            )
+        await session.commit()
+
+    async with get_session() as session:
+        await cancel_task_qa_core(session, task_id=task_id)
+        await session.commit()
+
+    async with get_session() as session:
+        audit = await session.get(TrialModel, audit_id)
+        assert audit.status == TrialStatus.FAILED
+        assert audit.harbor_stage == "cancelled"
+        task = await session.get(TaskModel, task_id)
+        assert task.status == TaskStatus.FAILED
+        assert task.finished_at is not None
+
+
 @pytest.mark.asyncio
 async def test_cleanup_reimports_a_settled_audit(monkeypatch):
     """Needs a database. A settled audit whose importer died mid-write
@@ -1032,6 +1172,29 @@ def test_the_validator_holds_audit_items_to_the_prompt_schema():
         assert check_analysis_result({"items": [broken]}, expected), key
     assert check_analysis_result({"items": {}}, expected)
 
+    # Optional fields the importer's parser still type-checks: a wrong type
+    # must fail here, in the sandbox where failing buys a retry, not at
+    # import where refusal is terminal.
+    for key, bad in (
+        ("id", 1),
+        ("links_to", 7),
+        ("exploited", "yes"),
+        ("causal", "no"),
+        ("exploit_evidence", 3),
+    ):
+        typed = dict(item)
+        typed[key] = bad
+        assert check_analysis_result({"items": [typed]}, expected), key
+    optional_ok = dict(
+        item,
+        id=None,
+        links_to="a1",
+        exploited=True,
+        causal=False,
+        exploit_evidence=None,
+    )
+    assert check_analysis_result({"items": [optional_ok]}, expected) == []
+
 
 @pytest.mark.asyncio
 async def test_the_qa_import_is_all_or_nothing(monkeypatch):