feat(eval): freeze cell identity and resume semantics before stable cut (#436)

* feat(eval): freeze cell identity payload and resume semantics

- Document identity fields, per-state resume rules, and stale-cell
  keep-and-report policy in docs/phase-eval-cell-identity.md
- Snapshot-lock the identity payload keys and cell_id digest in tests
- Normalize CRLF/CR prompt line endings before identity hashing
- Record manifest_digest in every cell.json (running and final writes)
- Number attempts as max(existing) + 1, tolerating gaps and foreign dirs
- Print stale-cell count to stderr on execute --resume
- Cover manifest-edit-under-resume and kill-mid-run resume in tests

* fix(eval): persist attempt high-water mark against reuse

Review feedback on #436: max(attempt dirs) + 1 reuses a number when the
highest attempt directory is deleted. Take the next attempt as max over
both the attempt-NNN directories and the attempt value recorded in
cell.json, and document the mechanism.

* test(eval): cover running marker with no attempt directory

CodeRabbit review on #436: pin the crash window where cell.json records
a running attempt but no attempt-NNN directory exists yet, so resume
allocates the next number instead of reusing it.

* fix(eval): ignore non-positive recorded attempt numbers

A corrupt-but-valid-JSON cell.json marker (attempt -1 or 0) would lower
the high-water mark and produce attempt-000, violating the 1-based
numbering contract. Only positive integers count.

Author: solomonneas

docs/phase-eval-cell-identity.md

@@ -0,0 +1,97 @@
+# Eval cell identity and resume semantics
+
+This document is the contract for `brigade.eval_cell.v1` receipts produced by
+`src/brigade/model_trials.py`. It freezes the rules that become compatibility
+surfaces at the stable cut: how `cell_id` is derived, what `execute --resume`
+does for every recorded state, and how stale cells are handled.
+
+## Cell identity
+
+`cell_id` is the sha256 hex digest of a canonical JSON identity payload
+(`json.dumps` with sorted keys, `(",", ":")` separators, UTF-8) built in
+`expand_cells`. Exactly these fields participate, and nothing else:
+
+- `schema` — the `CELL_SCHEMA` tag (`brigade.eval_cell.v1`).
+- `case.id` — the case identifier from the manifest.
+- `case.prompt` — the inlined prompt text, after line-ending normalization
+  (see below).
+- `seat.seat` — the seat name.
+- `seat.cli` — the agent's CLI adapter.
+- `seat.model` — the pinned model.
+- `seat.reasoning` — the reasoning setting.
+- `seat.transport` — the transport, if any.
+- `seat.transport_version` — the transport version, if any.
+- `seat.env` — the agent's environment map, if any.
+- `seat.codex_transport` — the roster codex transport; set for codex seats
+  only, otherwise `null`.
+- `trial` — the 1-based trial number.
+- `graders` — the grader list for the case.
+- `execution_mode` — `read-only` or `writable-worktree`.
+
+The `coordinate` (`case:seat:trial`) is the human-stable axis and is **not**
+part of the identity payload; it is how staleness is detected (below).
+
+### Line-ending normalization
+
+Before prompt text (inline or from `prompt_file`) enters the identity payload,
+`\r\n` and bare `\r` are normalized to `\n`, so the same logical manifest
+hashes identically across checkouts with different line-ending conventions.
+This changes `cell_id` only for manifests that contained CRLF or CR line
+endings; that breakage is accepted because it lands before the stable cut.
+
+### Changing the identity payload
+
+Any change to the field set above — adding, removing, or reinterpreting a
+field — requires bumping `CELL_SCHEMA` and writing a migration note in this
+document. The identity lock test in `tests/test_model_trials.py` snapshots the
+exact payload keys and the resulting digest, so an accidental change fails CI.
+
+## Resume semantics
+
+`execute --resume` rebuilds the plan from the current manifest and decides per
+cell from the recorded `cell.json`:
+
+- `accepted`, `rejected`, `unscored`, `execution_error`, `adapter_error`,
+  `grader_error` (the terminal states): the cell is **skipped**; the existing
+  receipt stands.
+- `running`: the cell **re-runs as a new attempt**. `running` means the
+  previous process died mid-run (or, without a lock, is still executing in
+  another process). Resume treats it as a crash and starts the next attempt,
+  preserving the original `started_at`.
+- Missing, unreadable, or corrupt `cell.json`: the cell runs as a new attempt.
+- Any other state value: the cell re-runs (only exact terminal-state
+  membership causes a skip).
+
+Attempt numbers are `max(existing attempt numbers) + 1` over two sources —
+the `attempt-NNN` directories under `attempts/` and the `attempt` value
+recorded in `cell.json` — tolerating gaps and non-`attempt-NNN` directories.
+Because `cell.json` persists the last attempt number, deleting even the
+highest attempt directory never causes a number to be reused.
+
+Every `cell.json` — both the `running` marker written before the run and the
+final receipt — records `manifest_digest`, the canonical digest of the
+manifest that produced the plan, so each cell stays attributable to its
+generation even after later manifest edits.
+
+## Stale cells
+
+A cell is stale when its `coordinate` exists in the previous `plan.json` with
+a different `cell_id` (for example after a manifest edit). Staleness is
+computed against the immediately previous `plan.json` only.
+
+The policy is **keep and report, no pruning**:
+
+- Stale cell directories are left on disk untouched.
+- The new `plan.json` lists them under `stale_cells` with the previous and
+  current ids.
+- `summarize` excludes them from the headline counts and reports them under
+  `stale_counts`.
+- On `execute --resume` with stale cells present, a one-line stale count is
+  printed to stderr.
+
+## Concurrency
+
+Two concurrent `execute` processes on one output directory are not guarded:
+both see a `running` cell and interleave writes. A lockfile-based guard is
+tracked separately and deliberately not part of this freeze; until it lands,
+do not run concurrent executes against the same output directory.

src/brigade/model_trials.py

@@ -73,17 +73,21 @@ def load_manifest(path: Path) -> dict[str, Any]:
     return data
 
 
+def _normalize_prompt(text: str) -> str:
+    return text.replace("\r\n", "\n").replace("\r", "\n")
+
+
 def _case_prompt(case: dict[str, Any], base_dir: Path) -> str:
     prompt = case.get("prompt")
     prompt_file = case.get("prompt_file")
     if isinstance(prompt, str) and prompt:
-        return prompt
+        return _normalize_prompt(prompt)
     if isinstance(prompt_file, str) and prompt_file:
         candidate = (base_dir / prompt_file).resolve()
         if base_dir.resolve() not in candidate.parents and candidate != base_dir.resolve():
             raise ValueError(f"case {case.get('id')!r} prompt_file escapes the manifest directory")
         try:
-            return candidate.read_text()
+            return _normalize_prompt(candidate.read_text())
         except OSError as exc:
             raise ValueError(f"case {case.get('id')!r} prompt_file unreadable: {exc}") from exc
     raise ValueError(f"case {case.get('id')!r} needs prompt or prompt_file")
@@ -322,10 +326,25 @@ def _trial_worktree_path(
     )
 
 
+_ATTEMPT_DIR = re.compile(r"attempt-(\d+)")
+
+
 def _attempt_number(cell_dir: Path) -> int:
     attempts = cell_dir / "attempts"
-    existing = [p for p in attempts.iterdir() if p.is_dir()] if attempts.is_dir() else []
-    return len(existing) + 1
+    numbers = (
+        [
+            int(match.group(1))
+            for entry in attempts.iterdir()
+            if entry.is_dir() and (match := _ATTEMPT_DIR.fullmatch(entry.name)) is not None
+        ]
+        if attempts.is_dir()
+        else []
+    )
+    recorded = _load_json(cell_dir / "cell.json") or {}
+    prior = recorded.get("attempt")
+    if isinstance(prior, int) and not isinstance(prior, bool) and prior > 0:
+        numbers.append(prior)
+    return max(numbers, default=0) + 1
 
 
 def _load_json(path: Path) -> dict[str, Any] | None:
@@ -385,6 +404,11 @@ def execute(
         print("error: writable-worktree trials require a git worktree target", file=sys.stderr)
         return 2
     localio.write_json(output_dir / "plan.json", plan)
+    if resume and plan["stale_cells"]:
+        print(
+            f"note: {len(plan['stale_cells'])} stale cell(s) from the previous plan kept and counted in summary",
+            file=sys.stderr,
+        )
     failures = 0
     for cell in cells:
         cell_dir = output_dir / "cells" / cell.cell_id
@@ -406,6 +430,7 @@ def execute(
                 "state": "running",
                 "attempt": attempt,
                 "started_at": started_at,
+                "manifest_digest": plan["manifest_digest"],
             },
         )
         run_dir = cell_dir / "attempts" / f"attempt-{attempt:03d}" / "run"
@@ -456,6 +481,7 @@ def execute(
             "state": state,
             "attempt": attempt,
             "started_at": started_at,
+            "manifest_digest": plan["manifest_digest"],
             "exit_code": rc,
             "duration_seconds": run_meta.get("duration_seconds"),
             "run_dir": str(run_dir),

tests/test_model_trials.py

@@ -28,6 +28,83 @@ def _roster() -> Roster:
     )
 
 
+def test_cell_identity_payload_is_locked():
+    # Snapshot of the frozen identity contract (docs/phase-eval-cell-identity.md).
+    # Any change to the field set below must come with a CELL_SCHEMA bump.
+    manifest = {
+        "schema": "brigade.eval_manifest.v1",
+        "name": "identity-lock",
+        "trials": 1,
+        "seats": ["cursor"],
+        "cases": [{"id": "hello", "prompt": "Say hello"}],
+        "graders": [{"type": "exact_output", "expected": "hello"}],
+    }
+    cell = model_trials.expand_cells(manifest, _roster())[0]
+    expected_identity = {
+        "schema": "brigade.eval_cell.v1",
+        "case": {"id": "hello", "prompt": "Say hello"},
+        "seat": {
+            "seat": "cursor",
+            "cli": "cursor",
+            "model": "composer-2.5",
+            "reasoning": None,
+            "transport": "direct",
+            "transport_version": None,
+            "env": None,
+            "codex_transport": None,
+        },
+        "trial": 1,
+        "graders": [{"type": "exact_output", "expected": "hello"}],
+        "execution_mode": "read-only",
+    }
+    assert model_trials._canonical_digest(expected_identity) == cell.cell_id
+    assert cell.cell_id == "55c07e87f401b5aa49f956b2cec1bfee87986088702bc0b1762cc722ff2638c1"
+
+
+def test_prompt_line_endings_do_not_change_identity():
+    crlf = _manifest()
+    crlf["cases"][0]["prompt"] = "Say hello\r\nagain\rnow"
+    unix = _manifest()
+    unix["cases"][0]["prompt"] = "Say hello\nagain\nnow"
+    crlf_cells = model_trials.expand_cells(crlf, _roster())
+    unix_cells = model_trials.expand_cells(unix, _roster())
+    assert [cell.cell_id for cell in crlf_cells] == [cell.cell_id for cell in unix_cells]
+    assert crlf_cells[0].prompt == "Say hello\nagain\nnow"
+
+
+def test_attempt_number_uses_max_plus_one_and_tolerates_gaps(tmp_path):
+    assert model_trials._attempt_number(tmp_path) == 1
+    attempts = tmp_path / "attempts"
+    attempts.mkdir()
+    (attempts / "attempt-001").mkdir()
+    (attempts / "attempt-003").mkdir()
+    (attempts / "scratch").mkdir()
+    (attempts / "attempt-002-partial").mkdir()
+    assert model_trials._attempt_number(tmp_path) == 4
+
+
+def test_attempt_number_does_not_reuse_deleted_highest_attempt(tmp_path):
+    attempts = tmp_path / "attempts"
+    attempts.mkdir()
+    (attempts / "attempt-001").mkdir()
+    (tmp_path / "cell.json").write_text(json.dumps({"attempt": 3}))
+    assert model_trials._attempt_number(tmp_path) == 4
+
+
+def test_attempt_number_counts_running_marker_without_attempt_dir(tmp_path):
+    # Crash window: cell.json was written as running before attempt-001 existed.
+    (tmp_path / "cell.json").write_text(json.dumps({"state": "running", "attempt": 1}))
+    assert model_trials._attempt_number(tmp_path) == 2
+
+
+def test_attempt_number_ignores_nonpositive_recorded_attempt(tmp_path):
+    # Corrupt-but-valid-JSON markers must not produce attempt-000.
+    (tmp_path / "cell.json").write_text(json.dumps({"state": "running", "attempt": -1}))
+    assert model_trials._attempt_number(tmp_path) == 1
+    (tmp_path / "cell.json").write_text(json.dumps({"state": "running", "attempt": 0}))
+    assert model_trials._attempt_number(tmp_path) == 1
+
+
 def test_expand_cells_is_stable_and_conditions_change_identity():
     first = model_trials.expand_cells(_manifest(), _roster())
     second = model_trials.expand_cells(_manifest(), _roster())
@@ -228,6 +305,92 @@ def fake_run(task, roster, **kwargs):
     assert summary["stale_counts"] == {"accepted": 2}
 
 
+def test_resume_after_manifest_edit_reruns_only_changed_cells(tmp_path, monkeypatch, capsys):
+    manifest = _manifest()
+    manifest["trials"] = 1
+    manifest["cases"] = [
+        {"id": "alpha", "prompt": "Say alpha"},
+        {"id": "beta", "prompt": "Say beta"},
+    ]
+    manifest_path = tmp_path / "eval.json"
+    manifest_path.write_text(json.dumps(manifest))
+    tasks: list[str] = []
+
+    def fake_run(task, roster, **kwargs):
+        tasks.append(task)
+        out = kwargs["output_dir"]
+        out.mkdir(parents=True, exist_ok=True)
+        (out / "final.txt").write_text("hello\n")
+        (out / "run.json").write_text(json.dumps({"status": "ok", "duration_seconds": 0.5}))
+        return 0
+
+    monkeypatch.setattr(model_trials.aboyeur, "run", fake_run)
+    root = tmp_path / "results"
+    assert model_trials.execute(manifest_path, _roster(), workspace=tmp_path, output_dir=root, resume=False) == 0
+    assert sorted(tasks) == ["Say alpha", "Say beta"]
+
+    original_ids = {cell.case_id: cell.cell_id for cell in model_trials.expand_cells(manifest, _roster())}
+    alpha_path = root / "cells" / original_ids["alpha"] / "cell.json"
+
+    edited = json.loads(manifest_path.read_text())
+    edited["cases"][1]["prompt"] = "Say beta differently"
+    manifest_path.write_text(json.dumps(edited))
+    tasks.clear()
+    capsys.readouterr()
+    assert model_trials.execute(manifest_path, _roster(), workspace=tmp_path, output_dir=root, resume=True) == 0
+
+    # The unchanged cell is skipped; the edited cell re-runs under a new cell_id.
+    assert tasks == ["Say beta differently"]
+    alpha = json.loads(alpha_path.read_text())
+    assert alpha["state"] == "accepted"
+    assert alpha["attempt"] == 1
+    edited_ids = {cell.case_id: cell.cell_id for cell in model_trials.expand_cells(edited, _roster())}
+    assert edited_ids["alpha"] == original_ids["alpha"]
+    assert edited_ids["beta"] != original_ids["beta"]
+    new_beta = json.loads((root / "cells" / edited_ids["beta"] / "cell.json").read_text())
+    assert new_beta["state"] == "accepted"
+    plan = json.loads((root / "plan.json").read_text())
+    assert new_beta["manifest_digest"] == plan["manifest_digest"]
+
+    # The old cell is kept and reported, not pruned; resume warns on stderr.
+    assert (root / "cells" / original_ids["beta"] / "cell.json").is_file()
+    summary = json.loads((root / "summary.json").read_text())
+    assert summary["counts"] == {"accepted": 2}
+    assert summary["stale_counts"] == {"accepted": 1}
+    assert "1 stale cell(s)" in capsys.readouterr().err
+
+
+def test_resume_reruns_killed_running_cell_as_new_attempt(tmp_path, monkeypatch):
+    manifest = _manifest()
+    manifest["trials"] = 1
+    manifest_path = tmp_path / "eval.json"
+    manifest_path.write_text(json.dumps(manifest))
+
+    def fake_run(task, roster, **kwargs):
+        out = kwargs["output_dir"]
+        out.mkdir(parents=True, exist_ok=True)
+        (out / "final.txt").write_text("hello\n")
+        (out / "run.json").write_text(json.dumps({"status": "ok", "duration_seconds": 0.5}))
+        return 0
+
+    monkeypatch.setattr(model_trials.aboyeur, "run", fake_run)
+    root = tmp_path / "results"
+    assert model_trials.execute(manifest_path, _roster(), workspace=tmp_path, output_dir=root, resume=False) == 0
+    cell_path = next((root / "cells").glob("*/cell.json"))
+
+    # Simulate a kill mid-run: the last durable state is "running".
+    killed = json.loads(cell_path.read_text())
+    killed["state"] = "running"
+    cell_path.write_text(json.dumps(killed))
+
+    assert model_trials.execute(manifest_path, _roster(), workspace=tmp_path, output_dir=root, resume=True) == 0
+    final = json.loads(cell_path.read_text())
+    assert final["state"] == "accepted"
+    assert final["attempt"] == 2
+    attempts = sorted(p.name for p in (cell_path.parent / "attempts").iterdir())
+    assert attempts == ["attempt-001", "attempt-002"]
+
+
 def test_grader_envelope_links_digested_output(tmp_path, monkeypatch):
     manifest_path = tmp_path / "eval.json"
     manifest_path.write_text(json.dumps(_manifest()))