## Code Review

### expert_build/pipeline.py (new file)
VERDICT: CONCERN
CORRECTNESS: QUESTIONABLE
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: PARTIAL
INTEGRATION: WIRED
REASONING:

1. **`--no-fetch` suppresses `--pdf` chunking (bug)**: `_stage_ingest` returns immediately when `args.no_fetch` is True, skipping PDF chunking even if `--pdf` was explicitly provided. The flag's help text says "Skip fetch-docs (use existing sources/)" — PDF chunking is local file processing, not fetching. A user running `expert-build pipeline --no-fetch --pdf paper.pdf` would expect the PDF to be chunked.

2. **Unused `import re`** (line 3): `re` is never used in this module.

3. **Stage 8 missing**: `total_stages = 9` and the banner jumps from Stage 7 (DEDUPLICATE) to Stage 9 (EXPORT). The `_stage_export` docstring says "Stage 9" confirming this is intentional, but it reads as a gap to anyone watching the output.

4. **`_stage_export` unconditionally overwrites `README.md`** (line 237): This silently destroys any existing README. At minimum, check if it exists and warn, or use a different filename like `EXPERT-CARD.md`.

5. **`_stage_export` inline `import json`** (line 231): Minor — all other imports are either at the top of the file or at the top of the function body. This one is mid-function.

6. **`args.domain` can be `None`** passed to `export_card` and `build_prompt` — cannot verify these handle `None` since `reasons_lib/api.py` was not available in observations.

---

### expert_build/pipeline.py:_stage_extract
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: The `setattr(prop_args, "all", False)` workaround for the `all` builtin is correct. The flow — propose, auto-accept markers, then accept into DB — is logically sound. The `return False` on `--no-auto-accept` properly signals `cmd_pipeline` to stop early. Test `test_auto_accepts_and_imports` covers the happy path and `test_stops_on_no_auto_accept` covers the early-exit.

---

### expert_build/pipeline.py:_stage_derive
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: Correctly loops up to `max_derive_rounds`, detects saturation (no proposals or no valid proposals), handles LLM errors gracefully by breaking, and accumulates `total_added` across rounds. The `isinstance(r, dict)` check on apply results is a reasonable way to distinguish success from error strings. Three unit tests cover empty network, saturation, and valid-proposal paths.

---

### expert_build/pipeline.py:cmd_pipeline
VERDICT: CONCERN
CORRECTNESS: QUESTIONABLE
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: The convergence loop logic is correct — early-exit when `added == 0 and invalid_count == 0`. The concern is that when `--no-auto-accept` is set, the function returns without any export, and the user must manually run the remaining stages. This is documented in stderr output but could be surprising. The `_stage_repair` stage is conditionally skipped when `invalid_count == 0`, which is correct. Full pipeline tests cover convergence, non-convergence, early-exit, and model-unavailable cases.

---

### expert_build/propose.py:auto_accept_proposals
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: COVERED
INTEGRATION: WIRED
REASONING: Simple regex substitution `[ACCEPT/REJECT]` → `[ACCEPT]`. Does not affect already-`[ACCEPT]`ed entries. Does not affect `[REJECT]` entries (none would exist at this point in the pipeline since the LLM generates `[ACCEPT/REJECT]`). Three tests cover the function well: marker replacement, already-accepted preservation, and no-op on marker-free files.

---

### expert_build/cli.py (pipeline registration)
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 properly registered. The `--depth` default of 2 (vs 1 for standalone `fetch-docs`) is an intentional choice for pipeline use. The lazy-load dispatch `"pipeline": lambda a: _lazy("pipeline", "cmd_pipeline")(a)` follows the existing pattern. No CLI-level test (e.g., argparse integration test), but the command is testable through the function-level tests.

---

### tests/test_pipeline.py (new file)
VERDICT: CONCERN
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: PARTIAL
INTEGRATION: WIRED
REASONING:

1. **`MagicMock` imported but never used** (line 5).
2. **No tests for `_stage_summarize` or `_stage_export`** — these are always patched out in the full pipeline tests. An individual test for `_stage_export` would catch the README overwrite concern.
3. **No test for `--no-fetch` + `--pdf` interaction** — the bug where `--no-fetch` suppresses PDF chunking is untested.
4. **No test for URL + PDF combined ingest** — both being provided is a valid use case.
5. **Patch targets are correct** — lazy imports inside stage functions mean the patches on the source modules work as expected.

---

### uv.lock changes
VERDICT: PASS
CORRECTNESS: VALID
SPEC_COMPLIANCE: N/A
ISSUE_COMPLIANCE: N/A
BELIEF_COMPLIANCE: N/A
TEST_COVERAGE: N/A
INTEGRATION: N/A
REASONING: Version bump of `ftl-reasons` from 0.34.0 to 0.43.0 with new optional extras (`cluster`, `mcp`, `api`, `vertex`, `langfuse`, `llm`). The pipeline uses `reasons_lib.api.review_beliefs`, `research`, `deduplicate`, `export_card` — these are likely additions in the newer version. The dependency is editable (`../ftl-reasons`), so the lock change is just tracking the local state.

---

### SELF_REVIEW
LIMITATIONS:
- `reasons_lib/api.py` and `reasons_lib/derive.py` were not available — could not verify signatures, return types, or `None`-handling for `export_card(domain=None)`, `review_beliefs`, `research`, `deduplicate`, `build_prompt(domain=None)`.
- Could not verify whether `export_card` producing a README.md is the expected behavior in the wider project context.
- Could not see how `cmd_summarize` handles the `input_dir` when entries already exist (the pipeline always calls summarize, even on re-runs).
---

### FEATURE_REQUESTS
- Include source for functions in sibling editable-install packages (`reasons_lib/api.py`, `reasons_lib/derive.py`) when they are imported and called by the diff under review.
- Flag unused imports automatically in the observation data.
---
