Rifqi Hafizuddin Claude Opus 4.8 commited on
Commit
fd4865b
·
1 Parent(s): 8dec182

[NOTICKET] fix: split report floor from report body; scope analysis reads by user_id

Browse files

Two defects with one root cause, plus the security pair from the 2026-07-23 review.

Report correctness. Planner recipes R2/R2b make the analyze_* step optional (R2 is
"ONE grouped retrieve_data IR (+/- analyze_aggregate)"; R2b, added yesterday, is
explicitly "NO analyze_* step"), so an aggregate answered entirely inside one
retrieve_data IR is a correct, complete analysis that uses no analyze_* tool. One
predicate gated both "is this session worth a report" and "should this run appear in
it", so those runs were classed non-substantive, dropped from the report body, and
their business question rendered "Unanswered" while the chat had answered it
correctly (analysis 966224d4, two of six runs, both threshold questions, real findings).

- has_reportable_result (BODY): plan HAS an analysis step -> it must have succeeded;
plan has NO analysis step -> a successful non-check_* task is enough. A run whose
analysis step FAILED still stays in `unresolved`.
- has_successful_analysis (FLOOR) additionally clears on a retrieve_data that returned
rows, fixing a hard 409 when every question in a session is R2/R2b-shaped. Not a
relaxation: empty retrievals, check_*-only and fully-failed runs all still fail it.
- Deliberate asymmetry: a zero-row retrieval fails the floor but passes the body --
"no units missed both targets" is a real answer.
- GET .../records `substantive` repointed to the body predicate, so the curation list
stops contradicting the artifact it curates.
- CK5b: CK5 only inspected analyze_* tasks, so on an analyze-free plan an all-null
aggregate column reached the answer unflagged. check_* excluded.
- The dropped-runs path now logs; its silence is why this needed a bug report.

Security. Go makes no outbound call to Python (verified in the Go source), so the
"Go fronts Python" premise in the code comments is not wired and the surface is
reachable directly.

- Service-secret gate (X-Dataeyond-Service-Secret, constant-time) as a router-level
dependency, so a new endpoint cannot skip it. Blank env var = disabled, so this is
inert until dataeyond__service__secret is set; boot logs which mode is active.
- CatalogStore.get_by_analysis filtered on analysis_id alone where Go filters on
analysis_id AND user_id. The catalog payload carries the owner's user_id, so
DbExecutor's ownership check compared the victim's id against itself and passed: a
caller knowing another tenant's analysis_id could execute SQL against that tenant's
database. user_id was already a parameter of AnalysisScopedCatalogReader.read and
simply never passed down. Scoping added to all six analysis-keyed reads; the reports
read tolerates user_id IS NULL for pre-pr/18 rows. A denied read that would have
matched unscoped logs loudly.
- These predicates are defence-in-depth, NOT the control: while user_id is
caller-supplied an attacker can read it from the unauthenticated GET /traceability
response. The gate is what closes the door.

ruff src/: 238 errors, unchanged from baseline (zero new, measured per touched file).
Suite: 394 passed, 0 failed, 7 skipped. import main: 0.

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

API_CONTRACT_BE_PYTHON.md CHANGED
@@ -36,6 +36,20 @@ The frontend uses this service during the analysis conversation flow:
36
  | `GET` | `/api/v1/traceability` | Retrieve provenance for one assistant answer. |
37
  | `GET` | `/api/v1/charts` | Retrieve chart(s) produced by `render_chart` for one assistant answer (added 2026-07-13). |
38
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
39
  ## Common Concepts
40
 
41
  ### Identifiers
@@ -426,7 +440,9 @@ Response `200`:
426
  ]
427
  ```
428
 
429
- `substantive: false` means no `analyze_*` step succeeded that run is listed in the report's `unresolved` JSON field rather than the findings body. (Since 2026-07-09 the rendered markdown is compact and no longer includes "Attempted, Unresolved" / "Notes & Limitations" / "How This Was Analyzed" sections; the JSON fields `unresolved` / `caveats` / `open_questions` / `method_steps` are unchanged.) If no runs exist, returns `[]`.
 
 
430
 
431
  ### `GET /api/v1/tools/report/{analysis_id}/readiness` (added 2026-07-09)
432
 
 
36
  | `GET` | `/api/v1/traceability` | Retrieve provenance for one assistant answer. |
37
  | `GET` | `/api/v1/charts` | Retrieve chart(s) produced by `render_chart` for one assistant answer (added 2026-07-13). |
38
 
39
+ ## Authentication (added 2026-07-23)
40
+
41
+ Every endpoint in the table above sits behind a **shared service secret**. Callers send it as a header on every request:
42
+
43
+ ```
44
+ X-Dataeyond-Service-Secret: <secret>
45
+ ```
46
+
47
+ A missing or incorrect value returns `401` with `{"detail": "Missing or invalid service credentials."}`. `GET /` and `GET /health` are exempt (deployment health probes).
48
+
49
+ **Rollout state — currently inert.** The gate is **disabled unless** the service is deployed with `dataeyond__service__secret` set. Until that env var is configured, requests without the header succeed exactly as before, so no frontend change is required to keep working today. Send the header as soon as the secret is issued; it is safe to send before the gate is armed (an unconfigured service ignores it).
50
+
51
+ **What this is and is not.** It authenticates the *calling service*, not the end user — it does not identify which user is making the request, and it is not a substitute for the frontend's own auth. It exists because this service currently accepts `user_id` and `analysis_id` as ordinary request fields with no verification, and is reachable directly. Per-user authorization will move to a verified identity forwarded by the Go service; when it does, `user_id` will come from the verified token rather than the request body, and the request fields documented below may become redundant.
52
+
53
  ## Common Concepts
54
 
55
  ### Identifiers
 
440
  ]
441
  ```
442
 
