Stop an empty env var from shadowing .env, and refuse a total OCR blackout

Two faults found by running the pipeline inside the Docker image for the first
time, both of which let a 25-page run finish "successfully" having stored nothing.

The image declares ENV DASHSCOPE_API_KEY= so the variable is visible to
docker inspect and overridable at docker run. That leaves it set-but-empty in
every container, and load_dotenv used os.environ.setdefault, so the empty string
won and the mounted .env was ignored. An empty value now counts as unset; a real
value still wins, because overriding on the command line is the point.

The consequence was worse than a missing key. Every page failed, the per-page
fallback kept the font-mangled embedded text -- exactly the text OCR is there to
replace -- and the run went on to spend a full detection pass on it. Tolerating
one unreadable page is right; tolerating every page is a configuration error
wearing a success message, so Document now raises OcrUnavailable when OCR was
attempted and not one page came back, quoting the first real failure. A page
range that needed no OCR at all is still not an error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ci5T3gWDhhmqDec3e1XtYZ

leanbench/config.py

@@ -17,7 +17,15 @@ DEFAULT_API_KEY_ENV = "DASHSCOPE_API_KEY"
 
 
 def load_dotenv(path: Path | str = ".env") -> None:
-    """Load ``KEY=VALUE`` lines into os.environ without overwriting what is set."""
+    """Load ``KEY=VALUE`` lines into os.environ without overwriting a real value.
+
+    A variable that is set but *empty* counts as unset. This is not pedantry: the
+    Docker image declares ``ENV DASHSCOPE_API_KEY=`` so the variable is documented
+    and overridable at ``docker run``, which left it present-and-empty in every
+    container. With plain ``setdefault`` that empty string shadowed the .env file,
+    and the only symptom was every OCR page failing with "No API key" -- a whole
+    25-page run spent on text the pipeline had already judged unreadable.
+    """
     path = Path(path)
     if not path.is_file():
         return
@@ -26,7 +34,10 @@ def load_dotenv(path: Path | str = ".env") -> None:
         if not line or line.startswith("#") or "=" not in line:
             continue
         key, _, value = line.partition("=")
-        os.environ.setdefault(key.strip(), value.strip().strip('"').strip("'"))
+        key = key.strip()
+        if os.environ.get(key):  # a non-empty value wins; empty or absent does not
+            continue
+        os.environ[key] = value.strip().strip('"').strip("'")
 
 
 def _env(name: str, default: str) -> str:

leanbench/document.py

@@ -46,6 +46,10 @@ def normalise_page_text(text: str) -> str:
     return text
 
 
+class OcrUnavailable(RuntimeError):
+    """Raised when OCR was asked for and could not read a single page."""
+
+
 @dataclass(frozen=True)
 class Page:
     number: int  # 1-based, physical page order in the file
@@ -178,6 +182,19 @@ class Document:
             replaced.append(page.number)
             say(f"  page {page.number}: OCR (embedded text scored {quality})")
 
+        # One page the model chokes on is a bad page; every page failing is a broken
+        # configuration -- a missing key, an unavailable model, no network. Falling
+        # back page by page is right in the first case and disastrous in the second,
+        # because the fallback text is exactly the mangled text OCR exists to replace:
+        # the run completes, costs a full detection pass, and yields nothing. It is
+        # cheaper to stop here than to explain the empty database afterwards.
+        if skipped and not replaced:
+            reason = next(iter(ocr.failures.values()), "no reason recorded")
+            raise OcrUnavailable(
+                f"OCR produced nothing for all {len(skipped)} page(s) via "
+                f"{ocr.backend.name}; first failure: {reason}"
+            )
+
         if replaced:
             self.extractor = f"{self.extractor}+{ocr.backend.name}@{ocr.dpi}dpi"
         self.meta["ocr"] = {

tests/test_config.py

@@ -0,0 +1,69 @@
+"""The .env loader, which is only interesting where it disagrees with itself.
+
+These tests exist because of one real incident: the Docker image declares
+``ENV DASHSCOPE_API_KEY=`` so the variable shows up in ``docker inspect`` and can
+be overridden at ``docker run``. That left it set-but-empty inside every container,
+where it shadowed the mounted .env file. Every OCR page failed with "No API key",
+the pipeline fell back page by page as designed, and a 25-page run finished
+successfully having stored nothing.
+"""
+from __future__ import annotations
+
+from pathlib import Path
+
+from leanbench.config import load_dotenv
+
+
+def write_env(directory: Path, body: str) -> Path:
+    path = directory / ".env"
+    path.write_text(body, encoding="utf-8")
+    return path
+
+
+def test_an_empty_variable_does_not_shadow_the_file(tmp_path: Path, monkeypatch) -> None:
+    """The incident, in one assertion."""
+    monkeypatch.setenv("LEANBENCH_TEST_KEY", "")
+    load_dotenv(write_env(tmp_path, "LEANBENCH_TEST_KEY=from-file\n"))
+    import os
+
+    assert os.environ["LEANBENCH_TEST_KEY"] == "from-file"
+
+
+def test_a_real_value_still_wins(tmp_path: Path, monkeypatch) -> None:
+    """Overriding at the command line must keep working -- that is the whole point."""
+    monkeypatch.setenv("LEANBENCH_TEST_KEY", "from-environment")
+    load_dotenv(write_env(tmp_path, "LEANBENCH_TEST_KEY=from-file\n"))
+    import os
+
+    assert os.environ["LEANBENCH_TEST_KEY"] == "from-environment"
+
+
+def test_an_unset_variable_is_taken_from_the_file(tmp_path: Path, monkeypatch) -> None:
+    monkeypatch.delenv("LEANBENCH_TEST_KEY", raising=False)
+    load_dotenv(write_env(tmp_path, "LEANBENCH_TEST_KEY=from-file\n"))
+    import os
+
+    assert os.environ["LEANBENCH_TEST_KEY"] == "from-file"
+
+
+def test_comments_blanks_and_quotes(tmp_path: Path, monkeypatch) -> None:
+    monkeypatch.delenv("LEANBENCH_A", raising=False)
+    monkeypatch.delenv("LEANBENCH_B", raising=False)
+    monkeypatch.delenv("LEANBENCH_C", raising=False)
+    load_dotenv(write_env(tmp_path, """
+# a comment
+LEANBENCH_A = "quoted value"
+
+LEANBENCH_B='single'
+not a pair
+LEANBENCH_C=has=equals=inside
+"""))
+    import os
+
+    assert os.environ["LEANBENCH_A"] == "quoted value"
+    assert os.environ["LEANBENCH_B"] == "single"
+    assert os.environ["LEANBENCH_C"] == "has=equals=inside"
+
+
+def test_a_missing_file_is_not_an_error(tmp_path: Path) -> None:
+    load_dotenv(tmp_path / "nope.env")

tests/test_ocr.py

@@ -4,7 +4,7 @@ from pathlib import Path
 
 import pytest
 
-from leanbench.document import Document
+from leanbench.document import Document, OcrUnavailable
 from leanbench.ocr import (
     OcrCache,
     OcrReader,
@@ -64,6 +64,22 @@ class ExplodingBackend:
         raise RuntimeError("model refused")
 
 
+class FlakyBackend:
+    """Fails on chosen pages only -- the ordinary case of one unreadable page."""
+
+    name = "flaky:v1"
+
+    def __init__(self, fail_on: set[int]) -> None:
+        self.fail_on = fail_on
+        self.seen = 0
+
+    def transcribe(self, image: bytes) -> str:
+        self.seen += 1
+        if self.seen in self.fail_on:
+            raise RuntimeError(f"page {self.seen} refused")
+        return CLEAN
+
+
 # -- the quality heuristic -------------------------------------------------
 
 
@@ -225,11 +241,37 @@ def test_text_mode_never_touches_the_model(mixed_book: Path) -> None:
 
 
 def test_a_failed_page_keeps_its_embedded_text(mixed_book: Path, monkeypatch) -> None:
+    """One bad page falls back; the rest of the book is unaffected."""
+    monkeypatch.setattr("leanbench.ocr.render_page", lambda *a, **k: b"png")
+    reader = OcrReader(backend=FlakyBackend(fail_on={2}), cache=None)
+    document = Document.load(mixed_book, mode="ocr", ocr=reader)
+    assert document.meta["ocr"]["pages_failed"] == [2]
+    assert document.meta["ocr"]["pages_replaced"] == [1]
+    assert "Š" in document.page(2).text, "we fall back, we do not lose the page"
+
+
+def test_total_ocr_failure_is_loud(mixed_book: Path, monkeypatch) -> None:
+    """Every page failing is a broken configuration, not a run to complete.
+
+    Silently falling back here is the expensive failure: the fallback text is the
+    mangled text OCR was called in to replace, so the run costs a full detection
+    pass and stores nothing.
+    """
     monkeypatch.setattr("leanbench.ocr.render_page", lambda *a, **k: b"png")
     reader = OcrReader(backend=ExplodingBackend(), cache=None)
-    document = Document.load(mixed_book, mode="ocr", ocr=reader)
-    assert document.meta["ocr"]["pages_failed"] == [1, 2]
-    assert "Š" in document.page(2).text, "we fall back, we do not lose the page"
+    with pytest.raises(OcrUnavailable) as caught:
+        Document.load(mixed_book, mode="ocr", ocr=reader)
+    assert "model refused" in str(caught.value), "the real cause must reach the operator"
+    assert "boom:v1" in str(caught.value)
+
+
+def test_auto_mode_with_nothing_to_do_does_not_raise(tmp_path: Path) -> None:
+    """No page needed OCR, so nothing failed -- that is success, not an outage."""
+    path = tmp_path / "clean.txt"
+    path.write_text(PROSE, encoding="utf-8")
+    document = Document.load(path, mode="auto", ocr=OcrReader(backend=ExplodingBackend()))
+    assert document.meta["ocr"]["pages_replaced"] == []
+    assert document.meta["ocr"]["pages_failed"] == []
 
 
 def test_offsets_stay_usable_after_ocr(mixed_book: Path, monkeypatch) -> None: