fix(review): separate review and verifier rubric lifecycles

[bingran-you]

Summary

• reserve verifier/rubric.json and tests/rubric.json exclusively for detached post-run bench review
• stop LLMJudgeRewardFunc from auto-discovering JSON and reject explicit references to either reserved task path before evidence download or model calls
• move the default verifier-scoring contract to verifier/rubrics/verifier.toml, while retaining explicit TOML and Harvey-style JSON scoring rubrics at non-reserved paths
• make review-rubric ownership path-based instead of guessing the JSON dialect
• preserve the review lifecycle: completed reward artifacts are read-only evidence and reviewer output cannot change the original reward
• tell the reviewer exactly where the canonical solver trajectory lives, what its recorded ACP events may contain, and how to use it safely for behavioral and chronological criteria

Why

rubric.json is a quality-audit contract, not a reward contract. Its {name, description, guidance} criteria belong only to the isolated second-stage reviewer. The first-stage verifier must use a separately owned scoring rubric and must have zero influence from the detached review contract.

The previous shape-based behavior allowed ownership to depend on JSON fields. The new boundary is structural and fail-closed: files in the two reserved task slots are always review rubrics, regardless of their shape.

The reviewer previously received only a pointer to /evidence/trial/trajectory/. The default prompt now names /evidence/trial/trajectory/acp_trajectory.jsonl, explains its chronological redacted JSONL event contract and capture limitations, directs behavioral checks to it, and treats all solver evidence as untrusted data rather than instructions.

Migration

• Post-run review: keep {name, description, guidance} in verifier/rubric.json or legacy tests/rubric.json and run bench review.
• Verifier-time LLM scoring: use verifier/rubrics/verifier.toml or an explicit non-reserved JSON path such as verifier/rubrics/llm-judge.json.

Crossing the boundary now reports an actionable error naming bench review and the verifier-owned default path.

Regression coverage

Tests prove that:

• bench review loads verifier/rubric.json, renders name + guidance, and leaves the source reward byte-identical
• the reviewer prompt identifies the canonical ACP trajectory, describes its evidence contract and limitations, and warns against following embedded instructions
• JSON is never auto-discovered by LLMJudgeRewardFunc
• direct reward-function use, legacy verifier config, and native verifier strategies reject both reserved slots
• verification artifacts exist before detached review begins
• the default LLM-judge scoring path is verifier-owned

Validation

• uv run python -m pytest tests/ -q — 5,382 passed, 65 skipped, 7 deselected
• uv run ty check src/ — passed
• uv run ruff check . — passed
• uv run ruff format --check . — 638 files already formatted

[bingran-you]

@wenjun-ke could you please review this fail-closed rubric-dialect fix? The PR prevents LLMJudgeRewardFunc from silently discarding detached-review guidance and documents the correct migration path.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 477b302d8e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Accept judge criteria that also contain guidance

When a valid LLM-judge criterion contains both id/match_criteria and a supplemental guidance field, this unconditional check rejects it even though match_criteria remains the unambiguous grading rule and no guidance is silently substituted. This also conflicts with is_review_rubric_file() in src/benchflow/review/config.py, which explicitly classifies every entry containing id and match_criteria as the judge dialect, so such a rubric is claimed by neither consumer: bench review skips it and the judge loader fails. Restrict the rejection to criteria without match_criteria or to the actual detached-review shape.

Useful? React with 👍 / 👎.

[bingran-you]

@wenjun-ke the latest head now implements the full lifecycle boundary: verifier/rubric.json and tests/rubric.json are review-only, verifier scoring uses a separate contract, JSON auto-discovery is removed, and lifecycle/immutability regressions are covered. Could you please review the updated PR?

[bingran-you]

@wenjun-ke follow-up on the latest head: the reviewer prompt now explicitly names /evidence/trial/trajectory/acp_trajectory.jsonl, explains its chronological redacted ACP-event contents and capture limits, directs behavioral criteria to it, and treats embedded solver content as untrusted evidence rather than instructions.