sofhiaazzhr Claude Opus 4.8 commited on
Commit
db2a10f
·
1 Parent(s): f873f92

[NOTICKET] check: scope to analysis catalog + list database tables

Browse files

Scope the `check` skill's structured reads to the analysis catalog instead of
the user catalog, and expand databases to their table names in the inventory.

Why: at user scope a database shows its auto-generated `postgres_<hash>` name
and every source the user ever registered — not this room's. The analysis-scope
catalog row carries the real name (e.g. "xl test") and only the bound sources.

- store: `get_by_analysis()` reads the `scope_type='analysis'` catalog row
(additive; the user-scope `get()` is untouched, so other tools/paths are
unaffected).
- reader: `AnalysisScopedCatalogReader` serves analysis-scope for STRUCTURED
reads (the naming/scope gap is DB-only), passing documents + any miss through
to the user-scope reader so unbound/legacy rooms behave exactly as before.
- chat_handler: the check branch builds the invoker with that reader.
- check: the inventory listing drills each database for its table names and
nests them (uncapped) under the source, so a DB is no longer an opaque
"N tables" and a "what are the other tables?" follow-up is already answered.

Note: not yet verified end-to-end against a live DB (unit-tested with fakes).

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

src/agents/chat_handler.py CHANGED
@@ -182,13 +182,17 @@ class ChatHandler:
182
  self._document_retriever = RetrievalRouter()
183
  return self._document_retriever
184
 
185
- def _get_check_invoker(self, user_id: str) -> Any:
186
- """Build the per-request data-access invoker for the `check` skill."""
 
 
 
 
187
  if self._check_invoker_factory is not None:
188
  return self._check_invoker_factory(user_id)
189
  from ..tools.data_access import DataAccessToolInvoker
190
 
191
- return DataAccessToolInvoker(user_id, self._get_catalog_reader())
192
 
193
  def _get_ps_agent(self) -> ProblemStatementAgent:
194
  if self._ps_agent is None:
@@ -477,8 +481,19 @@ class ChatHandler:
477
  return
478
  elif intent == "check":
479
  try:
 
 
 
 
 
 
 
 
 
480
  # Wrap the check invoker so its check_* tool calls land in the trace.
481
- invoker = TraceabilityToolInvoker(self._get_check_invoker(user_id), pad)
 
 
482
  # Detect from the ORIGINAL message (not `rewritten`, which the
483
  # router normalizes to English) so the deterministic check reply
484
  # matches the user's language like the other paths.
 
182
  self._document_retriever = RetrievalRouter()
183
  return self._document_retriever
184
 
185
+ def _get_check_invoker(self, user_id: str, catalog_reader: Any = None) -> Any:
186
+ """Build the per-request data-access invoker for the `check` skill.
187
+
188
+ `catalog_reader` lets the caller scope the read (e.g. to the analysis
189
+ catalog); defaults to the user-scope reader.
190
+ """
191
  if self._check_invoker_factory is not None:
192
  return self._check_invoker_factory(user_id)
193
  from ..tools.data_access import DataAccessToolInvoker
194
 
195
+ return DataAccessToolInvoker(user_id, catalog_reader or self._get_catalog_reader())
196
 
197
  def _get_ps_agent(self) -> ProblemStatementAgent:
198
  if self._ps_agent is None:
 
481
  return
482
  elif intent == "check":
483
  try:
484
+ # Scope check to the analysis catalog: it holds only this room's
485
+ # bound sources and their real names (a DB shows as "xl test", not
486
+ # the user-scope `postgres_<hash>` placeholder). Falls back to the
487
+ # user-scope reader when the analysis has no catalog row.
488
+ from ..catalog.reader import AnalysisScopedCatalogReader
489
+
490
+ scoped_reader = AnalysisScopedCatalogReader(
491
+ self._get_catalog_reader(), analysis_id
492
+ )
493
  # Wrap the check invoker so its check_* tool calls land in the trace.
494
+ invoker = TraceabilityToolInvoker(
495
+ self._get_check_invoker(user_id, scoped_reader), pad
496
+ )
497
  # Detect from the ORIGINAL message (not `rewritten`, which the
498
  # router normalizes to English) so the deterministic check reply
499
  # matches the user's language like the other paths.
src/agents/handlers/check.py CHANGED
@@ -179,14 +179,19 @@ def render_tool_output(out: ToolOutput, reply_language: str = "English") -> str:
179
  return f"{header}\n{separator}\n{body}"
180
 
181
 
182
- def _render_source_list(out: ToolOutput, reply_language: str) -> str:
 
 
 
 
183
  """Render a check_data/check_knowledge *listing* as a bullet list, not a table.
184
 
185
  One bullet per source: `- name — Type (N tables)`. The type + table-count
186
  annotation is only added for structured sources (file vs database); documents
187
  are all "unstructured", so the section header already says so — just the name.
188
- Returns '' when there are no rows. (The column-level schema drill-down still
189
- renders as a table via `render_tool_output` that data is genuinely tabular.)
 
190
  """
191
  if out.kind == "error":
192
  return _s(reply_language)["lookup_error"].format(error=out.error)
@@ -197,6 +202,8 @@ def _render_source_list(out: ToolOutput, reply_language: str) -> str:
197
 
198
  idx = {c: i for i, c in enumerate(columns)}
199
  type_labels = _SOURCE_TYPE_LABELS.get(reply_language, _SOURCE_TYPE_LABELS["English"])
 
 
200
 
201
  def _table_word(n: int) -> str:
202
  if reply_language == "English":
@@ -207,16 +214,70 @@ def _render_source_list(out: ToolOutput, reply_language: str) -> str:
207
  for row in rows:
208
  name = str(row[idx["name"]]) if "name" in idx else ""
209
  st = str(row[idx["source_type"]]) if "source_type" in idx else ""
 
210
  annotation = ""
211
  if st and st != "unstructured":
212
  annotation = type_labels.get(st, st)
213
  if "table_count" in idx:
214
  tc = row[idx["table_count"]]
215
  annotation += f" ({tc} {_table_word(int(tc))})"
216
- items.append(f"- {name} — {annotation}" if annotation else f"- {name}")
 
 
 
 
 
 
 
 
 
 
 
 
217
  return "\n".join(items)
218
 
219
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
220
  def _matched_source_ids(message: str, inventory: ToolOutput) -> list[str]:
221
  """All source_ids whose name appears as a whole word in the message.
222
 
@@ -246,13 +307,16 @@ def _matched_source_ids(message: str, inventory: ToolOutput) -> list[str]:
246
 
247
 
248
  def _render_helicopter(
249
- data_out: ToolOutput, knowledge_out: ToolOutput, reply_language: str = "English"
 
 
 
250
  ) -> str:
251
  """Stitch structured + document inventory into one helicopter-view reply."""
252
  strings = _s(reply_language)
253
  parts: list[str] = []
254
 
255
- data_list = _render_source_list(data_out, reply_language)
256
  if data_list:
257
  parts.append(f"{strings['structured']}:\n{data_list}")
258
 
@@ -555,7 +619,8 @@ async def run_check(
555
  inventory = await invoker.invoke("check_data", {})
556
  if inventory.kind == "error":
557
  return render_tool_output(inventory, reply_language)
558
- listing = _render_source_list(inventory, reply_language)
 
559
  if not listing:
560
  return _no_match
561
  n = len(inventory.rows or [])
@@ -566,4 +631,5 @@ async def run_check(
566
  invoker.invoke("check_data", {}),
567
  invoker.invoke("check_knowledge", {}),
568
  )
569
- return _render_helicopter(data_out, knowledge_out, reply_language)
 
 
179
  return f"{header}\n{separator}\n{body}"
180
 
181
 
182
+ def _render_source_list(
183
+ out: ToolOutput,
184
+ reply_language: str,
185
+ db_tables: dict[str, list[tuple[str, Any]]] | None = None,
186
+ ) -> str:
187
  """Render a check_data/check_knowledge *listing* as a bullet list, not a table.
188
 
189
  One bullet per source: `- name — Type (N tables)`. The type + table-count
190
  annotation is only added for structured sources (file vs database); documents
191
  are all "unstructured", so the section header already says so — just the name.