443
+ `substantive: true` means **the run will appear in the report's findings body**; `substantive: false` means it will be listed in the report's `unresolved` JSON field instead. (Since 2026-07-09 the rendered markdown is compact and no longer includes "Attempted, Unresolved" / "Notes & Limitations" / "How This Was Analyzed" sections; the JSON fields `unresolved` / `caveats` / `open_questions` / `method_steps` are unchanged.) If no runs exist, returns `[]`.
444
+
445
+ > **Changed 2026-07-23 — behavioral, non-breaking (field name, type and position unchanged).** `substantive` previously meant the narrower "a successful `analyze_*` step exists". Planner recipes R2/R2b legitimately answer a question with a single aggregate `retrieve_data` and **no** `analyze_*` step, so under the old rule such a run was reported `substantive: false`, was dropped from the report body, and its business question rendered **"Unanswered"** in `bq_answers` — while the chat had answered it correctly. `substantive` now tracks the report body exactly. **FE impact:** none required. A run that previously showed `false` and was silently excluded may now show `true` and be included; no field was added, removed or retyped.
446
 
447
  ### `GET /api/v1/tools/report/{analysis_id}/readiness` (added 2026-07-09)
448
 
main.py CHANGED
@@ -2,9 +2,10 @@
2
 
3
  from contextlib import asynccontextmanager
4
 
5
- from fastapi import FastAPI
6
  from src.middlewares.logging import configure_logging, get_logger
7
  from src.middlewares.cors import add_cors_middleware
 
8
  from src.middlewares.rate_limit import limiter, _rate_limit_exceeded_handler
9
  from slowapi.errors import RateLimitExceeded
10
  # --- pr/5 Phase 1: unwire non-AI routers (Go owns these now). ---
@@ -41,6 +42,16 @@ async def lifespan(app: FastAPI):
41
  logger.info("Database initialized")
42
  else:
43
  logger.info("Skipping database initialization (SKIP_INIT_DB=true)")
 
 
 
 
 
 
 
 
 
 
44
  yield
45
 
46
 
@@ -65,12 +76,19 @@ app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
65
  # app.include_router(db_client_router) # unwired: Go registers DB client
66
  # app.include_router(data_catalog_router) # unwired: Go handles the catalog
67
  # app.include_router(chat_router) # unwired: v2 chat replaces it (drops v1 cache ops routes)
68
- app.include_router(report_router)
69
- app.include_router(tools_router)
70
- app.include_router(help_router)
71
- app.include_router(traceability_router) # KM-691: GET /api/v1/traceability
72
- app.include_router(charts_router) # W2: GET /api/v1/charts (SPINE_V2_PLAN §4.5)
73
- app.include_router(chat_v2_router) # pr/5 Phase 2: POST /api/v2/chat/stream (analysis_id)
 
 
 
 
 
 
 
74
 
75
 
76
  @app.get("/")
 
2
 
3
  from contextlib import asynccontextmanager
4
 
5
+ from fastapi import Depends, FastAPI
6
  from src.middlewares.logging import configure_logging, get_logger
7
  from src.middlewares.cors import add_cors_middleware
8
+ from src.middlewares.service_auth import is_enforced, require_service_secret
9
  from src.middlewares.rate_limit import limiter, _rate_limit_exceeded_handler
10
  from slowapi.errors import RateLimitExceeded
11
  # --- pr/5 Phase 1: unwire non-AI routers (Go owns these now). ---
 
42
  logger.info("Database initialized")
43
  else:
44
  logger.info("Skipping database initialization (SKIP_INIT_DB=true)")
45
+ # Make the security posture visible at boot: an unset secret means the whole
46
+ # surface is open, which is a deployment fact worth seeing in the logs rather
47
+ # than inferring from the absence of 401s.
48
+ if is_enforced():
49
+ logger.info("Service-secret gate ENFORCED on all live routers")
50
+ else:
51
+ logger.warning(
52
+ "Service-secret gate DISABLED (dataeyond__service__secret unset) — "
53
+ "every endpoint is reachable without credentials"
54
+ )
55
  yield
56
 
57
 
 
76
  # app.include_router(db_client_router) # unwired: Go registers DB client
77
  # app.include_router(data_catalog_router) # unwired: Go handles the catalog
78
  # app.include_router(chat_router) # unwired: v2 chat replaces it (drops v1 cache ops routes)
79
+ # F-2 (2026-07-23): every live router is mounted behind the service-secret gate.
80
+ # Applied here rather than per route so a new endpoint cannot be added without it.
81
+ # The dependency is a NO-OP until `dataeyond__service__secret` is set, so this is
82
+ # inert for local dev / the current FE until the secret is deployed on both sides.
83
+ # `/` and `/health` stay open for the HF Space health probe.
84
+ _guard = [Depends(require_service_secret)]
85
+
86
+ app.include_router(report_router, dependencies=_guard)
87
+ app.include_router(tools_router, dependencies=_guard)
88
+ app.include_router(help_router, dependencies=_guard)
89
+ app.include_router(traceability_router, dependencies=_guard) # KM-691: GET /api/v1/traceability
90
+ app.include_router(charts_router, dependencies=_guard) # W2: GET /api/v1/charts (§4.5)
91
+ app.include_router(chat_v2_router, dependencies=_guard) # pr/5 Phase 2: POST /api/v2/chat/stream
92
 
93
 
94
  @app.get("/")
src/agents/chat_handler.py CHANGED
@@ -272,7 +272,7 @@ class ChatHandler:
272
  # not-ready) — the HelpAgent guard only offers generate_report when ready.
273
  from .report.readiness import is_report_ready
274
 
275
- report_ready = await is_report_ready(analysis_id, state)
276
 
277
  yield {"event": "sources", "data": json.dumps([])}
278
  try:
@@ -525,7 +525,7 @@ class ChatHandler:
525
  # HelpAgent only offers `generate_report` when this says ready.
526
  from .report.readiness import is_report_ready
527
 
528
- report_ready = await is_report_ready(analysis_id, state)
529
  # The prompt sees chat history -> masked.
530
  hc = tracer.callbacks(masked=True)
531
  hkw = {"callbacks": hc} if hc else {}
 
272
  # not-ready) — the HelpAgent guard only offers generate_report when ready.
273
  from .report.readiness import is_report_ready
274
 
275
+ report_ready = await is_report_ready(analysis_id, state, user_id=user_id)
276
 
277
  yield {"event": "sources", "data": json.dumps([])}
278
  try:
 
525
  # HelpAgent only offers `generate_report` when this says ready.
526
  from .report.readiness import is_report_ready
527
 
