M2 review: two regressions the green suite hid, and the reports

The post-implementation review of M2, plus the three corrections it took
to make the evidence true. Reports:

  planning/reports/M2-BASELINE-REPORT.md        868 lines, the measurements
  planning/reports/M2-IMPLEMENTATION-REPORT.md  758 lines, the reading of them

Verdict is PASS, accept with non-blocking debt, proceed to M3. Every M2
requirement is met and the ones that matter were tested by running the
build rather than reading it: a cloud endpoint written straight into
SQLite with sqlite3, behind the API's back, still refused at the wire;
trusted-LAN HTTPS against the real second machine with verification on;
captures showing zero packets outside loopback and the approved host.

Three defects, all found by running the shipped image.

The memory bank was dead. M2 removed Settings.api_key_plain with the API
key, and memorybank's two provider factories still read it. It failed
inside a fire-and-forget task, so no user error, no log anyone would
read, and no test — every memory test stubs those factories. All 604
tests passed with summaries and embeddings silently not happening.

The configurable model timeout never reached the turn engine. Stored,
validated, exposed in the API, rendered in the UI, and not passed to the
provider. M2's own exit criterion was half met: the constant had moved
but the setting did nothing.

And requirements.lock still pinned quickjs, psycopg and cryptography, so
the setup path DEVELOPMENT.md gives a new developer would have
reinstalled all three.

Both code defects now have the test that would have caught them: one
constructs every provider factory from a real Settings row, one drives
the turn endpoint, the chat endpoint and the summariser and asserts the
configured timeout arrives at each. That is the lesson worth keeping from
this milestone — after removing an attribute, build each consumer from a
real object; after adding a setting, prove it lands. Both failures were
in background or plumbing paths, which is exactly where a subtractive
change cannot see itself.

606 tests pass, up from 604. Lint, build and image are clean. Every
runtime result in the baseline report came from an image built after
these fixes; the reports say plainly that commit 8c65ae9 itself does not
contain them.

Also recorded: 88 test node IDs disappeared and every one is accounted
for — 64 whole files whose subject was removed, 5 replaced by a better
file, 15 individually retired with their features, and 4 renames. No
meaningful coverage was lost, and the eight files that used a JavaScript
counter as instrumentation kept their assertions by moving the counter to
the world-state engine.

Six planning recommendations are reported, not applied. Three are marked
before M3: the threat model still describes the inherited SSRF guard's
opposite rule, TECHNICAL-DESIGN §5.1 still marks two hardening items
open, and the endpoint policy is a load-bearing security decision that
exists only as a module docstring and deserves an ADR.

M3 is clear to start. Its chokepoints are untouched or simplified — the
rollback paths now carry one shared state instead of two — and the Phase
0B undo/redo spike still applies. No M3 work here: Undo still deletes and
there is still no Redo.

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

backend/app/memorybank.py

@@ -142,26 +142,22 @@ _running: set[int] = set()
 _tasks: set[asyncio.Task] = set()
 
 
-# Both factories below use the user's own key by construction. They read the
-# endpoint and key from `Settings` and never from `auth.DEMO_*`, so
-# summarization and embedding cannot spend the shared demo key. Their call sites
-# are also skipped when `using_demo` is true.
-#
-# Do not change these to accept a `ProviderConfig`. `summary_model` and
-# `embedding_model` are free-form user input and are not on the demo allowlist.
+# Both factories read the endpoint and the model names straight off `Settings`.
+# They used to also read an API key, which is gone: Ollama does not use one and
+# M2 removed cloud providers. `summary_model` and `embedding_model` fall back to
+# the narrator model when the user has not named a separate one.
 def summary_provider(settings: models.Settings) -> OpenAICompatibleProvider:
     return OpenAICompatibleProvider(
         settings.endpoint_url,
-        settings.api_key_plain,
         settings.summary_model or settings.model,
         settings.api_mode,
-        settings.reasoning_max_tokens,
+        settings.model_timeout_seconds,
     )
 
 
 def embedding_provider(settings: models.Settings) -> OpenAICompatibleProvider:
     return OpenAICompatibleProvider(
-        settings.endpoint_url, settings.api_key_plain, settings.embedding_model
+        settings.endpoint_url, settings.embedding_model
     )
 
 

backend/app/models.py

@@ -520,10 +520,10 @@ class Settings(Base):
     # 800 leaves room for a full scene; 400 tended to truncate mid-paragraph
     # and left reasoning models with nothing after their thinking.
     max_output_tokens: Mapped[int] = mapped_column(Integer, default=800)
-    # Separate thinking budget for reasoning models (OpenRouter-style
-    # `reasoning: {max_tokens}`); 0 = param not sent, -1 = reasoning explicitly
-    # off (`reasoning: {effort: none}`). Added on top of
-    # max_output_tokens so story output keeps its full budget.
+    # Was an OpenRouter-style thinking budget. Ollama's OpenAI-compatible
+    # endpoint ignores the field, so M2 stopped sending it and removed it from
+    # the Settings API and UI. The column stays so existing databases open
+    # unchanged and is never read.
     reasoning_max_tokens: Mapped[int] = mapped_column(Integer, default=0)
     context_token_budget: Mapped[int] = mapped_column(Integer, default=16384)
     # How long to wait for the model, in seconds, before giving up on a turn.

backend/app/routers/adventures/turns.py

