Add LegalBench-RAG benchmark harness + parity report (composite of #1239/#1353/#1354/#1376)

[JSv4]

Summary

Composite branch built on top of #1239 (benchmark harness), #1353 (paragraph chunker), #1354 (reranker framework), and #1376 (corpus-isolation fix). On top of those merges, this PR adds:

• LegalBench-RAG-parity char-level recall/precision metrics
• --retrieval-only and --corpus-wide benchmark modes
• LLM-token instrumentation parsed from Datacell.llm_call_log
• Per-subset aggregates with equal-weight macro avg matching the paper's protocol
• OpenAIEmbedder input truncation (was 400ing on inputs >8192 tokens)
• Auto-grounding fuzzy-matcher hardening: per-query timeout, n-gram anchor pre-filter, tighter doc/query length caps
• similarity_top_k plumbed from CLI through to the agent's doc_extract_query_task
• Extract-prompt fix + None-failure-mode classifier — see "Agent failure-mode fix" below
• VECTOR_EMBEDDER_API_KEY wired through local.yml
• Comprehensive benchmark report at docs/benchmarks/legalbench_rag_results.md
• 90 unit tests pass

Scope of the LegalBench-RAG comparison

The paper measures single-shot retrieval only (no LLM, no agent, no grounding pass). Our headline claim against the paper is at that exact protocol — single-shot top-k vector search, character-level recall/precision against gold, on the corpus-wide retrieval surface that mirrors legalbenchrag/methods/baseline.py:193.

Our metric	Comparable to LB-RAG paper?
probe_char_recall / probe_char_precision	✅ Yes — exact formula and regime parity
citation_char_recall / citation_char_precision	❌ No analog (paper has no agent loop)
answer_token_f1	❌ No analog (paper does not generate answers)
extraction_success_rate	❌ No analog

Paper-comparison claims here always refer to probe char_recall vs the paper's published tables, at matched k, using the paper's exact formulas. Agent-pipeline numbers are reported separately as scope-extension and not part of any paper-comparison claim.

Headline result — retrieval-only, corpus-wide, k=32

Apples-to-apples retrieval comparison (no agent, no LLM, single-shot top-k):

Subset	Ours (MiniLM + paragraph + no rerank)	Paper best	Δ
privacy_qa	78.4%	71.2%	+7.2
contractnli	100.0%¹	69.9%	+30.1
cuad	60.2%	64.4%	−4.2
maud	26.1%	22.6%	+3.5
Equal-weight macro avg	66.2%	57.0%	+9.2 pts

¹ contractnli's 194-task slice covers only 20 docs of ~10 KB each; k=32 retrieves a sizeable fraction of any given document. The paper's fixed-500 chunker on the same slice still got 69.9%.

Agent pipeline (scope-extension, not LB-RAG-comparable)

Full OpenContracts pipeline (retrieval + iterative agent + structured extraction + citation grounding) on the same task slices.

Agent failure-mode fix

The "extraction returned None" outcome on doc_extract_query_task was hiding three distinct failure modes under one error message:

1. agent_committed_none — agent searched, decided absent, returned None. Legitimate.
2. no_final_response — agent issued a tool call, the message log ends there with no tool-return / no synthesis. The pydantic-ai loop exited without producing a structured answer. Pipeline bug, not a data signal.
3. tool_loop_no_output — agent stuck issuing the same tool call without ever synthesising an answer.

The structured-extraction system prompt at all three sites in pydantic_ai_agents.py said "If the information cannot be found using the tools, return null/None." Combined with output_type=str and gpt-4o-mini's eagerness to satisfy structured output schemas, this gave the agent a license to bail after a single search. Tightened to require 2-3 distinct queries before concluding absence. Cross-subset before / after at k=32:

Metric	privacy_qa Δ	contractnli Δ	cuad Δ
extraction_success_rate	−5.7 pts	+21.1 pts	+26.3 pts
citation_char_recall	+0.247	+0.211	+0.248
answer_token_f1	+0.100	+0.188	+0.101
answer_contains_verbatim_span	+0.142	+0.323	+0.116
input_tokens / task	+60%	+160%	+7%
llm_calls / task	−0.64	−0.12	−1.0

Quality wins on every subset on every quality axis. LLM call counts actually drop because the agent commits faster once it has data, instead of looping. New _classify_none_result helper in data_extract_tasks records the failure mode in the cell's stacktrace so operators can grep failure_mode= to separate legitimate "data not present" outcomes from pipeline bugs.

Model sweep (privacy_qa, post-prompt fix not yet rerun for gpt-4o / sonnet)

Model	ok%	citation_char_recall	citation_char_prec	char_F1	answer_F1	tokens/task
Probe (no agent)	n/a	0.784 (probe)	0.009 (probe)	0.019	—	0
gpt-4o-mini	1.000	0.425	0.128	0.197	0.242	10,096
gpt-4o	0.985	0.336	0.163	0.220	0.263	13,490
claude-sonnet-4-6 ⚠️	0.129	0.100	0.011	0.020	—	6,881

⚠️ Sonnet success rate is 13% — pydantic-ai integration issue, see #1381. Stronger model = MORE selective curation (lower recall, higher precision), not broader. By design.

What's NOT in this PR

• A working reranker. PR Add post-retrieval reranker framework for vector search #1354's BGE and MiniLM cross-encoders both hurt retrieval quality in our agent loop — see Evaluate effective rerankers + revisit reranker integration in agent loop #1378.
• CUAD chunker tuning. We lose ~4 pts on CUAD vs the paper's fixed-500 chunker; sliding-window or RCTS sweep on that subset is a follow-up.
• Fixed Anthropic integration. Sonnet doesn't reliably commit to structured output after tool calls. Tracked in Anthropic models silently fail in doc_extract_query_task — return tool calls/thinking instead of structured output #1381.
• Answer-quality benchmarking against derivative work. The original LB-RAG paper doesn't score answer quality, so the agent's answer_token_f1 has no external reference point. Tracked in Benchmark answer quality (not just retrieval) against derivative LegalBench-RAG work #1382.

Test plan

• python manage.py test opencontractserver.tests.test_benchmarks — 36 tests pass
• python manage.py test opencontractserver.tests.test_text_alignment — 24 tests pass (5 new in TestFuzzyHardening)
• python manage.py test opencontractserver.tests.test_extraction_grounding — 24 tests pass
• python manage.py test opencontractserver.tests.test_corpus_isolation_vector_store — 6 tests pass (added by Scope structural annotations to the queried corpus in vector store #1376)
• Full LegalBench-RAG retrieval-only run on all 4 subsets at k=32 — see docs/benchmarks/legalbench_rag_results.md
• Pre/post prompt-fix agent runs on privacy_qa + contractnli + cuad — see report
• Model sweep on privacy_qa: gpt-4o-mini, gpt-4o, sonnet-4-6 — see report

[claude]

Code Review - PR #1380

This is a large composite PR (benchmark harness + paragraph chunker + reranker framework + corpus-isolation fix). The code is well-structured and the test plan is thorough. Below are specific findings, ordered by severity.

---

Bugs

runner.py - finished_at missing from in-memory BenchmarkReport
BenchmarkReport is constructed from a dict(config) snapshot before config["finished_at"] is set. The snapshot therefore never contains the finish timestamp. The JSON on disk gets it correctly (via _write_run_config), but any caller inspecting report.config["finished_at"] programmatically will get a KeyError. Set config["finished_at"] before constructing the report, or populate report.config directly after construction.

metrics.py - token_recall("", "") returns 1.0; char_recall("", "") returns 0.0
Asymmetry is documented as following SQuAD convention, but empty gold in a benchmark run is almost always a data-quality bug. A row where both prediction and gold are empty will silently inflate aggregate F1. Consider returning 0.0 for the empty-gold case and emitting a warning, rather than silently treating it as a perfect hit.

legalbench_rag.py - random.seed() called inside a sort-key function
_paper_sample_tests builds a sort key that calls random.seed(fp) then random.random(). Calling random.seed inside a key function that sort() invokes repeatedly mutates process-wide random state on every comparison. It happens to be deterministic here (seed -> one draw), but any concurrent code relying on random in the same process will be affected. Precompute the keys as a dict: keys = {t: (random.seed(fp) or random.random()) for t in tests}, then sort by keys[t].

---

Security

local.yml - API_KEY: abc123 committed to version control
The vector-embedder service now gets API_KEY: abc123 in local.yml. The multimodal-embedder already had the same pattern before this PR, so this follows existing precedent -- but both values should live in a gitignored .env or .env.local file, not in the compose file. At minimum add a comment that this must be overridden before any network-accessible deployment.

data_extract_tasks.py - model_override is unvalidated
doc_extract_query_task accepts model_override: str | None and passes it straight to the agent. If this task is ever exposed to user-controlled input (webhook, API surface), an attacker could direct extraction traffic to an arbitrary model endpoint. Current call sites (CLI/runner) are controlled, but an allowlist or at least a short comment noting the trust assumption would be appropriate.

---

Performance

retrieval.py - _resolve_doc_id cache is call-local; N+1 queries across a full run
The struct_set_to_doc cache that avoids repeated DB lookups inside a single probe_retrieval call is not shared across calls. A full benchmark run (776 tasks x k=32 hits) can trigger hundreds of Document.objects.filter(...) queries for structs not yet seen. Elevating the cache to module-level (keyed by (corpus_id, struct_set_id)) would amortize this cost across a run.

text_alignment.py - doc_text.lower() recomputed on every _has_anchor_ngram call
For a 200 KB document at MAX_DOC_LENGTH_FOR_FUZZY, each call allocates a new 200 KB string. The lowercased doc should be computed once per align_text_to_document invocation and passed down, rather than recomputed on every anchor check.

pipeline/utils.py - Reranker instance-cache key requires a DB round-trip on every lookup
The cache key includes PipelineSettings.modified, fetched from the DB on each call to _get_reranker_cache_key. A Django cache-backed value (already used for embedder settings elsewhere) would avoid the per-lookup DB query on what is supposed to be the warm-cache hot path.

---

Style / Convention

run_benchmark.py - magic number 194 should use the named constant
PAPER_MAX_TESTS_PER_BENCHMARK = 194 is defined in legalbench_rag.py and already imported at the top of run_benchmark.py. Both the default=194 in add_argument and the options.get("max_per_subset", 194) fallback should use default=PAPER_MAX_TESTS_PER_BENCHMARK instead of the literal. Violates the no-magic-numbers rule in CLAUDE.md.

constants/benchmarks.py - BENCHMARK_QUERY_PREVIEW_TRIM_LEN is a literal instead of a derived value
The comment explains it equals BENCHMARK_QUERY_PREVIEW_MAX_LEN - 1, but it is defined as 63. A future maintainer changing MAX_LEN will likely miss updating TRIM_LEN. Define it as BENCHMARK_QUERY_PREVIEW_TRIM_LEN = BENCHMARK_QUERY_PREVIEW_MAX_LEN - 1.

cross_encoder_reranker.py - variable s shadows the settings object
In _rerank_impl, s = self._effective_settings() is defined at the top, then later score_list = [float(s) for s in scores] reuses s as a loop variable. The settings object is no longer needed at that point so there is no runtime impact, but it will trip a linter. Rename the comprehension variable to score.

data_extract_tasks.py - f-strings in logger calls
New log lines use f-strings (e.g. logger.info(f"Linked {n} retrieval citations...")). The rest of the codebase uses %-style lazy formatting (logger.info("Linked %d citations", n)) which defers string construction when the log level is suppressed. Please align for consistency.

text_chunkers.py - _INVISIBLE_CHARS_RE Unicode range is too broad
The current range covers the General Punctuation block, including en dash, em dash, thin space and other characters with legitimate typographic meaning. A more targeted allowlist -- ZWSP, BOM, soft-hyphen, word joiner, Mongolian vowel separator -- avoids stripping content characters from financial or legal documents.

---

Test Coverage Gaps

• _classify_none_result (data_extract_tasks.py) -- no unit test for each of its four failure modes. The classifier's operational value is as a diagnostic tool; a small unit test with mock message lists would protect it.
• _link_retrieval_citations (data_extract_tasks.py) -- similarly untested in isolation.
• CrossEncoderReranker -- test_reranker.py has a comment block for it but no tests. At a minimum, mock the model load and test the scoring/ranking logic.
• Benchmark retrieval_only=True code path -- test_benchmarks.py exercises the agent path but not the retrieval-only mode through run_benchmark.

---

Positive Highlights

• The failure-mode classifier (_classify_none_result) is a thoughtful addition; separating "data absent" from pipeline bugs in Datacell.stacktrace makes production debugging significantly easier.
• The _has_anchor_ngram pre-filter and per-query timeout in text_alignment.py address real production reliability problems in fuzzy-matching.
• force_celery_eager() context manager in loader.py and the per-run BenchmarkReport artifact structure are clean, reusable abstractions.
• 90 tests passing, benchmark results committed with reproducible run artifacts -- the traceability between report numbers and the committed JSON files is excellent practice.

[claude]

Code Review — PR #1380 (LegalBench-RAG benchmark harness + parity report)

This is a thorough composite PR. The engineering quality is generally high, the benchmark methodology is honest (the self-correction on the inflated "+24.9 pts" headline is particularly good), and most of the production code changes are well-reasoned. Below are the issues I found, grouped by severity.

---

Blockers / Must-Fix

1. Agent pipeline is broken and documented as such

The committed benchmark report states: extraction_success_rate: 0.0013 (1 of 776 cells succeeded). That's a 99.9% failure rate on the production path this PR is supposed to validate. The no_final_response failure mode (agent issues tool calls, pydantic-ai loop exits without producing a final structured output) happens on 775/776 cells.

The PR description says this is tracked in #1381, but the PR is also still introducing the prompt-tightening that was supposed to fix it — and that fix apparently doesn't work on the current branch. Merging a PR that claims to deliver a working pipeline while shipping committed benchmark artifacts that prove the pipeline is broken creates a confusing paper trail. Options:

• Fix the pydantic-ai loop exit issue before merging
• OR clearly split off all agent-pipeline claims into a follow-up PR and explicitly scope this PR to retrieval-only validation

2. ~724K additions are large committed data artifacts

The net change count is dominated by gold.json, report.json, and report.csv files under docs/benchmarks/runs/. These are multi-MB benchmark run artifacts that permanently bloat git history for every future clone. The MANIFEST.md rationale ("every number resolves to a committed artifact") is sound, but the implementation should use Git LFS for the large files or store them externally (S3/GCS/Dropbox with a pointer in MANIFEST.md). The gold.json files also contain verbatim text excerpts from legal contracts — if LegalBench-RAG has redistribution constraints, committing them verbatim creates a licensing concern.

---

Significant Issues

3. TODO placeholder left in committed documentation

docs/benchmarks/legalbench_rag_results.md line 313:

[PR #1380 audit thread]: TODO replace with link

This is committed as-is. Replace or remove before merge.

4. Inline import inside mutate() body

config/graphql/pipeline_settings_mutations.py lines 152–155 inside mutate():

from opencontractserver.pipeline.utils import (
    invalidate_reranker_cache,
)

This should be a module-level import. Inline imports inside mutation handlers are surprising and complicate tooling (import linters, refactors). The import is cheap so there's no lazy-loading justification here.

5. DEFAULT_EMBEDDER env-override in test settings creates test hermeticity risk

config/settings/test.py now reads:

DEFAULT_EMBEDDER = env("DEFAULT_EMBEDDER", default="...TestEmbedder")

If a developer has DEFAULT_EMBEDDER set in their shell environment (e.g., from a local benchmark run that exports it), all subsequent docker compose -f test.yml runs will silently use the real embedder instead of TestEmbedder. The comment correctly explains the intent, but the mechanism is fragile. Consider a separate settings file (e.g. config/settings/benchmark.py) for benchmark runs rather than polluting the test environment.

6. MAX_DOC_LENGTH_FOR_FUZZY reduced from 500,000 to 200,000 without impact assessment

The reduction is a production behavior change: documents between 200–500 KB will now skip fuzzy grounding entirely. The comment correctly notes that the timeout and n-gram filter are the "real safety valves," but the cap reduction is a regression in recall for large legal documents (EPC agreements, ISDA schedules mentioned in the same comment routinely exceed 200 KB). This change needs an explicit statement in the PR of what the measured impact on production extraction recall is, or the cap should be kept at 500 KB and the timeout + n-gram filter relied on exclusively.

---

Minor / Polish

7. _classify_none_result tests

The function is well-implemented and the logic for the three failure modes is correct. But given its operational importance — operators will grep for failure_mode=no_final_response to distinguish pipeline bugs from legitimate "not in document" outcomes — it should have explicit unit tests covering each case (empty messages, agent_committed_none, no_final_response, tool_loop_no_output). The benchmark test file covers many other things but I don't see this function explicitly covered.

8. get_structured_response_and_sources_from_document is a thin wrapper with duplicated kwargs

llms/api.py has this new method that takes the same kwargs as get_structured_response_from_document and calls for_document then structured_response in the same sequence. The only addition is capturing retrieved_annotation_ids from agent_deps. This seems like a candidate for merging into the existing method (e.g., always return a tuple, or add an include_sources: bool = False parameter) rather than a parallel method. With two nearly-identical code paths, future changes to the kwargs list will need to be applied in both places.

9. OpenAIEmbedder.api_batch_size = 256 is a 5x increase over historical 50

The comment explains the math (256 × ~375 tokens ≈ 96K tokens/call, well under the 8M cap). But this is a behavior change that may hit RPM limits on Tier-1 accounts (3000 req/min ÷ 256 = ~12 effective calls/min for a corpus ingest). For teams running parallel corpus ingests this could cause unexpected 429s that the 8-retry budget won't cover. At minimum this should be documented in the CHANGELOG as a tunable config change rather than a silent class-level constant.

10. rerank_oversample_factor applied unconditionally in vector store

core_vector_stores.py applies RERANK_OVERSAMPLE_FACTOR (×3) to top_k whenever a reranker is configured. At similarity_top_k=32 (benchmark default) this fetches 96 candidates from pgvector before reranking. At high k values (e.g., top_k=100) this would request 300 candidates against a RERANK_MAX_CANDIDATES=128 cap, silently truncating at 128. The logic in the base class handles this correctly (warns and truncates), but the user-visible behavior — "I asked for top-100 with reranking and got 128 candidates reranked to 100" — is surprising. Consider documenting this trade-off in the GraphQL default_reranker field description.

11. Process-local reranker instance cache cross-worker coherence note

pipeline/utils.py documents that the cache key includes PipelineSettings.modified and relies on the shared Django cache (Redis) to propagate changes. The comment says convergence happens "within Django's PipelineSettings cache TTL (5 minutes)." A 5-minute window where some workers use the old reranker and others use the new one after a settings change is probably fine for this use case, but it's worth making this explicit in operator documentation (e.g., in docs/deployment/performance_tuning.md).

12. _has_anchor_ngram called with doc_lower_cached before it's populated on CUAD

text_alignment.py:align_text_to_document initializes doc_lower_cached = None and only populates it inside the len(doc_text) > MAX_DOC_LENGTH_FOR_FUZZY else-branch for each query. If the document is large enough to skip fuzzy (falls into the continue branch), doc_lower_cached never gets set for that document and subsequent queries also skip correctly. But if the document is small and the FIRST query skips for a different reason (e.g., short query length), doc_lower_cached gets set on the SECOND call. This is correct behavior but reads confusingly — a brief comment clarifying that doc_lower_cached is intentionally lazily initialized (and that the initialization site is inside the fuzzy-eligible block) would help reviewers.

---

Positives Worth Calling Out

• The two-phase pre_delete + post_delete GC for StructuralAnnotationSet is correctly implemented and solves a real contamination bug.
• The _classify_none_result failure-mode classifier is exactly the right abstraction: it turns an opaque None into an actionable diagnostic that operators can grep for.
• safe_rerank / safe_arerank fault-tolerance design is solid — reranker failures silently degrade to first-stage ordering rather than breaking search.
• The fuzzy matcher hardening (per-query timeout, n-gram anchor pre-filter, query length cap) directly addresses the grounding latency issues flagged in prior PRs without changing the happy-path behavior.
• The citation-capturing similarity_search wrapper (_make_similarity_search_tool) cleanly separates citation accumulation from the search logic and avoids mutating the tool's return value.
• The benchmark methodology correction (retracting the "+24.9 pts" headline and documenting the retrieval budget disparity) is exactly the intellectual honesty this kind of work needs.

---

Summary: The retrieval-only harness and the production code improvements (chunker, reranker framework, fuzzy hardening, GC signal, failure-mode classification) are solid. The main blockers are the broken agent pipeline shipped alongside agent-pipeline claims, the committed data artifacts, and the unresolved TODO. I'd recommend resolving #1 and #2 before merge (or explicitly re-scoping to retrieval-only), and addressing #3–#6 in the same pass.

[claude]

Code Review — PR #1380

This is a large, well-motivated composite PR. The scientific integrity in the benchmark results section is commendable — proactively retracting prior headline numbers and explaining the three root causes (annotation contamination, sampling drift, metric drift) is exactly the right behaviour. The overall architecture for the reranker framework, text chunker registry, and benchmark harness is solid. Below are findings from most-to-least critical.

---

Blockers

1. Broken agent pipeline shipped in committed docs

docs/benchmarks/legalbench_rag_results.md (line ~307) states:

The agent layer of the production pipeline is currently broken on this branch (only 1 of 776 cells succeeds end-to-end; pydantic-ai exits the loop after a multi-tool-call message without producing a final structured output — same no_final_response failure mode …)

This is committed as a permanent artifact in docs/benchmarks/runs/. If the agent pipeline is known-broken in this branch, that's a regression from main. The fix is tracked in #1381, but merging a branch that documents its own production regression should be gated on that issue landing first, or the result doc should clearly mark the agent rows as "pending #1381 fix" rather than stating the pipeline is broken on the current branch.

2. Unfixed TODO in committed documentation

docs/benchmarks/legalbench_rag_results.md, line ~313:

[PR #1380 audit thread]: TODO replace with link

This will render as a broken Markdown link reference forever once merged.

---

Significant Issues

3. ThreadPoolExecutor + Django ORM in _run_extraction (runner.py)

_run_extraction submits doc_extract_query_task.apply(args=[cell_id], kwargs=kwargs) to a ThreadPoolExecutor. That task does extensive ORM work (reads/writes to Datacell, annotations, etc.) in each worker thread. The performance tuning docs correctly note that "DB writes staying in the calling thread" is the safe pattern for the embedder, but that isolation does not apply here — each worker thread calling .apply() will lazily open a Django DB connection that is never explicitly closed. The docs acknowledge this risk:

Doing ORM writes from worker threads is correct but requires explicit connections.close_all() at thread exit, otherwise idle connections accumulate against max_connections. We sidestepped this by keeping DB writes in the calling thread.

The benchmark runner does not sidestep it — it hands full ORM-writing tasks to threads. Recommend either: (a) add a try/finally: django.db.connections.close_all() inside _run_one, or (b) add a note that concurrency > 1 is explicitly unsupported for production-scale runs and document the leak.

4. _classify_none_result tight coupling to pydantic-ai internals

data_extract_tasks.py's _classify_none_result classifies failure modes by introspecting ToolCallPart, ToolReturnPart, etc. from pydantic-ai's message history. This is fragile: any pydantic-ai internal rename or message-structure change will silently fall through to no_final_response without a test failure. Two requests:

• Pin the exact pydantic-ai version in requirements/base.txt (or equivalent) alongside a comment linking back to this function.
• Add an explicit assert or isinstance guard that surfaces an unexpected type clearly, rather than silently reclassifying.

5. model_override trust surface on a Celery task

doc_extract_query_task now accepts model_override: str | None and passes it unvalidated into pydantic-ai's model selection. The comment acknowledges:

If this is ever wired to a user-facing API, run it behind an allowlist of approved model identifiers.

Celery tasks can be triggered via Celery Beat, management commands, and in some deployments via admin interfaces that accept arbitrary kwargs. Recommend adding the allowlist now rather than after the fact — a frozenset of approved model strings at the top of the task module is a two-line change that closes this path entirely, and the benchmark runner can just assert its chosen model is in the set.

---

Minor Issues

6. DEFAULT_EMBEDDER env-override in config/settings/test.py

DEFAULT_EMBEDDER = env("DEFAULT_EMBEDDER", default="...TestEmbedder")

If DEFAULT_EMBEDDER is present in a developer's shell environment (dotenv, CI secret leaked to environment, etc.), regular test runs will silently use a real embedder and make live API calls. The comment says "standard CI never sets DEFAULT_EMBEDDER" — that's currently true but fragile. Consider using a more specific env var name like BENCHMARK_DEFAULT_EMBEDDER that is unlikely to collide, and keep the test setting unconditionally pointing to TestEmbedder.

7. Inconsistent empty-input edge cases in metrics.py

• token_f1([], []) → 1.0 (SQuAD convention, documented)
• token_recall([], []) → 0.0 with a warning

This inconsistency means a query where both prediction and gold are empty strings scores F1=1.0 but recall=0.0 — a contradiction. Either follow SQuAD convention consistently (token_recall([], []) = 1.0) or document in the function docstring why the asymmetry is intentional. Benchmark aggregates that average F1 and recall will get subtly different denominators for the empty case.

8. Character-count truncation in OpenAIEmbedder (openai_embedder.py)

text = text[:OPENAI_EMBEDDER_MAX_INPUT_CHARS]

Truncating by character count rather than token count will still exceed 8192 tokens for dense non-ASCII text (e.g., Chinese/Japanese legal documents). The fix is pragmatic and solves the Latin-script case, but the limit should be noted in OPENAI_EMBEDDER_MAX_INPUT_CHARS's constant comment as an approximation. A tighter option is to truncate to 8000 * 4 bytes (rough token approximation) or use tiktoken for the OpenAI-specific path.

9. safe_rerank/safe_arerank swallow silently

These helpers return None on any exception — including misconfiguration (bad class path, missing API key, etc.). The docstring says fault-tolerance is the intent, but an operator who accidentally sets an invalid default_reranker path will get zero reranking with no visible error unless they know to watch logs. Suggest adding a single logger.warning(...) in the exception handler so misconfiguration produces at least one visible signal per request.

10. SentenceChunker import failure surface

If spaCy is not installed, SentenceChunker.chunk() will raise ImportError deep in the lazy-load path with a stack trace pointing into the cache lock block. A cleaner UX would be a try/except ImportError at the top of the module that raises ImportError("spaCy is required for SentenceChunker; pip install spacy && python -m spacy download en_core_web_sm") at class definition or at chunk() entry.

11. docs/benchmarks/legalbench_rag_results.md — "a Karen demands both axes" (line ~417)

This is informal/potentially offensive section header language. Suggest replacing with "Per-subset character precision (both axes)" or similar.

---

Positive Notes

• The _finalize_results() dedup + out-of-range index validation in BaseReranker is robust and the right place to enforce invariants.
• Per-query wallclock deadline in _fuzzy_find is a genuine correctness fix — unbounded fuzzy matching on large documents was a real latency risk.
• The anchor n-gram pre-filter is a well-targeted performance optimization with a clean fallback (n=0 disables it).
• Equivalence-testing the metric implementation against a vendored copy of the upstream library (200 randomized trials) is thorough and the right way to validate a metric port.
• The !docs/benchmarks/runs/ gitignore exception is a smart way to commit reproducibility artifacts without polluting the general exclusion pattern.
• The _gc_orphan_structural_set pre/post-delete signal pattern is correct: stashing the FK in pre_delete before Django nulls it out, then acting in post_delete, handles the Django cascade ordering properly.
• The retraction and self-correction in the benchmark report is unusually honest for a PR and sets a good precedent.

---

Summary

The infrastructure work (reranker framework, chunker registry, benchmark harness, fuzzy-match hardening) is high quality and ready to merge. The two blockers — known agent regression in committed docs (#1381) and the unfixed TODO link — should be resolved before merge. Items 3–5 are worth addressing before merge for correctness and security reasons. Items 6–11 can be filed as follow-up issues if timeline pressure is high.

[claude]

Code Review — PR #1380: LegalBench-RAG Benchmark Harness + Parity Report

Scope note: This is a large composite PR (~10,650 additions) merging four feature branches. I've focused the review on the novel code added on top of those merges: the benchmark runner/metrics/retrieval, fuzzy-match hardening, None-failure classifier, chunker registry, reranker base, and OpenAI embedder fixes.

---

Overview

The PR delivers a well-structured benchmark infrastructure that integrates cleanly with the existing pipeline. The parity methodology is unusually rigorous — the PR proactively audits its own earlier claims, calls out the retrieval-budget caveat for Config C, and documents which metrics are paper-comparable vs. scope-extension. That intellectual honesty is a feature.

---

Issues

Medium — _classify_none_result depends on undocumented pydantic-ai internal identifiers

data_extract_tasks.py classifies failure modes by pattern-matching on part_kind strings ("response", "tool-call", "thinking", "output_tool"). These are private message-structure fields not guaranteed by pydantic-ai's public API.

# data_extract_tasks.py — _classify_none_result
response_msgs = [m for m in messages if getattr(m, "kind", None) == "response"]
last_part_kinds = [getattr(p, "part_kind", "?") for p in last_parts]

A pydantic-ai minor version bump could silently change these strings, making all results land in "empty_history" with no test failure. Recommend pinning the pydantic-ai version in requirements (or noting the tested version in a comment), and adding a smoke-test assertion that a known success case produces "agent_committed_none" (not just the mock-message tests that already exist).

---

Medium — _STRUCT_SET_TO_DOC process-global cache not auto-cleared in tests

retrieval.py has a module-level dict that caches (corpus_id, struct_set_id) → document_id resolutions:

_STRUCT_SET_TO_DOC: dict[tuple[int, int], int | None] = {}

A _clear_struct_set_cache() hook exists but must be called explicitly. If any test destroys a corpus and recreates objects without clearing this cache, it will return a stale document_id from the previous test's objects and produce mysterious failures. The pattern in this codebase for similar caches is to clear in setUp. This is worth enforcing with a TestCase.setUp call in test_benchmarks.py.

---

Medium — RateLimitError is no longer silently swallowed by OpenAIEmbedder

Before this PR, openai_embedder.py had:

except openai.RateLimitError:
    logger.error("OpenAI API rate limit exceeded.")
    return None

This was removed. After this PR, the SDK now retries 8 times before raising. If all 8 retries fail, the exception propagates instead of returning None. This is a behavior change for all callers of embed_texts/_embed_text_impl — callers that expected None on sustained rate-limit will now get an unhandled exception. Verify that embeddings_task.py's Celery retry wrapper catches this case correctly, or document the new behavior in CHANGELOG.

---

Low — model_override in doc_extract_query_task lacks structural enforcement of its access constraint

The docstring correctly notes:

"If this task is ever exposed to user-controlled input (webhook, public API), gate it behind an allowlist of approved model identifiers"

This is good documentation, but the constraint is advisory only. Since Celery tasks can be called via .delay() / .apply_async() from anywhere in the codebase, there's nothing structurally preventing a future contributor from calling doc_extract_query_task.delay(cell_id=..., model_override=user_supplied_string). Consider adding a compile-time ALLOWED_EXTRACT_MODELS: frozenset[str] constant in constants/extraction.py and a ValueError guard at the top of the task — this makes the constraint self-enforcing without changing the current behavior for operator-controlled callers.

---

Low — api_batch_size = 256 is a 5× behavior change not mentioned in CHANGELOG

The comment says "Raising this collapses HTTP-call count by 5× relative to the historical 50." This is a meaningful throughput change for existing users of OpenAIEmbedder, and the embed_max_concurrent_sub_batches = 4 parallel sub-batch is also new. Both should appear in CHANGELOG under "Changed" so operators can audit their rate-limit budgets.

---

Low — _paper_sample_tests mutates global random state as a side effect

The implementation faithfully replicates the upstream sort key (random.seed(fp), random.random())[1], which is good for parity. But random.seed() is a global side effect — calling _paper_sample_tests changes the subsequent output of Python's default random for the rest of that process. If test order or test parallelism ever causes _paper_sample_tests to interleave with any code that uses random.random() (e.g. test fixtures), results will be subtly non-reproducible. Consider using a local random.Random(seed) instance instead:

import random as _random

def _deterministic_key(fp: str) -> float:
    rng = _random.Random(fp)
    return rng.random()

This is fully compatible with upstream's intent (same seed→float mapping) without contaminating global state.

---

Positive Observations

• Failure-mode classifier (_classify_none_result): Excellent operability improvement. Separating "agent said absent" from "loop bug" is exactly the right thing to do, and the structured failure_mode= log tag makes grepping actionable.

• Fuzzy-match hardening: The combination of per-query timeout (FUZZY_PER_QUERY_TIMEOUT_SECONDS = 2.0), n-gram anchor pre-filter, and max-query-length cap is the right layered defense. The time.monotonic() deadline applied to both the outer window loop and the inner refinement pass is thorough.

• Citation capture via _make_similarity_search_tool closure: Capturing annotation_ids into ctx.deps.retrieved_annotation_ids without changing the tool's return contract is a clean approach — the tool signature is unchanged, event handlers that key on tool name continue working, and the M2M linkage is a separate pass.

• Text chunker registry: The register_chunker decorator + get_chunker(spec) pattern is clean and consistent with the pipeline's existing component-registration approach. The SENTENCE_CHUNK_LABEL / PARAGRAPH_CHUNK_LABEL / SLIDING_WINDOW_CHUNK_LABEL module constants are the right way to avoid string coupling.

• DEFAULT_EXTRACT_MODEL constant: Moving "openai:gpt-4o-mini" out of the task body and into constants/extraction.py is the right DRY call — previously the same string appeared in three places.

• Embedder truncation fix: Logging a warning before truncating at OPENAI_EMBEDDER_MAX_INPUT_CHARS is the right behavior — silent truncation was causing opaque 400s.

• Test coverage: 90 tests across six test modules for new code. TestUpstreamEquivalence with 200 randomized trials for the metric equivalence check is particularly solid.

• docs/benchmarks/legalbench_rag_results.md: The second-pass audit that recategorizes Config C from "fair paper comparison" to "ablation at different operating point" is exactly the kind of methodological rigor that makes benchmark reports trustworthy.

---

Summary

The core infrastructure is sound and well-tested. The four medium/low issues above are worth addressing before merge — particularly the RateLimitError propagation change (could cause silent task failures in production) and the pydantic-ai private-API dependency in _classify_none_result (could silently degrade observability on an upstream update). The rest are minor hardening suggestions. The benchmark parity methodology and the failure-mode classifier are genuine quality improvements.

[claude]

Code Review — PR #1380 (LegalBench-RAG benchmark harness + parity report)

This is a substantial composite PR (~10.7K additions) merging four branches plus new work. The overall quality is high — the PR description is unusually thorough and honest (including proactively retracting earlier incorrect benchmark claims). Below is an analysis by area.

---

Overview

The PR delivers:

1. Benchmark harness (opencontractserver/benchmarks/) — a pluggable adapter-based infrastructure for running production pipeline against external datasets
2. Reranker framework (pipeline/base/reranker.py, pipeline/rerankers/) — four backends with fault-tolerant helpers
3. Text chunker strategies (pipeline/parsers/text_chunkers.py) — SentenceChunker, ParagraphChunker, SlidingWindowChunker
4. Agent failure-mode classifier (_classify_none_result) — separates "data absent" from pipeline bugs
5. Embedder hardening — connection pooling, retry logic, batching improvements
6. Fuzzy matcher hardening — per-query timeout, n-gram anchor pre-filter
7. Orphan StructuralAnnotationSet GC — pre/post-delete signal pair
8. LegalBench-RAG benchmark results — including honest post-hoc methodology corrections

---

Correctness Issues

1. force_celery_eager() uses deprecated Celery 4 attribute form

In opencontractserver/benchmarks/loader.py:

conf.task_always_eager = True       # Celery 4 attribute name
conf.task_eager_propagates = True

But the test suite correctly uses the Celery 5 form:

@override_settings(CELERY_TASK_ALWAYS_EAGER=True, CELERY_TASK_EAGER_PROPAGATES=True)

In Celery 5, task_always_eager was removed as a runtime-settable attribute. On Celery 5, the force_celery_eager() context manager will silently do nothing (setting attributes on the conf that have no effect) and benchmarks will fail non-deterministically waiting for tasks that never run eagerly. Should use override_settings in test contexts and verify the Celery version your production stack uses.

2. MAX_DOC_LENGTH_FOR_FUZZY reduced from 500K → 200K without a migration note

# constants/extraction.py
MAX_DOC_LENGTH_FOR_FUZZY = 200_000  # was 500_000

Any document between 200K–500K characters will silently drop fuzzy alignment and fall back to exact + normalized matching. For many CUAD/MAUD commercial contracts this is the corpus — production deployments upgrading from the previous version will see citation grounding quality changes without any warning. This should be noted in CHANGELOG.md under Changed.

3. _link_retrieval_citations races on annotation deletion

existing = set(
    Annotation.objects.filter(id__in=valid_ids).values_list("id", flat=True)
)
existing_ids = [aid for aid in valid_ids if aid in existing]
if existing_ids:
    datacell.sources.add(*existing_ids)

The filter → add is two separate queries with no transaction wrapping. An annotation deleted between the filter and add will cause an IntegrityError (FK violation). This is a real race in concurrent workloads. Wrap in a try/except IntegrityError or use select_for_update().

---

Security

4. model_override needs an issue for future gating

The docstring correctly flags this:

"If this task is ever exposed to user-controlled input (webhook, public API), gate it behind an allowlist of approved model identifiers."

This is good documentation but should have a GitHub issue filed now, before the benchmark harness matures. The path from "benchmark CLI only" to "webhook-triggered" is short in active projects, and an unbounded model identifier can redirect extraction traffic to arbitrary endpoints. Recommend filing a tracking issue.

5. Path traversal protection in _load_document is good

The absolute.relative_to(self.corpus_dir) check is the correct defense. No issues here.

---

Performance

6. Reranker cache cross-worker propagation lag is 5 minutes

pipeline/utils.py documents:

"Cross-worker coherence: the cache key includes PipelineSettings.modified. Every config change bumps that timestamp, which propagates to all workers via PipelineSettings' Django cache (shared Redis). The next lookup in each worker misses on the new key and re-loads, so all workers converge to the new reranker within Django's PipelineSettings cache TTL (5 minutes)."

This means after a default_reranker change, production workers can serve mixed results (some reranked, some not) for up to 5 minutes. For A/B testing correctness this is a meaningful window. Consider whether the invalidate_reranker_cache() call in the mutation should also signal workers via a Celery broadcast or Redis pub/sub to drop their local cache immediately.

7. Batched embedding with ThreadPoolExecutor is a real win

The microservice embedder changes (connection pooling, HTTPAdapter with urllib3.Retry, POST in allowed_methods) are correct and well-implemented. The double-checked locking on _get_session() follows the standard pattern correctly. The performance tuning doc is a welcome addition and accurately captures the non-obvious bulk_create → signal bypass lesson.

---

Code Quality

8. _paper_sample_tests key-by-id() is correct but worth a comment

keys = {id(test): _seed_value(test["snippets"][0]["file_path"]) for test in valid}
valid.sort(key=lambda test: keys[id(test)])

Using id() as a dict key is correct here because valid is a local list with no object reuse — objects can't be garbage-collected during the sort. However, this pattern surprises readers (CPython's id reuse guarantee doesn't extend across GC boundaries). A one-line comment explaining why this is safe would prevent future "that looks wrong" refactors.

9. DEFAULT_EMBEDDER is now env-overridable in test settings

# config/settings/test.py
DEFAULT_EMBEDDER = env(
    "DEFAULT_EMBEDDER",
    default="opencontractserver.pipeline.embedders.test_embedder.TestEmbedder",
)

This is necessary for benchmark runs but creates a hermeticity hole in CI: any DEFAULT_EMBEDDER set in the CI environment (e.g. from a production deploy step) will silently make unit tests use a real embedder. Add a comment and/or a CI check that warns when this env var is set outside benchmark contexts.

10. CHANGELOG for MAX_DOC_LENGTH_FOR_FUZZY reduction is missing from the diff

The changelog entry covers the reranker and chunker additions thoroughly, but the fuzzy-matcher hardening (reduced doc length cap, new query length cap, per-query timeout, n-gram anchor filter) isn't listed under Changed — only the text_alignment fuzzy hardening test mention is there. These are observable behavior changes.

---

Testing

11. Test coverage is strong

• TestUpstreamEquivalence (200 randomized trials per metric) is a strong guarantee on benchmark metric correctness — rare and valuable.
• test_orphan_structural_set_gc.py regression coverage for the GC fix is appropriate.
• TestFuzzyHardening for the text alignment hardening is good.
• The micro fixture (fixtures/benchmarks/legalbench_rag_micro/) is well-designed for hermetic CI.

12. test_benchmarks.py — check that use_eager_ingestion=False path is exercised

If the force_celery_eager() Celery 5 concern (item 1 above) is real, the tests may be using @override_settings(CELERY_TASK_ALWAYS_EAGER=True) which bypasses force_celery_eager() entirely, meaning the production code path isn't being exercised in tests.

---

Documentation

The benchmark results doc (docs/benchmarks/legalbench_rag_results.md) and the performance tuning doc (docs/deployment/performance_tuning.md) are both high-quality. The explicit retraction of the "+24.9 pts macro" framing in the PR description and the methodology fixes section is unusual and laudable — it makes the project's claims credible.

---

Summary

Area	Status
Correctness	⚠️ 3 items (Celery 5 compat, doc length regression, FK race)
Security	✅ Good — model_override trust documented; needs tracking issue
Performance	✅ Solid improvements, 5-min reranker cache lag documented
Code quality	✅ High — minor id() comment + changelog gap
Testing	✅ Strong coverage — Celery eager path worth verifying
Docs	✅ Excellent, including benchmark methodology retraction

The Celery 5 task_always_eager issue (item 1) and the _link_retrieval_citations race (item 3) are the most actionable before merge. The rest are lower priority or documentation gaps.

[claude]

Code Review — PR #1380: LegalBench-RAG Benchmark Harness + Parity Report

This is a large composite PR (~11k additions) covering the benchmark harness, paragraph chunker, reranker framework, and corpus-isolation fix. Overall the engineering quality is high — the areas below are targeted observations rather than a broad rejection.

---

Overview

The PR introduces:

• A pluggable reranker framework (cross-encoder, Cohere, microservice, no-op backends)
• A LegalBench-RAG benchmark harness (opencontractserver/benchmarks/) with metrics, runner, adapters, and a Django management command
• A paragraph chunker (ParagraphChunker) alongside sentence/sliding-window strategies
• OpenAI embedder input truncation fix (was 400-ing on >8192-token inputs)
• Agent failure-mode classifier (_classify_none_result) in data_extract_tasks.py
• Plumbing for similarity_top_k and model_override from CLI to agent tools
• 90 new/updated tests

---

Strengths

• Fault tolerance is excellent. safe_rerank() / safe_arerank() swallow all exceptions and fall back to first-stage results; strict_rerank() variants are available for benchmarks. No silent quality regressions for existing deployments.
• Backward-compatible by default. Reranking is gated by PipelineSettings.default_reranker (empty = disabled), so existing setups see zero behavior change.
• Secrets handling is solid. Cohere API key flows through encrypted_secrets + SettingType.SECRET — no plaintext leakage in logs.
• Async/sync discipline is maintained. Async code uses sync_to_async for ORM calls; the cross-encoder backend wraps sync via sync_to_async(thread_sensitive=False).
• Test coverage is comprehensive. 681-line reranker test suite covers base-class contract, each backend, HTTP error paths, safe/strict wrappers, and vector store integration.

---

Issues and Suggestions

1. model_override trust boundary needs a hard guard (MEDIUM risk)

File: opencontractserver/tasks/data_extract_tasks.py

The code has a good trust-boundary comment, but it's advisory only. The string is passed directly to the model registry with no validation today:

# Trust assumption: this string is passed straight to the agent
# factory and ultimately to the model registry. Current call
# sites (CLI run_benchmark command, internal benchmark runner)
# are operator-controlled. If this task is ever exposed to
# user-controlled input (webhook, public API), gate it behind
# an allowlist of approved model identifiers.

A comment is not a guard. A future contributor adding a webhook or a Celery beat task that accepts user input won't necessarily read this comment. Recommend adding a runtime allowlist check, even if it's currently populated from a settings key that happens to include all models:

ALLOWED_MODEL_OVERRIDES = getattr(settings, "BENCHMARK_ALLOWED_MODEL_OVERRIDES", None)
if model_override and ALLOWED_MODEL_OVERRIDES is not None:
    if model_override not in ALLOWED_MODEL_OVERRIDES:
        raise ValueError(f"model_override {model_override!r} not in allowlist")

If BENCHMARK_ALLOWED_MODEL_OVERRIDES = None (default), the check is skipped (operator-controlled only). This gives you a path to lock it down without a code change when the surface expands.

---

2. Reranker instance cache TTL/invalidation is implicit (LOW risk)

File: opencontractserver/pipeline/utils.py

The cache key is (class_path, PipelineSettings.modified). That correctly busts the cache when settings change, but PipelineSettings.modified is only updated on DB write — in-memory changes (e.g. test fixtures that patch settings) won't invalidate the process-local dict. The STRICT_RERANKER Django setting bypasses caching for tests, which covers the benchmark path, but it's worth documenting this assumption explicitly in a code comment or the get_reranker() docstring so future contributors don't accidentally hit stale instances in integration tests.

---

3. Microservice reranker: SSL/certificate verification undocumented

File: opencontractserver/pipeline/rerankers/microservice_reranker.py

The HTTP client doesn't appear to enforce TLS certificate verification by default. For internal Cloud Run services this is often fine (IAM auth + VPC), but operators pointing this at third-party endpoints will silently accept self-signed certs. A one-line mention in the class docstring (verify=True is the default for httpx/requests if you're using those) or a test using verify=False explicitly would make the behavior clear.

---

4. Benchmark corpus lifecycle is not cleaned up (LOW risk)

File: opencontractserver/benchmarks/loader.py

The materialize_corpus() function creates a real Corpus + Documents in the database. There's no teardown / delete_corpus() called at the end of run_benchmark(). This means repeated benchmark runs accumulate test corpora. Consider:

• Documenting that the operator should clean up via the admin after each run, or
• Adding a --cleanup flag to run_benchmark that deletes the materialized corpus after the run.

---

5. _classify_none_result stacktrace field repurposed for structured data (stylistic)

File: opencontractserver/tasks/data_extract_tasks.py

_classify_none_result writes structured data (failure_mode=agent_committed_none) into the stacktrace field of Datacell. This is clever for operator grep-ability but the field name implies unstructured exception text. If there's a metadata or extra JSON field available on Datacell, consider writing the failure mode classification there instead, keeping stacktrace for actual exception tracebacks. If stacktrace is the only writable field for this purpose, a comment explaining the convention would help.

---

6. Minor: hardcoded default cross-encoder model in settings migration

File: opencontractserver/migrations/0037_add_default_reranker_to_pipeline_settings.py

The migration seeds DEFAULT_RERANKER from an env var at migration time. This is a one-shot, non-reversible seed — if the env var isn't set at migration time, the default is empty (reranker disabled), which is the safe default. Worth noting explicitly in the migration comment that re-running migrate won't re-seed the value if it was already set, in case operators expect to change it by re-running migrations.

---

Test Coverage Notes

• The benchmarking runner is tested with a mocked structured-response agent — this is appropriate for unit tests. Consider adding at least one integration-level smoke test (even against the no-op reranker + in-memory SQLite corpus) to catch wiring regressions.
• test_text_alignment.py has 5 new tests in TestFuzzyHardening — great to see the per-query timeout and n-gram anchor pre-filter covered explicitly.
• TestFuzzyHardening covering edge cases (very short queries, no anchor n-grams) would be valuable additions in a follow-up.

---

CHANGELOG

The CHANGELOG entry is thorough and follows the project's Keep-a-Changelog format. One suggestion: the "Technical Details" section for the agent failure-mode fix could cross-reference the new _classify_none_result helper by file path/function name to make it easier to find during incident response.

---

Summary

Area	Assessment
Security	✅ Good — secrets encrypted, model_override documented, one runtime guard suggested
Performance	✅ Good — oversample cap, process cache, graceful degradation
Async/sync	✅ No violations found
Error handling	✅ Excellent — safe/strict wrapper pattern
Tests	✅ Comprehensive — 90 tests, edge cases covered
Code quality	✅ Clean — minor issues noted above
Backward compat	✅ Reranking opt-in, no behavior change by default

The core concern is the model_override trust boundary — a lightweight runtime guard would make this safe to widen the call surface later without a separate audit. The remaining items are low-risk quality suggestions. Happy to approve once the model_override guard is addressed or explicitly deferred with a tracking issue.

[claude]

Code Review: LegalBench-RAG Benchmark Harness + Parity Report

This is a substantial composite PR (11,175 additions). I've focused my review on correctness, security, and architecture rather than the benchmark numbers themselves.

---

Overview

The PR lands five interlocking pieces:

1. Benchmark harness (opencontractserver/benchmarks/) — adapter pattern, loader, runner, metrics, report
2. Pluggable text chunkers (text_chunkers.py) — sentence / paragraph / sliding-window strategies
3. Reranker framework (pipeline/base/reranker.py, pipeline/rerankers/) — pluggable post-retrieval stage
4. Grounding harness hardening (text_alignment.py) — per-query timeout, n-gram anchor pre-filter, tighter length caps
5. Agent quality fixes (pydantic_ai_agents.py, data_extract_tasks.py) — system-prompt tightening, failure-mode classifier, citation accumulation

---

What's done well

• _classify_none_result is a clean diagnostic addition. Distinguishing agent_committed_none (signal) from no_final_response / tool_loop_no_output (bugs) is exactly the right way to make operations actionable. The convention of piggybacking structured failure_mode= lines onto Datacell.stacktrace is pragmatic given the schema constraint.
• doc_lower_cached optimization in align_text_to_document is correctly implemented — the lazy-init pattern (if doc_lower_cached is None: doc_lower_cached = document_text.lower()) correctly amortizes the 200 KB allocation across all queries on the same document.
• _link_retrieval_citations has good defensive layering: positive-int guard, DB existence check, idempotent M2M add().
• Parallel embedding with ThreadPoolExecutor cleanly separates HTTP work (thread pool) from ORM writes (main thread), which is the right model for Django's per-thread connection management.
• Test coverage is thorough: 90 tests across metrics, adapters, loader, runner (with mocked LLM), alignment hardening, and chunkers.
• _get_function_tools cleanup — switching from the multi-attr attribute probe to the public agent.toolsets iteration is the right direction as pydantic-ai stabilises its API.
• No magic numbers — all new thresholds (FUZZY_PER_QUERY_TIMEOUT_SECONDS, FUZZY_ANCHOR_MIN_NGRAM_WORDS, MAX_QUERY_LENGTH_FOR_FUZZY, RERANK_OVERSAMPLE_FACTOR, etc.) land in the constants files.

---

Issues

Medium

1. _classify_none_result is coupled to undocumented pydantic-ai message schema

The function checks getattr(m, "kind", None) == "response" and getattr(p, "part_kind", None) against string literals ("tool-call", "thinking", etc.). These are not part of pydantic-ai's public API surface and have already shifted between 0.2.x and 1.x. If a future minor version renames them, response_msgs will silently be empty and every None result will be classified as empty_history, making the feature entirely invisible rather than failing loudly.

Suggestion: add a canary assertion in tests that pydantic-ai's ModelResponse instances do in fact have .kind == "response", so a version bump surfaces immediately. Alternatively, import the message types directly instead of string-matching:

from pydantic_ai.messages import ModelResponse, ToolCallPart, TextPart
response_msgs = [m for m in messages if isinstance(m, ModelResponse)]

2. ThreadPoolExecutor waits for in-flight sub-batches on transient error

In _batch_embed_text_annotations, when a transient requests.Timeout or EmbeddingServerError is raised inside the as_completed loop, it exits the with ThreadPoolExecutor(...) block. Python's __exit__ calls shutdown(wait=True, cancel_futures=False) (default), meaning all other in-flight sub-batches are allowed to run to completion before the exception propagates to Celery. With max_workers=4, this can delay Celery's retry by up to 3× the sub-batch round-trip time.

Suggestion: either document this as acceptable, or use explicit shutdown(wait=False, cancel_futures=True) on error by catching the exception before exiting the block and re-raising afterwards:

with ThreadPoolExecutor(max_workers=max_workers) as executor:
    ...
    transient_exc = None
    for future in as_completed(future_to_idx):
        try:
            ...
        except (requests.exceptions.Timeout, ...):
            transient_exc = sys.exc_info()
            break  # don't raise inside; let executor drain cleanly
    if transient_exc:
        raise transient_exc[1]

3. force_celery_eager() mutates a global setting

loader.py's force_celery_eager() context manager sets current_app.conf.task_always_eager = True globally. In a shared Celery worker process (or any multi-threaded test that overlaps run_benchmark calls), this silently affects all other tasks dispatched during the benchmark window. The context manager correctly restores the original value on exit, but concurrent use is unsafe.

This is marked as a known limitation ("Non-eager extraction is not yet supported"), but the docstring and the management command should warn explicitly that run_benchmark must not be called from a live production worker. Consider asserting task_always_eager is False before setting it, or raising RuntimeError if called in a non-test/non-CLI context.

Minor

4. Corpus agent system prompt says "most legal documents" (document-scoped wording)

In CorpusAgent._build_structured_system_prompt (line ~2503 in the new code), the hardened search protocol says "most legal documents need multiple targeted queries to surface a relevant span." The corpus agent searches across documents, so "most legal corpora" or "legal collections" would be more precise and avoids confusing operators debugging corpus-scoped extractions.

5. DEFAULT_EMBEDDER is env-overridable in test settings without a CI guard

config/settings/test.py now reads DEFAULT_EMBEDDER from the environment:

DEFAULT_EMBEDDER = env("DEFAULT_EMBEDDER", default="...TestEmbedder")

If DEFAULT_EMBEDDER is set in the CI environment for any other reason, the regular test suite will run against a real embedder and tests will make live network calls. The comment says "Standard CI never sets DEFAULT_EMBEDDER", which is true today, but this is a silent footgun. Consider guarding this with a BENCHMARK_MODE=1 flag that must be explicitly set before the env override takes effect.

6. model_override allowlist is opt-in with no default enforcement

The BENCHMARK_ALLOWED_MODEL_OVERRIDES guard is explicit and well-documented. The only suggestion is a small logging statement when model_override is accepted in the unset-allowlist (open) mode, so operators can see in logs when the unrestricted path fires:

if allowed is None:
    logger.info("BENCHMARK_ALLOWED_MODEL_OVERRIDES unset; accepting model_override=%r (operator-only path)", model_override)

7. PipelineSettings.default_reranker and cache coherence

The cache key for get_default_reranker_instance is (class_path, PipelineSettings.modified). A direct .save() on a PipelineSettings instance (e.g., via the admin or a migration) will bump modified. But migrations that backfill default_reranker using bulk_update bypass save() signals and won't update modified, so workers could stay on a stale (no-reranker) config until the cache TTL expires. Worth a note in the migration or get_default_reranker docstring.

---

Test coverage gaps

• No test exercises the ThreadPoolExecutor early-exit path (transient error with max_workers > 1).
• No test verifies _classify_none_result against an actual pydantic-ai message history (all existing tests mock the structure). Importing ModelResponse in at least one test would catch a version-schema drift.
• test_benchmarks.py's end-to-end runner test mocks get_structured_response_and_sources_from_document at the API layer, so _link_retrieval_citations is not exercised by the E2E path (it is tested separately in test_data_extract_helpers.py, which is good).

---

Changelog / docs

The CHANGELOG.md entry is present and comprehensive. The benchmark results in docs/benchmarks/legalbench_rag_results.md are traceable to committed docs/benchmarks/runs/ artifacts — appreciated.

---

Summary

This is a well-thought-out PR with solid test coverage and good documentation. The main concerns are: (1) _classify_none_result coupling to undocumented pydantic-ai internals that could silently break, (2) the ThreadPoolExecutor shutdown latency on transient errors, and (3) the global Celery eager-mode mutation needing an explicit safety guard. None are blockers, but (1) in particular is worth addressing before the next pydantic-ai version bump arrives unexpectedly.