From 3359a3771b2c8a0d53ffca2463cbae2662827618 Mon Sep 17 00:00:00 2001
From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com>
Date: Wed, 29 Jul 2026 22:59:53 -0400
Subject: [PATCH] fix(scopes): skip semantic retrieval when the embedding
 precompute failed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses three open review comments.

1. Major — the representation read could embed inside its DB session. The
   route's precompute is suppressed, and both
   `RepresentationManager.get_working_representation` and
   `crud.query_documents` fall back to embedding when a query arrives without
   one, so a failed precompute meant an external call inside the read session
   this branch opens for the scope re-check — the connection-holding rule the
   route's own comment claimed to satisfy. The innermost fallback also only
   catches ValueError, so a provider outage surfaced as a 500. The semantic
   query is now passed only when an embedding exists, degrading to
   derived+recent retrieval. (`crud.query_documents` embedding inside a caller's
   session predates this branch and is left alone.)

2. Minor — `test_resolved_scope_peer_rejected_at_membership_upsert` described a
   race it does not perform. It creates an already-flagged scope and calls crud
   directly; the unflagged → flagged transition is not simulated. Docstring now
   says what the test actually pins.

3. Minor — `test_empty_replacement_preserves_scope_membership` asserted only
   half its docstring. It passed if the empty PUT left every ordinary
   membership intact; now asserts the ordinary peer's left_at is set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 src/routers/peers.py        | 12 +++++++++++-
 tests/routes/test_scopes.py | 21 ++++++++++++++++-----
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/routers/peers.py b/src/routers/peers.py
index bbcfa214..6845dbe0 100644
--- a/src/routers/peers.py
+++ b/src/routers/peers.py
@@ -440,7 +440,17 @@ async def get_representation(
                 session_allowlist=[options.session_id]
                 if options.session_id is not None
                 else session_allowlist,
-                include_semantic_query=options.search_query,
+                # Only ask for the semantic branch when we actually have an
+                # embedding. The precompute above is suppressed, and both
+                # `RepresentationManager.get_working_representation` and
+                # `crud.query_documents` fall back to embedding internally when a
+                # query arrives without one — which would run an external call
+                # inside this session, and the innermost fallback is unsuppressed
+                # (a provider outage would surface as a 500). Degrading to
+                # derived+recent retrieval keeps the session DB-only.
+                include_semantic_query=options.search_query
+                if embedding is not None
+                else None,
                 embedding=embedding,
                 semantic_search_top_k=options.search_top_k,
                 semantic_search_max_distance=options.search_max_distance,
diff --git a/tests/routes/test_scopes.py b/tests/routes/test_scopes.py
index 59149d55..3b187389 100644
--- a/tests/routes/test_scopes.py
+++ b/tests/routes/test_scopes.py
@@ -945,9 +945,10 @@ async def test_resolved_scope_peer_rejected_at_membership_upsert(
 ):
     """The last-line guard runs on resolved rows, closing the check-then-upsert race.
 
-    Simulates the race by flagging the peer *after* the route-level name check
-    would have passed: the peer exists and is unflagged when named, and is a real
-    scope by the time membership is upserted.
+    Calls crud directly, bypassing the route-level name check, so the only thing
+    standing between the caller and a scope membership is the guard on the
+    resolved peer row — the guard the racing caller would hit. The unflagged →
+    flagged transition itself is not simulated here.
     """
     test_workspace, _ = sample_data
     scope_name = str(generate_nanoid())
@@ -1116,8 +1117,10 @@ def test_generic_replacement_preserves_scope_membership(
     assert response.json()["session_ids"] == [session_name]
 
 
-def test_empty_replacement_preserves_scope_membership(
-    client: TestClient, sample_data: tuple[Workspace, Peer]
+async def test_empty_replacement_preserves_scope_membership(
+    client: TestClient,
+    db_session: AsyncSession,
+    sample_data: tuple[Workspace, Peer],
 ):
     """An empty replacement map clears ordinary peers but not scopes."""
     test_workspace, test_peer = sample_data
@@ -1146,6 +1149,14 @@ def test_empty_replacement_preserves_scope_membership(
     )
     assert response.json()["session_ids"] == [session_name]
 
+    # The other half of the docstring: without this the test passes even if the
+    # empty replacement became a no-op for ordinary peers too.
+    session_peer = await _get_session_peer(
+        db_session, test_workspace.name, session_name, test_peer.name
+    )
+    assert session_peer is not None
+    assert session_peer.left_at is not None
+
 
 async def test_replacement_still_removes_unflagged_squatter(
     client: TestClient,