@@ -170,7 +170,8 @@ async def _generate_turn(
     parts = PromptParts(system=system_text, story=story_text)
 
     provider = OpenAICompatibleProvider(
-        settings.endpoint_url, settings.model, settings.api_mode
+        settings.endpoint_url, settings.model, settings.api_mode,
+        settings.model_timeout_seconds,
     )
     chunks: list[str] = []
     reasoning_chunks: list[str] = []

backend/app/routers/chat.py

@@ -59,7 +59,8 @@ async def run_chat(
     a `done` event, so the frontend reuses the same code.
     """
     provider = OpenAICompatibleProvider(
-        settings.endpoint_url, model, settings.api_mode
+        settings.endpoint_url, model, settings.api_mode,
+        settings.model_timeout_seconds,
     )
     messages = [m.model_dump() for m in payload.messages]
     chunks: list[str] = []

backend/requirements.lock

@@ -19,10 +19,8 @@ annotated-doc==0.0.5
 annotated-types==0.8.0
 anyio==4.14.2
 certifi==2026.7.22
-cffi==2.1.1
 charset-normalizer==3.5.1
 click==8.5.0
-cryptography==50.0.1
 fastapi==0.141.1
 greenlet==3.5.5
 h11==0.16.0
@@ -33,16 +31,12 @@ idna==3.19
 iniconfig==2.3.0
 packaging==26.3
 pluggy==1.6.0
-psycopg==3.3.5
-psycopg-binary==3.3.5
-pycparser==3.0
 pydantic==2.13.5
 pydantic_core==2.46.5
 Pygments==2.21.0
 pytest==9.1.1
 python-dotenv==1.2.3
 PyYAML==6.0.3
-quickjs==1.19.4
 regex==2026.9.3
 requests==2.34.2
 SQLAlchemy==2.0.52

backend/tests/test_local_only_surface.py

@@ -179,3 +179,91 @@ def test_compose_publishes_to_loopback_only():
     assert published, "no published ports found — has the file moved?"
     for mapping in published:
         assert mapping.startswith("127.0.0.1:"), mapping
+
+
+# --- every provider factory must actually build ---------------------------
+
+def test_every_provider_factory_builds_from_a_real_settings_row(client):
+    """M2 shipped with a defect this test would have caught.
+
+    `Settings.api_key_plain` was removed with the API key, but `memorybank`'s
+    two provider factories still read it. Nothing failed at import, and no test
+    noticed, because every memory test stubs those factories out — so the break
+    only appeared at runtime, in a background task, as a swallowed
+    `AttributeError` that silently stopped summaries and embeddings.
+
+    Constructing each factory from a real row is the cheapest thing that would
+    have caught it, and it catches the same shape of mistake next time a
+    Settings column moves.
+    """
+    from app import memorybank
+
+    db = SessionLocal()
+    try:
+        settings = db.query(models.Settings).first()
+        settings.embedding_model = "nomic-embed-text:latest"
+        settings.summary_model = ""
+        db.commit()
+
+        summary = memorybank.summary_provider(settings)
+        assert summary.model == settings.model  # falls back to the narrator
+        assert summary.base_url == settings.endpoint_url.rstrip("/")
+
+        embed = memorybank.embedding_provider(settings)
+        assert embed.model == "nomic-embed-text:latest"
+
+        turn = OpenAICompatibleProvider(
+            settings.endpoint_url, settings.model, settings.api_mode,
+            settings.model_timeout_seconds,
+        )
+        assert turn.read_timeout == settings.model_timeout_seconds
+    finally:
+        db.close()
+
+
+def test_the_configured_timeout_reaches_every_generating_client(client, monkeypatch):
+    """The second defect this review caught. The setting was stored, validated
+    and exposed, and then not passed to the provider — so the turn engine kept
+    using the module default and "configurable" was a claim rather than a fact.
+
+    Each generating path is driven for real and the constructed provider is
+    recorded. Embeddings are deliberately excluded: they are short, never
+    cold-load a large model, and keep their own shorter constant.
+    """
+    from app import memorybank
+    from app.routers import chat as chat_router
+    from app.routers.adventures import turns as turns_router
+
+    seen = []
+
+    class Recorder:
+        last_usage = None
+
+        def __init__(self, endpoint_url, model, api_mode="chat", read_timeout=None):
+            seen.append(read_timeout)
+
+        async def generate(self, *a, **k):
+            yield ("text", "narration")
+
+        async def chat(self, *a, **k):
+            yield ("text", "reply")
+
+    monkeypatch.setattr(turns_router, "OpenAICompatibleProvider", Recorder)
+    monkeypatch.setattr(chat_router, "OpenAICompatibleProvider", Recorder)
+    monkeypatch.setattr(memorybank, "OpenAICompatibleProvider", Recorder)
+
+    assert client.put("/api/settings", json={"model_timeout_seconds": 777}).status_code == 200
+
+    adv = client.post("/api/adventures", json={"title": "T"}).json()["id"]
+    client.post(f"/api/adventures/{adv}/actions", json={"type": "story", "text": "hello"})
+    assert seen and seen[-1] == 777, f"turn engine used {seen[-1]!r}"
+
+    client.post("/api/chat/stream", json={"messages": [{"role": "user", "content": "hi"}]})
+    assert seen[-1] == 777, f"chat used {seen[-1]!r}"
+
+    db = SessionLocal()
+    try:
+        memorybank.summary_provider(db.query(models.Settings).first())
+    finally:
+        db.close()
+    assert seen[-1] == 777, f"summarizer used {seen[-1]!r}"