[NOTICKET] fix: split report floor from report body; scope analysis reads by user_id
Browse filesTwo 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 +17 -1
- main.py +25 -7
- src/agents/chat_handler.py +2 -2
- src/agents/report/generator.py +8 -2
- src/agents/report/readiness.py +64 -14
- src/agents/report/store.py +22 -3
- src/agents/slow_path/checkpoint.py +24 -0
- src/agents/slow_path/store.py +20 -4
- src/api/v1/chat.py +12 -2
- src/api/v1/report.py +10 -3
- src/api/v2/chat.py +3 -1
- src/catalog/reader.py +6 -1
- src/catalog/store.py +46 -5
- src/config/settings.py +10 -0
- src/middlewares/service_auth.py +76 -0
- src/traceability/store.py +19 -7
|
@@ -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:
|
|
|
|
|
|
|
| 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 |
|
|
@@ -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 |
-
|
| 69 |
-
|
| 70 |
-
|
| 71 |
-
|
| 72 |
-
|
| 73 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 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("/")
|
|
@@ -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 {}
|
|
@@ -607,7 +607,12 @@ class ReportGenerator:
|
|
| 607 |
user_name: str | None = None,
|
| 608 |
exclude_record_ids: list[str] | None = None,
|
| 609 |
) -> AnalysisReport:
|
| 610 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 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 |
-
|
|
|
|
| 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
|
|
@@ -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
|
| 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
|
| 96 |
-
signal. A chart-only session therefore satisfies the
|
| 97 |
-
|
| 98 |
-
|
| 99 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 100 |
"""
|
| 101 |
-
return
|
| 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 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
| 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 |
-
|
|
|
|
|
|
|
|
|
|
| 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:
|
|
@@ -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(
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 100 |
async with AsyncSessionLocal() as session:
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 101 |
result = await session.execute(
|
| 102 |
select(AnalysisReportRow)
|
| 103 |
-
.where(
|
| 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()
|
|
@@ -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":
|
|
@@ -41,7 +41,9 @@ class ReportInputStore(Protocol):
|
|
| 41 |
|
| 42 |
async def save(self, record: AnalysisRecord) -> None: ...
|
| 43 |
|
| 44 |
-
async def list_for_analysis(
|
|
|
|
|
|
|
| 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(
|
|
|
|
|
|
|
| 59 |
return []
|
| 60 |
|
| 61 |
|
|
@@ -99,11 +103,23 @@ class PostgresReportInputStore:
|
|
| 99 |
error=str(exc),
|
| 100 |
)
|
| 101 |
|
| 102 |
-
async def list_for_analysis(
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 103 |
async with AsyncSessionLocal() as session:
|
|
|
|
|
|
|
|
|
|
| 104 |
result = await session.execute(
|
| 105 |
select(ReportInputRow.data)
|
| 106 |
-
.where(
|
| 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()
|
|
@@ -99,15 +99,25 @@ async def cache_response(redis, cache_key: str, response: str, sources: list):
|
|
| 99 |
)
|
| 100 |
|
| 101 |
|
| 102 |
-
async def load_history(
|
|
|
|
|
|
|
| 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(
|
| 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 |
)
|
|
@@ -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
|
| 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=
|
| 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
|
|
@@ -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 |
-
|
|
|
|
|
|
|
| 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():
|
|
@@ -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 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 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",
|
|
@@ -49,7 +49,9 @@ class CatalogStore:
|
|
| 49 |
decode_sample_values(catalog)
|
| 50 |
return catalog
|
| 51 |
|
| 52 |
-
async def get_by_analysis(
|
|
|
|
|
|
|
| 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))
|
|
@@ -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()
|
|
@@ -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 |
+
)
|
|
@@ -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:
|