## Code Review: derive-review-repair standalone command

This change extracts the convergence loop (stages 4-7) from `cmd_pipeline` into a shared `_run_convergence_loop` function and adds a new standalone `derive-review-repair` CLI command.

---

### expert_build/cli.py (derive-review-repair subcommand)
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: PARTIAL
INTEGRATION: WIRED
REASONING: CLI arguments are well-chosen with sensible defaults. The `_lazy("pipeline", "cmd_derive_review_repair")` dispatch is correct and consistent with the existing pattern. No direct test exercises the argparse wiring (e.g., verifying `--rounds` parses to `args.rounds`), but the function-level tests cover the downstream behavior.
---

### expert_build/pipeline.py:_run_convergence_loop
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: Clean extraction of the derive/review/repair/dedup loop. Convergence condition (`invalid_count == 0 and added == 0`) is correct. Summary accumulation is sound. The `on_stage` callback design correctly separates the loop logic from pipeline state management. One subtlety: when repair is skipped, only "end" with `skipped=True` fires (no "start") — this matches the original pipeline behavior where `_mark_stage` went straight to "completed" without "running". The `_stage_repair` return value is consumed via `.get()` with defaults, so no crash risk if the return dict is sparse.
---

### expert_build/pipeline.py:cmd_derive_review_repair
VERDICT: CONCERN
CORRECTNESS: QUESTIONABLE
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: The function works, but the tests patch `expert_build.llm.check_model_available` instead of `expert_build.pipeline.check_model_available`. Since `pipeline.py` uses `from .llm import check_model_available`, the name is bound in `pipeline`'s namespace — patching at `expert_build.llm` won't affect calls within `pipeline.py` (standard Python "patch where it's used" rule). The tests may pass incidentally (e.g., `test_model_not_available_exits` passes because the real function also returns False for "nonexistent"), but `test_prints_summary` could fail in a CI environment where the real `check_model_available("claude")` returns False. Check whether existing `cmd_pipeline` tests use the same pattern — if they do, this is a latent issue across the file.
---

### expert_build/pipeline.py:cmd_pipeline (refactored convergence section)
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: PARTIAL
INTEGRATION: WIRED
REASONING: The `_pipeline_on_stage` closure correctly maps "start"/"end" events to the original banner + `_mark_stage` calls. The `remaining_rounds` calculation (`args.rounds - start_cycle + 1`) preserves resume semantics. One nuance: the original code saved `state["current_cycle"]` at the top of each loop iteration, and the new code does this in the "start" handler for stage 4 only — this is correct since stage 4 is always the first stage in each cycle. No new tests for the refactored pipeline path, but existing `TestPipelineCommand` tests should cover regression.
---

### tests/test_pipeline.py:TestConvergenceLoop
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: Five tests cover the key behaviors: convergence detection, max rounds exhaustion, repair skip, accumulation across cycles, and the `on_stage` callback contract. The callback test correctly asserts that stage 6 "start" is absent when repair is skipped (0 invalids) while "end" is present. All patches target the correct module path (`expert_build.pipeline._stage_*`).
---

### tests/test_pipeline.py:TestCmdDeriveReviewRepair
VERDICT: CONCERN
CORRECTNESS: QUESTIONABLE
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: Three tests cover error exits and the happy path. However, all three patch `expert_build.llm.check_model_available` — this should be `expert_build.pipeline.check_model_available` per the "patch where it's looked up" rule. `test_model_not_available_exits` likely passes because the real function also returns False for model "nonexistent". `test_prints_summary` is more fragile — it depends on the real `check_model_available("claude")` returning True, which may not hold in CI. Recommend changing the patch target to `expert_build.pipeline.check_model_available`.
---

### SELF_REVIEW
LIMITATIONS: Could not see the existing `TestPipelineCommand` tests to verify whether they use the same (potentially wrong) patch target for `check_model_available` — if they do, the issue is pre-existing and this PR is at least consistent. Could not run the tests to confirm whether the patch target concern is real or theoretical.
---

### FEATURE_REQUESTS
- Show the full existing test class when new tests are added to the same file, to verify consistency of mocking patterns
- Flag patch targets that don't match the import style in the source file (detecting the "patch where it's defined vs. used" mistake)
---