528
+ report_ready = await is_report_ready(analysis_id, state, user_id=user_id)
529
  # The prompt sees chat history -> masked.
530
  hc = tracer.callbacks(masked=True)
531
  hkw = {"callbacks": hc} if hc else {}
src/agents/report/generator.py CHANGED
@@ -607,7 +607,12 @@ class ReportGenerator:
607
  user_name: str | None = None,
608
  exclude_record_ids: list[str] | None = None,
609
  ) -> AnalysisReport:
610
- all_records = await self._ensure_record_store().list_for_analysis(analysis_id)
 
 
 
 
 
611
  excluded_ids = set(exclude_record_ids or [])
612
  excluded = [r for r in all_records if r.record_id in excluded_ids]
613
  kept = [r for r in all_records if r.record_id not in excluded_ids]
@@ -686,7 +691,8 @@ class ReportGenerator:
686
  try:
687
  store = self._ensure_catalog_store()
688
  if analysis_id:
689
- cat = await store.get_by_analysis(analysis_id)
 
690
  if cat is not None:
691
  return cat
692
  return await store.get(user_id) if user_id else None
 
607
  user_name: str | None = None,
608
  exclude_record_ids: list[str] | None = None,
609
  ) -> AnalysisReport:
610
+ # Scoped to the requesting user (2026-07-23): `POST /tools/report` always
611
+ # supplies `user_id`, so a report can only ever be built from records the
612
+ # caller owns.
613
+ all_records = await self._ensure_record_store().list_for_analysis(
614
+ analysis_id, user_id
615
+ )
616
  excluded_ids = set(exclude_record_ids or [])
617
  excluded = [r for r in all_records if r.record_id in excluded_ids]
618
  kept = [r for r in all_records if r.record_id not in excluded_ids]
 
691
  try:
692
  store = self._ensure_catalog_store()
693
  if analysis_id:
694
+ # Scoped to the requesting user (2026-07-23) — see CatalogStore.
695
+ cat = await store.get_by_analysis(analysis_id, user_id)
696
  if cat is not None:
697
  return cat
698
  return await store.get(user_id) if user_id else None
src/agents/report/readiness.py CHANGED
@@ -85,23 +85,56 @@ def _is_analysis_tool(tool: str) -> bool:
85
  return tool.startswith("analyze") or tool == "render_chart"
86
 
87
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
88
  def has_successful_analysis(record) -> bool:
89
- """True if the record has at least one *result-producing* task that succeeded.
90
 
91
  A failed run still writes findings (narrating the failure) and its data-access
92
  tasks (check_/retrieve_) succeed, so we can't key on findings or on "any task
93
  succeeded". A completed analysis tool (analyze_*) — or, since W2 charts
94
  (2026-07-14), a completed `render_chart`, whose viz-tail upstream necessarily
95
- computed the numbers being charted — is the real "we produced a result"
96
- signal. A chart-only session therefore satisfies the report floor.
97
-
98
- This is the report **FLOOR** only. The report *body* uses the broader
99
- `has_reportable_result` see its docstring for why the two diverged.
 
 
 
 
 
 
 
 
 
 
 
 
100
  """
101
- return any(
102
- t.status == "success" and any(_is_analysis_tool(tool) for tool in t.tools_used)
103
- for t in record.tasks_run
104
- )
105
 
106
 
107
  def has_reportable_result(record) -> bool:
@@ -128,13 +161,24 @@ def has_reportable_result(record) -> bool:
128
  - The plan has NO analysis step -> a successful data-producing task is enough
129
  (`check_*` alone is not — that inspected the schema, it did not answer
130
  anything).
 
 
 
 
 
 
 
131
  """
132
  tasks = record.tasks_run
133
  plan_has_analysis = any(
134
  _is_analysis_tool(tool) for task in tasks for tool in task.tools_used
135
  )
136
  if plan_has_analysis:
137
- return has_successful_analysis(record)
 
 
 
 
138
  return any(
139
  task.status == "success"
140
  and any(tool not in _CATALOG_ONLY_TOOLS for tool in task.tools_used)
@@ -147,6 +191,7 @@ async def report_floor(
147
  state: AnalysisState,
148
  *,
149
  record_store=None,
 
150
  ) -> tuple[list[str], list]:
151
  """The report **floor**: ≥1 substantive analysis.
152
 