192
+ When `db_tables` supplies a database's table names, they are nested as sub-
193
+ bullets (capped at `_INVENTORY_TABLE_CAP`) so a DB isn't an opaque "N tables".
194
+ Returns '' when there are no rows.
195
  """
196
  if out.kind == "error":
197
  return _s(reply_language)["lookup_error"].format(error=out.error)
 
202
 
203
  idx = {c: i for i, c in enumerate(columns)}
204
  type_labels = _SOURCE_TYPE_LABELS.get(reply_language, _SOURCE_TYPE_LABELS["English"])
205
+ sc = _sc(reply_language)
206
+ db_tables = db_tables or {}
207
 
208
  def _table_word(n: int) -> str:
209
  if reply_language == "English":
 
214
  for row in rows:
215
  name = str(row[idx["name"]]) if "name" in idx else ""
216
  st = str(row[idx["source_type"]]) if "source_type" in idx else ""
217
+ sid = str(row[idx["source_id"]]) if "source_id" in idx else ""
218
  annotation = ""
219
  if st and st != "unstructured":
220
  annotation = type_labels.get(st, st)
221
  if "table_count" in idx:
222
  tc = row[idx["table_count"]]
223
  annotation += f" ({tc} {_table_word(int(tc))})"
224
+
225
+ head = f"- {name} — {annotation}" if annotation else f"- {name}"
226
+ tables = db_tables.get(sid) if st == "schema" else None
227
+ if tables:
228
+ # Uncapped for now: list every table of the database, so a follow-up
229
+ # "what are the other tables?" is inherently already answered.
230
+ lines = [head + ":"]
231
+ for tname, rc in tables:
232
+ suffix = f" ({rc} {sc['rows_word']})" if rc else ""
233
+ lines.append(f" - {tname}{suffix}")
234
+ items.append("\n".join(lines))
235
+ else:
236
+ items.append(head)
237
  return "\n".join(items)
238
 
239
 
240
+ def _distinct_tables(out: ToolOutput) -> list[tuple[str, Any]]:
241
+ """(table_name, row_count) pairs from a check_data(source_id) output, in order."""
242
+ if out.kind != "table":
243
+ return []
244
+ cols = out.columns or []
245
+ idx = {c: i for i, c in enumerate(cols)}
246
+ if "table_name" not in idx:
247
+ return []
248
+ seen: dict[str, Any] = {}
249
+ for r in out.rows or []:
250
+ tname = str(r[idx["table_name"]])
251
+ if tname not in seen:
252
+ seen[tname] = r[idx["table_row_count"]] if "table_row_count" in idx else None
253
+ return list(seen.items())
254
+
255
+
256
+ async def _fetch_db_tables(
257
+ inventory: ToolOutput, invoker: ToolInvoker
258
+ ) -> dict[str, list[tuple[str, Any]]]:
259
+ """Drill each database source in an inventory for its table names + row counts.
260
+
261
+ Only `schema` (database) sources are drilled — tabular files are always a
262
+ single table, so their `(N tabel)` line already says everything. Returns a
263
+ map keyed by source_id for `_render_source_list` to nest.
264
+ """
265
+ cols = inventory.columns or []
266
+ idx = {c: i for i, c in enumerate(cols)}
267
+ if "source_type" not in idx or "source_id" not in idx:
268
+ return {}
269
+ dbs = [r for r in (inventory.rows or []) if str(r[idx["source_type"]]) == "schema"]
270
+ if not dbs:
271
+ return {}
272
+ outs = await asyncio.gather(
273
+ *(invoker.invoke("check_data", {"source_id": str(r[idx["source_id"]])}) for r in dbs)
274
+ )
275
+ return {
276
+ str(r[idx["source_id"]]): _distinct_tables(o)
277
+ for r, o in zip(dbs, outs, strict=True)
278
+ }
279
+
280
+
281
  def _matched_source_ids(message: str, inventory: ToolOutput) -> list[str]:
282
  """All source_ids whose name appears as a whole word in the message.
283
 
 
307
 
308
 
309
  def _render_helicopter(
310
+ data_out: ToolOutput,
311
+ knowledge_out: ToolOutput,
312
+ reply_language: str = "English",
313
+ db_tables: dict[str, list[tuple[str, Any]]] | None = None,
314
  ) -> str:
315
  """Stitch structured + document inventory into one helicopter-view reply."""
316
  strings = _s(reply_language)
317
  parts: list[str] = []
318
 
319
+ data_list = _render_source_list(data_out, reply_language, db_tables)
320
  if data_list:
321
  parts.append(f"{strings['structured']}:\n{data_list}")
322
 
 
619
  inventory = await invoker.invoke("check_data", {})
620
  if inventory.kind == "error":
621
  return render_tool_output(inventory, reply_language)
622
+ db_tables = await _fetch_db_tables(inventory, invoker)
623
+ listing = _render_source_list(inventory, reply_language, db_tables)
624
  if not listing:
625
  return _no_match
626
  n = len(inventory.rows or [])
 
631
  invoker.invoke("check_data", {}),
632
  invoker.invoke("check_knowledge", {}),
633
  )
634
+ db_tables = await _fetch_db_tables(data_out, invoker)
635
+ return _render_helicopter(data_out, knowledge_out, reply_language, db_tables)
src/catalog/reader.py CHANGED
@@ -7,12 +7,24 @@ Catalog-level search is added later if catalog grows past the limit.
7
  from datetime import UTC, datetime
8
  from typing import Literal
9
 
10
- from .models import Catalog
11
  from .store import CatalogStore
12
 
13
  SourceHint = Literal["chat", "unstructured", "structured"]
14
 
15
 
 
 
 
 
 
 
 
 
 
 
 
 
16
  class CatalogReader:
17
  """Loads the user's catalog and filters by source_hint.
18
 
@@ -30,14 +42,7 @@ class CatalogReader:
30
  if catalog is None:
31
  return Catalog(user_id=user_id, generated_at=datetime.now(UTC))
32
 
33
- if source_hint == "chat":
34
- filtered: list = []
35
- elif source_hint == "structured":
36
- filtered = [s for s in catalog.sources if s.source_type in {"schema", "tabular"}]
37
- else: # "unstructured"
38
- filtered = [s for s in catalog.sources if s.source_type == "unstructured"]
39
-
40
- return catalog.model_copy(update={"sources": filtered})
41
 
42
 
43
  class MemoizingCatalogReader(CatalogReader):
@@ -66,3 +71,38 @@ class MemoizingCatalogReader(CatalogReader):
66
  cached = await self._inner.read(user_id, source_hint)
67
  self._cache[source_hint] = cached
68
  return cached
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
7
  from datetime import UTC, datetime
8
  from typing import Literal
9
 
10
+ from .models import Catalog, Source
11
  from .store import CatalogStore
12
 
13
  SourceHint = Literal["chat", "unstructured", "structured"]
14
 
15
 
16
+ def _filter_sources(catalog: Catalog, source_hint: SourceHint) -> Catalog:
17
+ """Return a copy of `catalog` keeping only the sources matching `source_hint`."""
18
+ filtered: list[Source]
19
+ if source_hint == "chat":
20
+ filtered = []
21
+ elif source_hint == "structured":
22
+ filtered = [s for s in catalog.sources if s.source_type in {"schema", "tabular"}]
23
+ else: # "unstructured"
24
+ filtered = [s for s in catalog.sources if s.source_type == "unstructured"]
25
+ return catalog.model_copy(update={"sources": filtered})
26
+
27
+
28
  class CatalogReader:
29
  """Loads the user's catalog and filters by source_hint.
30
 
 
42
  if catalog is None:
43
  return Catalog(user_id=user_id, generated_at=datetime.now(UTC))
44
 
45
+ return _filter_sources(catalog, source_hint)
 
 
 
 
 
 
 
46
 
47
 
48
  class MemoizingCatalogReader(CatalogReader):
 
71
  cached = await self._inner.read(user_id, source_hint)
72
  self._cache[source_hint] = cached
73
  return cached
74
+
75
+
76
+ class AnalysisScopedCatalogReader(CatalogReader):
77
+ """Reads the analysis-scope catalog, falling back to the user-scope reader.
78
+
79
+ Used by the `check` skill so "what data do I have" inside a room reflects
80
+ that analysis's bound sources AND their real names — a database shows as
81
+ "xl test" (analysis-scope) instead of the auto-generated `postgres_<hash>`
82
+ placeholder stored in the user-scope row. When the analysis has no catalog
83
+ row (legacy / not yet bound) or the read fails, it degrades to the wrapped
84
+ user-scope reader, so unbound rooms behave exactly as before.
85
+ """
86
+
87
+ def __init__(self, inner: CatalogReader, analysis_id: str | None) -> None:
88
+ # `inner` is a real CatalogReader (constructed at the check call site), so
89
+ # its `_store` is the live CatalogStore we need for the analysis read.
90
+ super().__init__(inner._store)
91
+ self._inner = inner
92
+ self._analysis_id = analysis_id
93
+
94
+ async def read(self, user_id: str, source_hint: SourceHint) -> Catalog:
95
+ # Only STRUCTURED reads get analysis scope — that's where the naming/scope
96
+ # problem lives (databases named `postgres_<hash>` in user-scope vs their
97
+ # real name in analysis-scope). Documents pass through to the user-scope
98
+ # reader: they carry no scope-name gap, and we don't assume Go populates
99
+ # the analysis-scope catalog with unstructured sources (scoping them could
100
+ # wrongly hide a user's documents in a room).
101
+ if source_hint == "structured" and self._analysis_id:
102
+ try:
103
+ catalog = await self._store.get_by_analysis(self._analysis_id)
104
+ except Exception: # noqa: BLE001 — never block check on the analysis read
105
+ catalog = None
106
+ if catalog is not None:
107
+ return _filter_sources(catalog, source_hint)
108
+ return await self._inner.read(user_id, source_hint)
src/catalog/store.py CHANGED
@@ -43,6 +43,28 @@ class CatalogStore:
43
  # edges so the planner and validator agree. No-op once Go emits real FKs.
44
  return infer_foreign_keys(Catalog.model_validate(row))
45
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
46
  async def upsert(self, catalog: Catalog) -> None:
47
  # Legacy: Go's catalog.Service owns catalog writes now. Kept working (and
48
  # reconciled to the dedorch shape) but no longer on any live Python path.
@@ -86,7 +108,9 @@ class CatalogStore:
86
  return
87
  filtered = [s for s in existing.sources if s.source_id != source_id]
88
  if len(filtered) == len(existing.sources):
89
- logger.info("remove_source: source not in catalog", user_id=user_id, source_id=source_id)
 
 
90
  return
91
  await self.upsert(existing.model_copy(update={"sources": filtered}))
92
  logger.info("remove_source: source removed", user_id=user_id, source_id=source_id)
 
43
  # edges so the planner and validator agree. No-op once Go emits real FKs.
44
  return infer_foreign_keys(Catalog.model_validate(row))
45
 
46
+ async def get_by_analysis(self, analysis_id: str) -> Catalog | None:
47
+ """Read the `scope_type='analysis'` catalog row for an analysis.
48
+
49
+ Distinct from `get()` (which reads the user-scope row): the analysis-scope
50
+ payload carries the sources actually bound to this analysis AND their
51
+ real names (a database is named e.g. "xl test" here, vs the auto-generated
52
+ `postgres_<hash>` placeholder in the user-scope row). Returns None when the
53
+ analysis has no catalog row (legacy / not yet bound), so callers fall back
54
+ to the user-scope catalog.
55
+ """
56
+ async with AsyncSessionLocal() as session:
57
+ result = await session.execute(
58
+ select(CatalogRow.catalog_payload).where(
59
+ CatalogRow.analysis_id == analysis_id,
60
+ CatalogRow.scope_type == "analysis",
61
+ )
62
+ )
63
+ row = result.scalar_one_or_none()
64
+ if row is None:
65
+ return None
66
+ return infer_foreign_keys(Catalog.model_validate(row))
67
+
68
  async def upsert(self, catalog: Catalog) -> None:
69
  # Legacy: Go's catalog.Service owns catalog writes now. Kept working (and
70
  # reconciled to the dedorch shape) but no longer on any live Python path.
 
108
  return
109
  filtered = [s for s in existing.sources if s.source_id != source_id]
110
  if len(filtered) == len(existing.sources):
111
+ logger.info(
112
+ "remove_source: source not in catalog", user_id=user_id, source_id=source_id
113
+ )
114
  return
115
  await self.upsert(existing.model_copy(update={"sources": filtered}))
116
  logger.info("remove_source: source removed", user_id=user_id, source_id=source_id)