Rifqi Hafizuddin Claude Opus 4.8 commited on
Commit
236d991
·
1 Parent(s): e58cc23

[KM-652] fix(orchestrator): un-brick sessions + enforce data-source binding

Browse files

Three critical fixes found tracing the request lifecycle:

- T-A: rooms from /room/create have no analysis_states row, so with the gate
on the turn redirect-looped (gate fail-closed + a silent no-op on the
problem_validated write) and gate-off it spammed duplicate report versions
(report_id write-back no-op'd). AnalysisStateStore.update now warns instead
of silently no-op'ing; new idempotent ensure() (INSERT ON CONFLICT DO
NOTHING) get-or-creates the row; ChatHandler ensures it once per turn and the
gate reuses that state (also drops the gate/help double-read).

- T-B: #10 binding only scoped the Planner's catalog; the data-access tools
re-read the full catalog themselves, so binding was a hint, not a boundary.
Scope at the reader (_ScopedCatalogReader) so the Planner AND the tools share
one scoped view. Fast path was already correct.

- T-C: _scope_to_binding failed CLOSED on a disjoint binding (empty catalog ->
hard fail). The scoped reader fails open (scoped or whole catalog), matching
the report generator.

Caveat: lazily-created legacy rows carry no bindings, so those analyses run
unscoped (whole catalog) until sessions converge on /analysis/create.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/agents/chat_handler.py CHANGED
@@ -186,24 +186,21 @@ class ChatHandler:
186
  self._binding_store = AnalysisDataSourceStore()
187
  return self._binding_store
188
 
189
- async def _scope_to_binding(self, catalog: Any, analysis_id: str | None) -> Any:
190
- """#10: restrict the catalog to the analysis's bound sources.
191
 
192
  Fail-open: no analysis_id, no binding rows (legacy room / FE not sending
193
- ids), or a read error → return the catalog unscoped (whole catalog).
 
 
194
  """
195
  if not analysis_id:
196
- return catalog
197
  try:
198
- bound = set(await self._get_binding_store().get(analysis_id))
199
  except Exception as e: # noqa: BLE001 — never block the query on this
200
  logger.warning("binding read failed — unscoped", analysis_id=analysis_id, error=str(e))
201
- return catalog
202
- if not bound:
203
- return catalog
204
- return catalog.model_copy(
205
- update={"sources": [s for s in catalog.sources if s.source_id in bound]}
206
- )
207
 
208
  async def _load_analysis_state(self, analysis_id: str | None) -> AnalysisState:
209
  """Load Analysis State for the Help skill; fail closed to a not-validated stub.
@@ -246,20 +243,30 @@ class ChatHandler:
246
  return
247
 
248
  intent = decision.intent
 
 
 
 
 
 
 
 
 
 
 
 
 
249
  # ---- 1b. Gate (deterministic, post-router) -------------------
250
  # Redirect structured_flow -> problem_statement until the analysis is
251
- # validated. Fails closed (not-validated) on a missing row / read error.
252
  if self._enable_gate and analysis_id:
253
  from .gate import gate, stub_analysis_state
254
 
255
- try:
256
- state = await self._get_state_store().get(analysis_id)
257
- except Exception as e:
258
- logger.warning("gate state read failed — not-validated", error=str(e))
259
- state = None
260
  intent = gate(
261
  intent,
262
- state if state is not None else stub_analysis_state(problem_validated=False),
 
 
263
  )
264
 
265
  # Back-compat: the frontend still reads `source_hint` off the intent event.
@@ -284,11 +291,15 @@ class ChatHandler:
284
  from ..catalog.reader import MemoizingCatalogReader
285
 
286
  req_reader = MemoizingCatalogReader(self._get_catalog_reader())
287
- catalog = await req_reader.read(user_id, "structured")
288
- catalog = await self._scope_to_binding(catalog, analysis_id)
 
 
 
 
289
  if self._enable_slow_path:
290
  async for event in self._run_slow_path(
291
- user_id, rewritten, catalog, tracer, req_reader, analysis_id
292
  ):
293
  yield event
294
  return
@@ -344,7 +355,7 @@ class ChatHandler:
344
  return
345
  elif intent == "help":
346
  try:
347
- state = await self._load_analysis_state(analysis_id)
348
  except Exception as e:
349
  logger.error("help route failed", user_id=user_id, error=str(e))
350
  yield {"event": "error", "data": f"Help failed: {e}"}
@@ -561,6 +572,31 @@ class ChatHandler:
561
  yield {"event": "done", "data": ""}
562
 
563
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
564
  def _intent_to_source_hint(intent: str) -> str:
565
  """Map a router intent to the legacy `source_hint` value (frontend back-compat)."""
566
  if intent == "structured_flow":
 
186
  self._binding_store = AnalysisDataSourceStore()
187
  return self._binding_store
188
 
189
+ async def _bound_source_ids(self, analysis_id: str | None) -> set[str]:
190
+ """#10: the catalog source_ids this analysis is bound to (empty = unscoped).
191
 
192
  Fail-open: no analysis_id, no binding rows (legacy room / FE not sending
193
+ ids), or a read error → empty set, which the caller treats as "whole
194
+ catalog". Used to build a `_ScopedCatalogReader` so the Planner AND the
195
+ data-access tools (which re-read the catalog themselves) see the same scope.
196
  """
197
  if not analysis_id:
198
+ return set()
199
  try:
200
+ return set(await self._get_binding_store().get(analysis_id))
201
  except Exception as e: # noqa: BLE001 — never block the query on this
202
  logger.warning("binding read failed — unscoped", analysis_id=analysis_id, error=str(e))
203
+ return set()
 
 
 
 
 
204
 
205
  async def _load_analysis_state(self, analysis_id: str | None) -> AnalysisState:
206
  """Load Analysis State for the Help skill; fail closed to a not-validated stub.
 
243
  return
244
 
245
  intent = decision.intent
246
+ # ---- 1a. Ensure session state row (T-A) ----------------------
247
+ # Rooms created via /room/create have no `analysis_states` row. Without one
248
+ # the gate redirect-loops and problem_statement / report_id writes silently
249
+ # no-op. Lazily get-or-create it (idempotent) so any session is gate-ready.
250
+ analysis_state: AnalysisState | None = None
251
+ if analysis_id:
252
+ try:
253
+ analysis_state = await self._get_state_store().ensure(analysis_id, user_id)
254
+ except Exception as e:
255
+ logger.warning(
256
+ "analysis state ensure failed", analysis_id=analysis_id, error=str(e)
257
+ )
258
+
259
  # ---- 1b. Gate (deterministic, post-router) -------------------
260
  # Redirect structured_flow -> problem_statement until the analysis is
261
+ # validated. Fails closed (not-validated) when the state row is unavailable.
262
  if self._enable_gate and analysis_id:
263
  from .gate import gate, stub_analysis_state
264
 
 
 
 
 
 
265
  intent = gate(
266
  intent,
267
+ analysis_state
268
+ if analysis_state is not None
269
+ else stub_analysis_state(problem_validated=False),
270
  )
271
 
272
  # Back-compat: the frontend still reads `source_hint` off the intent event.
 
291
  from ..catalog.reader import MemoizingCatalogReader
292
 
293
  req_reader = MemoizingCatalogReader(self._get_catalog_reader())
294
+ # #10: scope every catalog read — the Planner's AND the data-access
295
+ # tools' own re-reads — to the analysis's bound sources, so binding
296
+ # is a boundary, not just a planner hint (T-B). Fail-open (T-C).
297
+ bound = await self._bound_source_ids(analysis_id)
298
+ reader = _ScopedCatalogReader(req_reader, bound) if bound else req_reader
299
+ catalog = await reader.read(user_id, "structured")
300
  if self._enable_slow_path:
301
  async for event in self._run_slow_path(
302
+ user_id, rewritten, catalog, tracer, reader, analysis_id
303
  ):
304
  yield event
305
  return
 
355
  return
356
  elif intent == "help":
357
  try:
358
+ state = analysis_state or await self._load_analysis_state(analysis_id)
359
  except Exception as e:
360
  logger.error("help route failed", user_id=user_id, error=str(e))
361
  yield {"event": "error", "data": f"Help failed: {e}"}
 
572
  yield {"event": "done", "data": ""}
573
 
574
 
575
+ class _ScopedCatalogReader:
576
+ """Wraps a CatalogReader, restricting `structured` reads to an analysis's bound
577
+ sources (#10).
578
+
579
+ Scoping lives here — not at a single call site — so the Planner AND the
580
+ data-access tools (which re-read the catalog themselves) see the same scoped
581
+ view; otherwise binding is only a hint to the Planner while the executor runs
582
+ against the full catalog. Fail-open: an empty or fully-disjoint binding yields
583
+ the whole catalog, so a stale / cross-source binding degrades instead of
584
+ emptying the catalog. Only `structured` reads are scoped (all #10 binds today);
585
+ `unstructured` / retrieval reads pass through.
586
+ """
587
+
588
+ def __init__(self, inner: Any, bound: set[str]) -> None:
589
+ self._inner = inner
590
+ self._bound = bound
591
+
592
+ async def read(self, user_id: str, source_hint: str) -> Any:
593
+ catalog = await self._inner.read(user_id, source_hint)
594
+ if not self._bound or source_hint != "structured":
595
+ return catalog
596
+ scoped = [s for s in catalog.sources if s.source_id in self._bound]
597
+ return catalog.model_copy(update={"sources": scoped or catalog.sources})
598
+
599
+
600
  def _intent_to_source_hint(intent: str) -> str:
601
  """Map a router intent to the legacy `source_hint` value (frontend back-compat)."""
602
  if intent == "structured_flow":
src/agents/state_store.py CHANGED
@@ -10,6 +10,8 @@ Mirrors `PostgresAnalysisStore`: each call opens its own `AsyncSession`.
10
 
11
  from __future__ import annotations
12
 
 
 
13
  from src.agents.gate import AnalysisState
14
  from src.db.postgres.connection import AsyncSessionLocal
15
  from src.db.postgres.models import AnalysisStateRow
@@ -40,6 +42,38 @@ class AnalysisStateStore:
40
  row = await session.get(AnalysisStateRow, analysis_id)
41
  return _row_to_state(row) if row is not None else None
42
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
43
  async def create(
44
  self,
45
  *,
@@ -78,6 +112,10 @@ class AnalysisStateStore:
78
  async with AsyncSessionLocal() as session:
79
  row = await session.get(AnalysisStateRow, analysis_id)
80
  if row is None:
 
 
 
 
81
  return None
82
  if problem_statement is not None:
83
  row.problem_statement = problem_statement
 
10
 
11
  from __future__ import annotations
12
 
13
+ from sqlalchemy.dialects.postgresql import insert
14
+
15
  from src.agents.gate import AnalysisState
16
  from src.db.postgres.connection import AsyncSessionLocal
17
  from src.db.postgres.models import AnalysisStateRow
 
42
  row = await session.get(AnalysisStateRow, analysis_id)
43
  return _row_to_state(row) if row is not None else None
44
 
45
+ async def ensure(
46
+ self,
47
+ analysis_id: str,
48
+ owner_id: str,
49
+ analysis_title: str = "New analysis",
50
+ ) -> AnalysisState:
51
+ """Get-or-create the state row for a session (idempotent, race-safe).
52
+
53
+ Sessions born from `/room/create` have no `analysis_states` row; without
54
+ one the gate redirect-loops and `problem_statement` / `report_id` writes
55
+ silently no-op. Called per turn (analysis_id == room_id) so any session is
56
+ gate-ready. `INSERT ... ON CONFLICT DO NOTHING` makes concurrent first
57
+ turns safe; the row is then read back. Legacy rows created this way carry
58
+ no source bindings — binding scoping fail-opens to the whole catalog.
59
+ """
60
+ async with AsyncSessionLocal() as session:
61
+ stmt = (
62
+ insert(AnalysisStateRow)
63
+ .values(
64
+ id=analysis_id,
65
+ owner_id=owner_id,
66
+ analysis_title=analysis_title,
67
+ problem_statement="",
68
+ problem_validated=False,
69
+ )
70
+ .on_conflict_do_nothing(index_elements=[AnalysisStateRow.id])
71
+ )
72
+ await session.execute(stmt)
73
+ await session.commit()
74
+ row = await session.get(AnalysisStateRow, analysis_id)
75
+ return _row_to_state(row)
76
+
77
  async def create(
78
  self,
79
  *,
 
112
  async with AsyncSessionLocal() as session:
113
  row = await session.get(AnalysisStateRow, analysis_id)
114
  if row is None:
115
+ logger.warning(
116
+ "analysis_states row missing — update skipped",
117
+ analysis_id=analysis_id,
118
+ )
119
  return None
120
  if problem_statement is not None:
121
  row.problem_statement = problem_statement