@@ -169,7 +214,10 @@ async def report_floor(
169
  if analysis_id:
170
  try:
171
  store = record_store or _default_record_store()
172
- records = await store.list_for_analysis(analysis_id)
 
 
 
173
  substantive = [r for r in records if has_successful_analysis(r)]
174
  except Exception as exc: # noqa: BLE001 — never-throw; fail closed to not-ready
175
  logger.warning(
@@ -190,14 +238,16 @@ async def is_report_ready(
190
  *,
191
  record_store=None,
192
  report_store=None,
 
193
  ) -> ReportReadiness:
194
  """Return whether a report can be generated for this analysis, and the gaps if not.
195
 
196
  `record_store` / `report_store` are injectable for tests; they default to the
197
- real Postgres stores.
 
198
  """
199
  missing, substantive = await report_floor(
200
- analysis_id, state, record_store=record_store
201
  )
202
 
203
  if not substantive:
 
85
  return tool.startswith("analyze") or tool == "render_chart"
86
 
87
 
88
+ def _completed_analysis_task(record) -> bool:
89
+ """True if some task using an analysis tool succeeded. The original floor rule."""
90
+ return any(
91
+ t.status == "success" and any(_is_analysis_tool(tool) for tool in t.tools_used)
92
+ for t in record.tasks_run
93
+ )
94
+
95
+
96
+ def _produced_rows(record) -> bool:
97
+ """True if some successful `retrieve_data` actually returned rows.
98
+
99
+ Read from `results_snapshot`, not `tasks_run` — `TaskSummary` carries tool names
100
+ and status but no row counts. Requiring rows (not merely a successful call) means
101
+ an empty retrieval can never satisfy the floor.
102
+ """
103
+ for result in record.results_snapshot.values():
104
+ if result.status != "success":
105
+ continue
106
+ for out in result.outputs:
107
+ if out.tool == "retrieve_data" and out.kind == "table" and out.rows:
108
+ return True
109
+ return False
110
+
111
+
112
  def has_successful_analysis(record) -> bool:
113
+ """True if the record produced a real result the report **FLOOR**.
114
 
115
  A failed run still writes findings (narrating the failure) and its data-access
116
  tasks (check_/retrieve_) succeed, so we can't key on findings or on "any task
117
  succeeded". A completed analysis tool (analyze_*) — or, since W2 charts
118
  (2026-07-14), a completed `render_chart`, whose viz-tail upstream necessarily
119
+ computed the numbers being charted — is the classic "we produced a result"
120
+ signal. A chart-only session therefore satisfies the floor.
121
+
122
+ **Extended 2026-07-23.** Planner recipes R2/R2b made the `analyze_*` step
123
+ optional, so a correct, complete analysis can consist of exactly one aggregate
124
+ `retrieve_data` IR. Under the original rule such a session had NO substantive
125
+ record at all, and `POST /api/v1/tools/report` returned 409 — "not ready" — for a
126
+ session in which every business question had been answered. A successful
127
+ `retrieve_data` **that returned rows** therefore also clears the floor.
128
+
129
+ This is NOT a relaxation of the floor's intent (cf. the "Floor Fixer" failure
130
+ mode): the floor still asks "did we produce a real result", and an empty
131
+ retrieval, a `check_*`-only run, and a fully-failed run all still fail it. What
132
+ changed is that producing a result no longer requires a specific tool family.
133
+
134
+ The report *body* uses `has_reportable_result` instead — the two are deliberately
135
+ independent; see that docstring.
136
  """
137
+ return _completed_analysis_task(record) or _produced_rows(record)
 
 
 
138
 
139
 
140
  def has_reportable_result(record) -> bool:
 
161
  - The plan has NO analysis step -> a successful data-producing task is enough
162
  (`check_*` alone is not — that inspected the schema, it did not answer
163
  anything).
164
+
165
+ Deliberate asymmetry with the floor: a retrieval that returned **zero rows**
166
+ fails the floor (a session whose only outcome was an empty result is not worth a
167
+ report) but PASSES here. "No units missed both targets" is a real answer to a
168
+ real business question, and excluding it would recreate the original bug for
169
+ negative results. CK2 already flags the empty retrieve so the assembler narrates
170
+ it honestly. Do not "align" these two — the divergence is the point.
171
  """
172
  tasks = record.tasks_run
173
  plan_has_analysis = any(
174
  _is_analysis_tool(tool) for task in tasks for tool in task.tools_used
175
  )
176
  if plan_has_analysis:
177
+ # Deliberately calls `_completed_analysis_task`, NOT `has_successful_analysis`:
178
+ # the floor also clears on a row-producing retrieve_data, and inheriting that
179
+ # here would let a run whose analysis step FAILED back into the body on the
180
+ # strength of its upstream fetch. The two predicates stay independent.
181
+ return _completed_analysis_task(record)
182
  return any(
183
  task.status == "success"
184
  and any(tool not in _CATALOG_ONLY_TOOLS for tool in task.tools_used)
 
191
  state: AnalysisState,
192
  *,
193
  record_store=None,
194
+ user_id: str | None = None,
195
  ) -> tuple[list[str], list]:
196
  """The report **floor**: ≥1 substantive analysis.
197
 
 
214
  if analysis_id:
215
  try:
216
  store = record_store or _default_record_store()
217
+ # `user_id` scopes the read to the analysis's owner when the caller has
218
+ # one (2026-07-23); None means "unscoped", per the ReportInputStore
219
+ # Protocol default.
220
+ records = await store.list_for_analysis(analysis_id, user_id)
221
  substantive = [r for r in records if has_successful_analysis(r)]
222
  except Exception as exc: # noqa: BLE001 — never-throw; fail closed to not-ready
223
  logger.warning(
 
238
  *,
239
  record_store=None,
240
  report_store=None,
241
+ user_id: str | None = None,
242
  ) -> ReportReadiness:
243
  """Return whether a report can be generated for this analysis, and the gaps if not.
244
 
245
  `record_store` / `report_store` are injectable for tests; they default to the
246
+ real Postgres stores. `user_id`, when the caller has one, scopes the record read
247
+ to the analysis's owner (2026-07-23).
248
  """
249
  missing, substantive = await report_floor(
250
+ analysis_id, state, record_store=record_store, user_id=user_id
251
  )
252
 
253
  if not substantive:
src/agents/report/store.py CHANGED
@@ -14,7 +14,7 @@ from __future__ import annotations
14
 
15
  import hashlib
16
 
17
- from sqlalchemy import func, select, text
18
 
19
  from src.db.postgres.connection import AsyncSessionLocal
20
  from src.db.postgres.models import AnalysisReportRow
@@ -96,11 +96,30 @@ class ReportStore:
96
  )
97
  return report
98
 
99
- async def list_for_analysis(self, analysis_id: str) -> list[AnalysisReport]:
 
 
 
 
 
 
 
 
 
 
 
100
  async with AsyncSessionLocal() as session:
 
 
 
 
 
 
 
 
101
  result = await session.execute(
102
  select(AnalysisReportRow)
103
- .where(AnalysisReportRow.analysis_id == analysis_id)
104
  .order_by(AnalysisReportRow.version.asc())
105
  )
106
  rows = result.scalars().all()
 
14
 
15
  import hashlib
16
 
17
+ from sqlalchemy import func, or_, select, text
18
 
19
  from src.db.postgres.connection import AsyncSessionLocal
20
  from src.db.postgres.models import AnalysisReportRow
 
96
  )
97
  return report
98
 
99
+ async def list_for_analysis(
100
+ self, analysis_id: str, user_id: str | None = None
101
+ ) -> list[AnalysisReport]:
102
+ """Every version for one analysis, oldest-first.
103
+
104
+ `user_id` scopes the read to the owner (2026-07-23) but **tolerates NULL**:
105
+ `ReportStore.save` did not write `reports.user_id` until pr/18 (2026-07-22),
106
+ so every report generated before that date has a NULL owner. A strict
107
+ equality filter would hide them, so legacy rows are matched too — the loosest-
108
+ deployment-shape convention (§7D). Drop the NULL branch once the column is
109
+ backfilled.
110
+ """
111
  async with AsyncSessionLocal() as session:
112
+ where = [AnalysisReportRow.analysis_id == analysis_id]
113
+ if user_id is not None:
114
+ where.append(
115
+ or_(
116
+ AnalysisReportRow.user_id == user_id,
117
+ AnalysisReportRow.user_id.is_(None),
118
+ )
119
+ )
120
  result = await session.execute(
121
  select(AnalysisReportRow)
122
+ .where(*where)
123
  .order_by(AnalysisReportRow.version.asc())
124
  )
125
  rows = result.scalars().all()
src/agents/slow_path/checkpoint.py CHANGED
@@ -112,12 +112,15 @@ def _assess(run_state: RunState, task_list: TaskList) -> RunAssessment:
112
  # CK5 — an analyze_* consumed a table whose column(s) are entirely null.
113
  # Consumption is read from the PLAN (the `data`/`data_right` placeholders);
114
  # the runner resolves the same references at execution time.
 
115
  for task in task_list.tasks:
116
  for call in task.tool_calls:
117
  if not call.tool.startswith("analyze_"):
118
  continue
119
  for arg_name in ("data", "data_right"):
120
  ref = _placeholder_ref(call.args.get(arg_name))
 
 
121
  table = _last_table_output(results.get(ref)) if ref else None
122
  if table is None:
123
  continue
@@ -128,6 +131,27 @@ def _assess(run_state: RunState, task_list: TaskList) -> RunAssessment:
128
  "null — results based on them are meaningless",
129
  repairable=True)
130
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
131
  assessments: list[TaskAssessment] = []
132
  for tid, result in results.items():
133
  if result.status == "failure":
 
112
  # CK5 — an analyze_* consumed a table whose column(s) are entirely null.
113
  # Consumption is read from the PLAN (the `data`/`data_right` placeholders);
114
  # the runner resolves the same references at execution time.
115
+ consumed_refs: set[str] = set()
116
  for task in task_list.tasks:
117
  for call in task.tool_calls:
118
  if not call.tool.startswith("analyze_"):
119
  continue
120
  for arg_name in ("data", "data_right"):
121
  ref = _placeholder_ref(call.args.get(arg_name))
122
+ if ref:
123
+ consumed_refs.add(ref)
124
  table = _last_table_output(results.get(ref)) if ref else None
125
  if table is None:
126
  continue
 
131
  "null — results based on them are meaningless",
132
  repairable=True)
133
 
134
+ # CK5b — the same defect on a retrieval NO analyze_* consumed, i.e. one that goes
135
+ # straight to the assembler. Planner recipes R2/R2b answer a question with exactly
136
+ # one aggregate `retrieve_data` and no analyze_* step, so the consumer-side sweep
137
+ # above never runs for them and an all-null aggregate column (e.g. avg(PA) over a
138
+ # window where PA was never recorded) reached the answer unflagged. Found
139
+ # 2026-07-23, same root cause as the report-body bug. Restricted to
140
+ # `retrieve_data`: `check_*` outputs legitimately carry all-null columns (an
141
+ # uncounted table surfaces `table_row_count = None`).
142
+ for tid, result in results.items():
143
+ if tid in consumed_refs or result.status != "success":
144
+ continue
145
+ table = _last_table_output(result)
146
+ if table is None or table.tool != "retrieve_data":
147
+ continue
148
+ null_cols = _all_null_columns(table)
149
+ if null_cols:
150
+ flag(tid, "CK5",
151
+ f"column(s) {null_cols} in this result are entirely null — "
152
+ "findings based on them are meaningless",
153
+ repairable=True)
154
+
155
  assessments: list[TaskAssessment] = []
156
  for tid, result in results.items():
157
  if result.status == "failure":
src/agents/slow_path/store.py CHANGED
@@ -41,7 +41,9 @@ class ReportInputStore(Protocol):
41
 
42
  async def save(self, record: AnalysisRecord) -> None: ...
43
 
44
- async def list_for_analysis(self, analysis_id: str) -> list[AnalysisRecord]: ...
 
 
45
 
46
 
47
  class NullReportInputStore:
@@ -55,7 +57,9 @@ class NullReportInputStore:
55
  n_tasks=len(record.tasks_run),
56
  )
57
 
58
- async def list_for_analysis(self, analysis_id: str) -> list[AnalysisRecord]:
 
 
59
  return []
60
 
61
 
@@ -99,11 +103,23 @@ class PostgresReportInputStore:
99
  error=str(exc),
100
  )
101
 
102
- async def list_for_analysis(self, analysis_id: str) -> list[AnalysisRecord]:
 
 
 
 
 
 
 
 
 
103
  async with AsyncSessionLocal() as session:
 
 
 
104
  result = await session.execute(
105
  select(ReportInputRow.data)
106
- .where(ReportInputRow.analysis_id == analysis_id)
107
  .order_by(ReportInputRow.created_at.asc())
108
  )
109
  rows = result.scalars().all()
 
41
 
42
  async def save(self, record: AnalysisRecord) -> None: ...
43
 
44
+ async def list_for_analysis(
45
+ self, analysis_id: str, user_id: str | None = None
46
+ ) -> list[AnalysisRecord]: ...
47
 
48
 
49
  class NullReportInputStore:
 
57
  n_tasks=len(record.tasks_run),
58
  )
59
 
60
+ async def list_for_analysis(
61
+ self, analysis_id: str, user_id: str | None = None
62
+ ) -> list[AnalysisRecord]:
63
  return []
64
 
65
 
 
103
  error=str(exc),
104
  )
105
 
106
+ async def list_for_analysis(
107
+ self, analysis_id: str, user_id: str | None = None
108
+ ) -> list[AnalysisRecord]:
109
+ """Records for one analysis, oldest-first.
110
+
111
+ `user_id` scopes the read to the analysis's owner (2026-07-23). Optional so
112
+ the unthreaded call sites (`report_floor`, `GET …/records`, `GET …/readiness`
113
+ — none of which currently receive a user_id) keep working; those endpoints
114
+ gaining the parameter is the remaining half of the change.
115
+ """
116
  async with AsyncSessionLocal() as session:
117
+ where = [ReportInputRow.analysis_id == analysis_id]
118
+ if user_id is not None:
119
+ where.append(ReportInputRow.user_id == user_id)
120
  result = await session.execute(
121
  select(ReportInputRow.data)
122
+ .where(*where)
123
  .order_by(ReportInputRow.created_at.asc())
124
  )
125
  rows = result.scalars().all()
src/api/v1/chat.py CHANGED
@@ -99,15 +99,25 @@ async def cache_response(redis, cache_key: str, response: str, sources: list):
99
  )
100
 
101
 
102
- async def load_history(db: AsyncSession, analysis_id: str, limit: int = 10) -> list:
 
 
103
  """Load recent conversation messages for an analysis as LangChain messages (oldest-first).
104
 
105
  Reads the dedorch `analyses_messages` table (`role ∈ user|ai`), which replaced the
106
  deprecated `rooms`/`chat_messages`.
 
 
 
 
 
107
  """
 
 
 
108
  result = await db.execute(
109
  select(AnalysesMessageRow)
110
- .where(AnalysesMessageRow.analysis_id == analysis_id)
111
  .order_by(AnalysesMessageRow.created_at.asc())
112
  .limit(limit)
113
  )
 
99
  )
100
 
101
 
102
+ async def load_history(
103
+ db: AsyncSession, analysis_id: str, limit: int = 10, user_id: str | None = None
104
+ ) -> list:
105
  """Load recent conversation messages for an analysis as LangChain messages (oldest-first).
106
 
107
  Reads the dedorch `analyses_messages` table (`role ∈ user|ai`), which replaced the
108
  deprecated `rooms`/`chat_messages`.
109
+
110
+ `user_id` scopes the read to the analysis's owner (2026-07-23). Safe for both
111
+ roles: Go's own `ListByAnalysis` filters `WHERE analysis_id=$1 AND user_id=$2`
112
+ (`message_repo.go`), so `role='ai'` rows necessarily carry the same `user_id` —
113
+ otherwise Go's own history reads would lose every AI reply.
114
  """
115
+ where = [AnalysesMessageRow.analysis_id == analysis_id]
116
+ if user_id is not None:
117
+ where.append(AnalysesMessageRow.user_id == user_id)
118
  result = await db.execute(
119
  select(AnalysesMessageRow)
120
+ .where(*where)
121
  .order_by(AnalysesMessageRow.created_at.asc())
122
  .limit(limit)
123
  )
src/api/v1/report.py CHANGED
@@ -141,7 +141,7 @@ async def generate_report(
141
 
142
  state = await _load_state(analysis_id)
143
  floor_missing, _ = await report_floor(
144
- analysis_id, state or stub_analysis_state()
145
  )
146
  if floor_missing:
147
  raise HTTPException(
@@ -238,8 +238,15 @@ async def list_analysis_records(analysis_id: str):
238
 
239
  The FE shows this list before generating so the user can deselect runs; the
240
  chosen ids go to POST /tools/report as `exclude_record_ids`.
 
 
 
 
 
 
 
241
  """
242
- from src.agents.report.readiness import has_successful_analysis
243
  from src.agents.slow_path.store import PostgresReportInputStore
244
 
245
  try:
@@ -256,7 +263,7 @@ async def list_analysis_records(analysis_id: str):
256
  record_id=r.record_id,
257
  goal_restated=r.goal_restated,
258
  created_at=r.created_at,
259
- substantive=has_successful_analysis(r),
260
  findings_count=len(r.findings),
261
  )
262
  for r in records
 
141
 
142
  state = await _load_state(analysis_id)
143
  floor_missing, _ = await report_floor(
144
+ analysis_id, state or stub_analysis_state(), user_id=user_id
145
  )
146
  if floor_missing:
147
  raise HTTPException(
 
238
 
239
  The FE shows this list before generating so the user can deselect runs; the
240
  chosen ids go to POST /tools/report as `exclude_record_ids`.
241
+
242
+ `substantive` answers "will this run appear in the report?", so it uses
243
+ `has_reportable_result` — the same predicate the generator's body filter uses
244
+ (2026-07-23). It used to use the report FLOOR's predicate, which since planner
245
+ recipes R2/R2b diverged from the body: an analyze-free run showed
246
+ `substantive: false` here while appearing in the report, so the curation list
247
+ contradicted the artifact it curates.
248
  """
249
+ from src.agents.report.readiness import has_reportable_result
250
  from src.agents.slow_path.store import PostgresReportInputStore
251
 
252
  try:
 
263
  record_id=r.record_id,
264
  goal_restated=r.goal_restated,
265
  created_at=r.created_at,
266
+ substantive=has_reportable_result(r),
267
  findings_count=len(r.findings),
268
  )
269
  for r in records
src/api/v2/chat.py CHANGED
@@ -143,7 +143,9 @@ async def chat_stream(
143
  await _save_empty_chat_trace(analysis_id, body.user_id, message_id)
144
  return EventSourceResponse(stream_direct())
145
 
146
- history = await load_history(db, analysis_id, limit=10)
 
 
147
  handler = _chat_handler
148
 
149
  async def stream_response():
 
143
  await _save_empty_chat_trace(analysis_id, body.user_id, message_id)
144
  return EventSourceResponse(stream_direct())
145
 
146
+ # Scoped to the caller (2026-07-23): history for someone else's analysis_id
147
+ # now comes back empty rather than leaking their conversation.
148
+ history = await load_history(db, analysis_id, limit=10, user_id=body.user_id)
149
  handler = _chat_handler
150
 
151
  async def stream_response():
src/catalog/reader.py CHANGED
@@ -117,7 +117,12 @@ class AnalysisScopedCatalogReader(CatalogReader):
117
  # real DB names AND the room's documents (`source_type='unstructured'`),
118
  # unlike the user-scope rows (`postgres_<hash>` names, no documents).
119
  try:
120
- catalog = await self._store.get_by_analysis(self._analysis_id)
 
 
 
 
 
121
  except Exception as e: # noqa: BLE001 — never block check on the analysis read
122
  logger.warning(
123
  "analysis catalog read failed — returning empty",
 
117
  # real DB names AND the room's documents (`source_type='unstructured'`),
118
  # unlike the user-scope rows (`postgres_<hash>` names, no documents).
119
  try:
120
+ # `user_id` scopes the read to the analysis's owner (2026-07-23). It was
121
+ # always a parameter of this method and simply never passed down, so a
122
+ # caller supplying another tenant's `analysis_id` received that tenant's
123
+ # catalog — and every downstream ownership check then compared the
124
+ # victim's id against itself. Go enforces the same pair on its own reads.
125
+ catalog = await self._store.get_by_analysis(self._analysis_id, user_id)
126
  except Exception as e: # noqa: BLE001 — never block check on the analysis read
127
  logger.warning(
128
  "analysis catalog read failed — returning empty",
src/catalog/store.py CHANGED
@@ -49,7 +49,9 @@ class CatalogStore:
49
  decode_sample_values(catalog)
50
  return catalog
51
 
52
- async def get_by_analysis(self, analysis_id: str) -> Catalog | None:
 
 
53
  """Read the `scope_type='analysis'` catalog row for an analysis.
54
 
55
  Distinct from `get()` (which reads the user-scope row): the analysis-scope
@@ -58,15 +60,54 @@ class CatalogStore:
58
  `postgres_<hash>` placeholder in the user-scope row). Returns None when the
59
  analysis has no catalog row (legacy / not yet bound), so callers fall back
60
  to the user-scope catalog.
 
 
 
 
 
 
 
 
 
 
 
 
61
  """
62
  async with AsyncSessionLocal() as session:
 
 
 
 
 
 
63
  result = await session.execute(
64
- select(CatalogRow.catalog_payload).where(
65
- CatalogRow.analysis_id == analysis_id,
66
- CatalogRow.scope_type == "analysis",
67
- )
68
  )
69
  row = result.scalar_one_or_none()
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
70
  if row is None:
71
  return None
72
  catalog = infer_foreign_keys(Catalog.model_validate(row))
 
49
  decode_sample_values(catalog)
50
  return catalog
51
 
52
+ async def get_by_analysis(
53
+ self, analysis_id: str, user_id: str | None = None
54
+ ) -> Catalog | None:
55
  """Read the `scope_type='analysis'` catalog row for an analysis.
56
 
57
  Distinct from `get()` (which reads the user-scope row): the analysis-scope
 
60
  `postgres_<hash>` placeholder in the user-scope row). Returns None when the
61
  analysis has no catalog row (legacy / not yet bound), so callers fall back
62
  to the user-scope catalog.
63
+
64
+ **Tenant scoping (2026-07-23).** When `user_id` is supplied the row must be
65
+ owned by that user. Go enforces exactly this pair on every equivalent read
66
+ (`catalog_repo.go`: `WHERE scope_type='analysis' AND analysis_id=$1 AND
67
+ user_id=$2`); Python filtered on `analysis_id` alone, so a caller who knew
68
+ another tenant's `analysis_id` received that tenant's catalog — and, because
69
+ the payload also carries the owner's `user_id`, `DbExecutor`'s ownership
70
+ check then compared the victim's id against itself and passed, executing SQL
71
+ against their database.
72
+
73
+ `user_id` is optional so the legacy/unthreaded call sites keep working, but
74
+ an unscoped read is logged: those call sites are the remaining work.
75
  """
76
  async with AsyncSessionLocal() as session:
77
+ where = [
78
+ CatalogRow.analysis_id == analysis_id,
79
+ CatalogRow.scope_type == "analysis",
80
+ ]
81
+ if user_id is not None:
82
+ where.append(CatalogRow.user_id == user_id)
83
  result = await session.execute(
84
+ select(CatalogRow.catalog_payload).where(*where)
 
 
 
85
  )
86
  row = result.scalar_one_or_none()
87
+ if row is None and user_id is not None:
88
+ # Diagnose the miss: an owned row that we just refused is either a
89
+ # genuine cross-tenant attempt or a `user_id` format mismatch between
90
+ # what Go wrote and what the caller sent. Both need to be loud; the
91
+ # answer is the same either way (deny).
92
+ probe = await session.execute(
93
+ select(CatalogRow.user_id).where(
94
+ CatalogRow.analysis_id == analysis_id,
95
+ CatalogRow.scope_type == "analysis",
96
+ )
97
+ )
98
+ owner = probe.scalar_one_or_none()
99
+ if owner is not None:
100
+ logger.error(
101
+ "analysis catalog owner mismatch — denied",
102
+ analysis_id=analysis_id,
103
+ requested_by=user_id,
104
+ owner=owner,
105
+ )
106
+ if user_id is None:
107
+ logger.warning(
108
+ "analysis catalog read is UNSCOPED (no user_id) — call site needs threading",
109
+ analysis_id=analysis_id,
110
+ )
111
  if row is None:
112
  return None
113
  catalog = infer_foreign_keys(Catalog.model_validate(row))
src/config/settings.py CHANGED
@@ -99,6 +99,16 @@ class Settings(BaseSettings):
99
  alias="dataeyond__db__credential__key"
100
  )
101
 
 
 
 
 
 
 
 
 
 
 
102
 
103
  # Singleton instance
104
  settings = Settings()
 
99
  alias="dataeyond__db__credential__key"
100
  )
101
 
102
+ # Shared service secret guarding the live surface (F-2, 2026-07-23). Callers send
103
+ # it as the `X-Dataeyond-Service-Secret` header. **Blank by default and blank =
104
+ # disabled**, so nothing breaks until it is deployed on both sides; setting it is
105
+ # what arms the gate. Interim measure only — it authenticates the *caller service*,
106
+ # not the user, and is meant to be replaced by verified per-request identity once
107
+ # Go forwards one. See src/middlewares/service_auth.py.
108
+ dataeyond_service_secret: str = Field(
109
+ alias="dataeyond__service__secret", default=""
110
+ )
111
+
112
 
113
  # Singleton instance
114
  settings = Settings()
src/middlewares/service_auth.py ADDED
@@ -0,0 +1,76 @@
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1
+ """Service-secret gate for the live Python surface (F-2, 2026-07-23).
2
+
3
+ Python has no authentication of its own. The comments in `api/v1/traceability.py`
4
+ and `api/v1/charts.py` say "No auth — Go fronts Python", but Go does not: a
5
+ repo-wide search of the Orchestrator source finds no HTTP client pointed at this
6
+ service and no config key for one, and the FE calls `POST /api/v2/chat/stream`
7
+ directly. So every live endpoint is reachable by anyone who knows the URL, with
8
+ `user_id` and `analysis_id` supplied as ordinary request fields.
9
+
10
+ This is the interim control: a shared secret, carried in a header, checked before
11
+ the route runs. It deliberately does NOT identify *which* user is calling — it only
12
+ stops the open internet. The per-user authorization story is the tenant predicates
13
+ in the stores (see `CatalogStore.get_by_analysis`) plus, eventually, a real
14
+ per-request identity forwarded by Go.
15
+
16
+ Design:
17
+ - **Off unless configured.** With `dataeyond__service__secret` unset the
18
+ dependency is a no-op, so local dev, tests, and the current FE keep working
19
+ unchanged until the secret is deployed on both sides. Setting the env var is
20
+ what arms it — a single, reversible switch.
21
+ - **Constant-time compare** so the check can't be narrowed by timing.
22
+ - **Applied at router mount** (`main.py`), not per route, so a new endpoint cannot
23
+ be added without it.
24
+ - `/` and `/health` stay open — the HF Space health probe has no secret.
25
+
26
+ Replace with JWT verification once Go forwards a real identity; at that point
27
+ `user_id` should come from the verified claims rather than the request body, which
28
+ is what makes the store predicates authoritative instead of merely defensive.
29
+ """
30
+
31
+ from __future__ import annotations
32
+
33
+ import hmac
34
+
35
+ from fastapi import Header, HTTPException, status
36
+
37
+ from src.config.settings import settings
38
+ from src.middlewares.logging import get_logger
39
+
40
+ logger = get_logger("service_auth")
41
+
42
+ # Header name kept vendor-neutral; Go/FE send the same value.
43
+ SERVICE_SECRET_HEADER = "X-Dataeyond-Service-Secret"
44
+
45
+
46
+ def _configured_secret() -> str:
47
+ return (getattr(settings, "dataeyond_service_secret", "") or "").strip()
48
+
49
+
50
+ def is_enforced() -> bool:
51
+ """True when a secret is configured — i.e. the gate actually rejects."""
52
+ return bool(_configured_secret())
53
+
54
+
55
+ async def require_service_secret(
56
+ x_dataeyond_service_secret: str | None = Header(default=None),
57
+ ) -> None:
58
+ """FastAPI dependency: 401 unless the caller presents the configured secret.
59
+
60
+ No-op when no secret is configured, so enabling this is a deployment decision
61
+ rather than a code change.
62
+ """
63
+ expected = _configured_secret()
64
+ if not expected:
65
+ return
66
+ presented = (x_dataeyond_service_secret or "").strip()
67
+ if not presented or not hmac.compare_digest(presented, expected):
68
+ # Deliberately terse: never echo the presented value, never distinguish
69
+ # "missing" from "wrong" to the caller.
70
+ logger.warning(
71
+ "service secret rejected", presented=bool(presented)
72
+ )
73
+ raise HTTPException(
74
+ status_code=status.HTTP_401_UNAUTHORIZED,
75
+ detail="Missing or invalid service credentials.",
76
+ )
src/traceability/store.py CHANGED
@@ -39,7 +39,7 @@ class TraceabilityStore(Protocol):
39
  async def save(self, payload: TraceabilityPayload) -> None: ...
40
 
41
  async def get(
42
- self, analysis_id: str, message_id: str
43
  ) -> TraceabilityPayload | None: ...
44
 
45
 
@@ -55,7 +55,7 @@ class NullTraceabilityStore:
55
  )
56
 
57
  async def get(
58
- self, analysis_id: str, message_id: str
59
  ) -> TraceabilityPayload | None:
60
  return None
61
 
@@ -95,14 +95,26 @@ class PostgresTraceabilityStore:
95
  )
96
 
97
  async def get(
98
- self, analysis_id: str, message_id: str
99
  ) -> TraceabilityPayload | None:
 
 
 
 
 
 
 
 
 
100
  async with AsyncSessionLocal() as session:
 
 
 
 
 
 
101
  result = await session.execute(
102
- select(MessageTraceabilityRow.data).where(
103
- MessageTraceabilityRow.message_id == message_id,
104
- MessageTraceabilityRow.analysis_id == analysis_id,
105
- )
106
  )
107
  row = result.scalar_one_or_none()
108
  if row is None:
 
39
  async def save(self, payload: TraceabilityPayload) -> None: ...
40
 
41
  async def get(
42
+ self, analysis_id: str, message_id: str, user_id: str | None = None
43
  ) -> TraceabilityPayload | None: ...
44
 
45
 
 
55
  )
56
 
57
  async def get(
58
+ self, analysis_id: str, message_id: str, user_id: str | None = None
59
  ) -> TraceabilityPayload | None:
60
  return None
61
 
 
95
  )
96
 
97
  async def get(
98
+ self, analysis_id: str, message_id: str, user_id: str | None = None
99
  ) -> TraceabilityPayload | None:
100
+ """One turn's payload, or None on a miss.
101
+
102
+ `user_id` scopes the read to the turn's owner (2026-07-23). Optional because
103
+ `GET /api/v1/traceability` does not yet accept the parameter — adding it is an
104
+ FE contract change. Until then this row is reachable by `(analysis_id,
105
+ message_id)` alone, and the payload itself carries `user_id`, so the endpoint
106
+ also hands an attacker the id needed to pass every other scoped read. That is
107
+ why the service-secret gate, not this predicate, is the control that matters.
108
+ """
109
  async with AsyncSessionLocal() as session:
110
+ where = [
111
+ MessageTraceabilityRow.message_id == message_id,
112
+ MessageTraceabilityRow.analysis_id == analysis_id,
113
+ ]
114
+ if user_id is not None:
115
+ where.append(MessageTraceabilityRow.user_id == user_id)
116
  result = await session.execute(
117
+ select(MessageTraceabilityRow.data).where(*where)
 
 
 
118
  )
119
  row = result.scalar_one_or_none()
120
  if row is None: