feat(eval): freeze cell identity and resume semantics before stable cut - #436
Conversation
- 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughModel trials now normalize prompt line endings for stable cell identity, persist manifest digests in cell metadata, report stale cells during resume, and derive attempt numbers from the highest recorded attempt rather than directory counts. ChangesModel trial identity and resume
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant execute
participant build_plan
participant CellStorage
User->>execute: resume trial
execute->>build_plan: rebuild manifest plan
build_plan->>execute: return cells and stale_cells
execute->>CellStorage: read prior cell records
execute->>CellStorage: persist digest and new attempt
execute-->>User: report stale cell count
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR defines stable evaluation-cell identity and resume behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(eval): ignore non-positive recorded ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
solomonneas
added
the
coderabbit-review
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/brigade/model_trials.py`:
- Around line 329-341: Update _attempt_number to include the valid attempt value
from the persisted nonterminal cell.json marker when calculating the next
attempt, even if no attempt directory exists; preserve directory-based numbering
and default behavior otherwise. Add a regression test covering a running marker
with no attempt directory and verify that the next attempt number is allocated
without reuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34c935ee-2d27-4c35-87b2-e00694612c4d
⛔ Files ignored due to path filters (1)
docs/phase-eval-cell-identity.mdis excluded by!docs/**,!**/docs/**
📒 Files selected for processing (2)
src/brigade/model_trials.pytests/test_model_trials.py
| _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 | ||
| if not attempts.is_dir(): | ||
| return 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 | ||
| ] | ||
| return max(numbers, default=0) + 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Account for a durable running marker with no attempt directory.
cell.json is persisted as running before aboyeur.run can create attempt-<n>/run. A crash in that window leaves no matching directory, so resume returns 1 and reuses attempt 1 instead of allocating a new attempt. Include the previous nonterminal marker’s valid attempt in the maximum, and add a regression test for this crash window.
Proposed fix
-def _attempt_number(cell_dir: Path) -> int:
+def _attempt_number(cell_dir: Path, *, prior_attempt: int | None = None) -> int:
attempts = cell_dir / "attempts"
- if not attempts.is_dir():
- return 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()
+ and entry.is_dir()
+ and (match := _ATTEMPT_DIR.fullmatch(entry.name)) is not None
+ ] if attempts.is_dir() else []
+ if isinstance(prior_attempt, int) and not isinstance(prior_attempt, bool) and prior_attempt > 0:
+ numbers.append(prior_attempt)
return max(numbers, default=0) + 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _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 | |
| if not attempts.is_dir(): | |
| return 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 | |
| ] | |
| return max(numbers, default=0) + 1 | |
| _ATTEMPT_DIR = re.compile(r"attempt-(\d+)") | |
| def _attempt_number(cell_dir: Path, *, prior_attempt: int | None = None) -> int: | |
| attempts = cell_dir / "attempts" | |
| numbers = [ | |
| int(match.group(1)) | |
| for entry in attempts.iterdir() | |
| if attempts.is_dir() | |
| and entry.is_dir() | |
| and (match := _ATTEMPT_DIR.fullmatch(entry.name)) is not None | |
| ] if attempts.is_dir() else [] | |
| if isinstance(prior_attempt, int) and not isinstance(prior_attempt, bool) and prior_attempt > 0: | |
| numbers.append(prior_attempt) | |
| return max(numbers, default=0) + 1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brigade/model_trials.py` around lines 329 - 341, Update _attempt_number
to include the valid attempt value from the persisted nonterminal cell.json
marker when calculating the next attempt, even if no attempt directory exists;
preserve directory-based numbering and default behavior otherwise. Add a
regression test covering a running marker with no attempt directory and verify
that the next attempt number is allocated without reuse.
There was a problem hiding this comment.
Already addressed, plus the requested regression test. The fix landed in 683d6cd: _attempt_number reads the persisted attempt from cell.json (including a nonterminal running marker) and takes the max with the attempt-NNN directories, so the crash window you describe (running marker, no attempt dir) allocates a new number instead of reusing attempt 1. a7f4b84 adds test_attempt_number_counts_running_marker_without_attempt_dir pinning exactly that window: cell.json with state: running, attempt: 1, no attempts/ directory, next attempt is 2. (Used the existing read-cell.json-inside-_attempt_number shape rather than the proposed prior_attempt parameter; same coverage, one less caller-side contract.)
|
Review-response pass complete (683d6cd). Addressed
Rejected: none — the Greptile finding was the only one received. CodeRabbit: no CodeRabbit review or comments had posted after re-checking every 2 minutes for 20 minutes, so this pass proceeded with the Greptile feedback only. PR body correction: the Verify section previously cited a gate run whose receipt lived only in a throwaway checkout. Re-ran the full gate on a clean checkout of 683d6cd and preserved the receipt in the repo: |
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.
|
CodeRabbit review arrived at 16:40 UTC (after the earlier 20-minute poll window closed) — one Major finding on Status: already fixed by 683d6cd, which reads the persisted Loop gate after the test addition: |
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.
|
Additional local review pass with
No other findings on the PR diff. Loop gate after the fix: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Closes #434
Landed
docs/phase-eval-cell-identity.md: tracked semantics doc stating the exactcell_ididentity field set, per-state resume rules (includingrunning), the keep-and-report stale-cell policy, themanifest_digestattribution field, and the rule that any identity payload change requires aCELL_SCHEMAbump plus a migration note.cell_idhex from a fixed manifest/roster fixture.\r\nand\rnormalize to\nin prompt text before it enters the identity payload, so the same logical manifest hashes identically across checkouts. Changes ids only for manifests that contained CRLF/CR; documented as acceptable before stable.manifest_digestis recorded in everycell.jsonat both write sites (therunningmarker and the final payload).max(existing attempt numbers) + 1over both theattempt-NNNdirectories and theattemptvalue recorded incell.json, tolerating gaps and non-matching directories; deleting even the highest attempt directory never reuses a number. Tested with a gappedattempts/directory and with a deleted highest attempt.execute --resumeprints a one-line stale-cell count to stderr when the plan has stale cells.cell_id, old cell appears in summarystale_counts.runningre-runs on resume as a new attempt.Deferred (per issue, not in this PR)
executeprocesses on one output dir. The doc states the current answer: concurrent executes are unguarded;runningis treated as crash-on-resume.Verify
Gate:
brigade work verify run --target . --command "./scripts/verify" --capture brigade-work, run20260722-164001-work-verify-292c12on a clean checkout of this branch (receipt preserved at.brigade/work/verify-runs/20260722-164001-work-verify-292c12/receipt.json):Main-worktree loop gate:
brigade work verify run --target . --command ".venv/bin/python -m pytest tests/test_model_trials.py -q" --capture brigade-work, run20260722-163934-work-verify-a6f513: exit=0, 17 passed.Note: the full
./scripts/verifyagainst the main working tree fails atruff format --checkwithWould reformat: src/brigade/component_install.py— an unrelated uncommitted WIP file already dirty before this branch; this PR does not touch it.Summary by CodeRabbit
Bug Fixes
Tests