[KM-652] fix(report,chat): align report gate with readiness + cleanup
Browse files- T-D: generate_report only 409'd on zero records — it ignored problem_validated
and whether any analysis actually succeeded, so the button could 201 a
findings-empty report while Help said "not ready". Extract report_floor()
(validated goal + >=1 substantive analyze_* success) as the single source of
truth; is_report_ready composes it (+ its delta-check), and the report API
gates on it (no delta — a new version is always allowed, decision 4A).
- T-E: document that reports require ENABLE_SLOW_PATH=on (only the slow path
persists AnalysisRecords; no records => 409 by design), and return a clear
"still needs ..." 409 instead of a bare "no analyses recorded".
- T-F: drop the dead source_hint derivation + _intent_to_source_hint — the
intent event is consumed by the endpoint's cache gate and never forwarded to
the frontend; fix the misleading comment.
- T-G: document the history-blind chat cache key (message-only) limitation;
history-aware key deferred to the endpoint owner.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- src/agents/chat_handler.py +3 -12
- src/agents/report/readiness.py +32 -10
- src/api/v1/chat.py +4 -0
- src/api/v1/report.py +34 -14
|
@@ -269,11 +269,11 @@ class ChatHandler:
|
|
| 269 |
else stub_analysis_state(problem_validated=False),
|
| 270 |
)
|
| 271 |
|
| 272 |
-
#
|
| 273 |
-
#
|
|
|
|
| 274 |
event_data = decision.model_dump()
|
| 275 |
event_data["intent"] = intent
|
| 276 |
-
event_data["source_hint"] = _intent_to_source_hint(intent)
|
| 277 |
yield {"event": "intent", "data": json.dumps(event_data)}
|
| 278 |
|
| 279 |
rewritten = decision.rewritten_query or message
|
|
@@ -597,15 +597,6 @@ class _ScopedCatalogReader:
|
|
| 597 |
return catalog.model_copy(update={"sources": scoped or catalog.sources})
|
| 598 |
|
| 599 |
|
| 600 |
-
def _intent_to_source_hint(intent: str) -> str:
|
| 601 |
-
"""Map a router intent to the legacy `source_hint` value (frontend back-compat)."""
|
| 602 |
-
if intent == "structured_flow":
|
| 603 |
-
return "structured"
|
| 604 |
-
if intent == "unstructured_flow":
|
| 605 |
-
return "unstructured"
|
| 606 |
-
return "chat"
|
| 607 |
-
|
| 608 |
-
|
| 609 |
def _build_sources(
|
| 610 |
intent: str,
|
| 611 |
user_id: str,
|
|
|
|
| 269 |
else stub_analysis_state(problem_validated=False),
|
| 270 |
)
|
| 271 |
|
| 272 |
+
# The `intent` event is consumed by the endpoint (it gates response caching
|
| 273 |
+
# on the effective intent) and is NOT forwarded to the frontend. We emit the
|
| 274 |
+
# post-gate intent so the cache keys on what actually ran.
|
| 275 |
event_data = decision.model_dump()
|
| 276 |
event_data["intent"] = intent
|
|
|
|
| 277 |
yield {"event": "intent", "data": json.dumps(event_data)}
|
| 278 |
|
| 279 |
rewritten = decision.rewritten_query or message
|
|
|
|
| 597 |
return catalog.model_copy(update={"sources": scoped or catalog.sources})
|
| 598 |
|
| 599 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 600 |
def _build_sources(
|
| 601 |
intent: str,
|
| 602 |
user_id: str,
|
|
@@ -85,17 +85,20 @@ def _has_successful_analysis(record) -> bool:
|
|
| 85 |
)
|
| 86 |
|
| 87 |
|
| 88 |
-
async def
|
| 89 |
analysis_id: str | None,
|
| 90 |
state: AnalysisState,
|
| 91 |
*,
|
| 92 |
record_store=None,
|
| 93 |
-
|
| 94 |
-
|
| 95 |
-
|
| 96 |
-
|
| 97 |
-
|
| 98 |
-
|
|
|
|
|
|
|
|
|
|
| 99 |
"""
|
| 100 |
missing: list[str] = []
|
| 101 |
if not state.problem_validated:
|
|
@@ -109,15 +112,34 @@ async def is_report_ready(
|
|
| 109 |
substantive = [r for r in records if _has_successful_analysis(r)]
|
| 110 |
except Exception as exc: # noqa: BLE001 — never-throw; fail closed to not-ready
|
| 111 |
logger.warning(
|
| 112 |
-
"
|
| 113 |
analysis_id=analysis_id,
|
| 114 |
error=str(exc),
|
| 115 |
)
|
| 116 |
-
missing
|
| 117 |
-
return ReportReadiness(ready=False, missing=missing)
|
| 118 |
|
| 119 |
if not substantive:
|
| 120 |
missing.append(_MISSING_ANALYSIS)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 121 |
# No analyses to report on → the delta check is moot.
|
| 122 |
return ReportReadiness(ready=not missing, missing=missing)
|
| 123 |
|
|
|
|
| 85 |
)
|
| 86 |
|
| 87 |
|
| 88 |
+
async def report_floor(
|
| 89 |
analysis_id: str | None,
|
| 90 |
state: AnalysisState,
|
| 91 |
*,
|
| 92 |
record_store=None,
|
| 93 |
+
) -> tuple[list[str], list]:
|
| 94 |
+
"""The report **floor**: a validated goal + ≥1 substantive analysis.
|
| 95 |
+
|
| 96 |
+
Returns `(missing, substantive_records)`. This is the shared gate both the Help
|
| 97 |
+
readiness signal AND the report API enforce, so the button and Help can't drift
|
| 98 |
+
(T-D / T11). It deliberately excludes the delta-since-report check — that is
|
| 99 |
+
advisory and lives only in `is_report_ready`; the report button is always allowed
|
| 100 |
+
to cut a new version (decision 4A). Fails closed (counts as missing analysis) on
|
| 101 |
+
a record-store read error. `record_store` is injectable for tests.
|
| 102 |
"""
|
| 103 |
missing: list[str] = []
|
| 104 |
if not state.problem_validated:
|
|
|
|
| 112 |
substantive = [r for r in records if _has_successful_analysis(r)]
|
| 113 |
except Exception as exc: # noqa: BLE001 — never-throw; fail closed to not-ready
|
| 114 |
logger.warning(
|
| 115 |
+
"report_floor: record store read failed — not ready",
|
| 116 |
analysis_id=analysis_id,
|
| 117 |
error=str(exc),
|
| 118 |
)
|
| 119 |
+
return [*missing, _MISSING_ANALYSIS], []
|
|
|
|
| 120 |
|
| 121 |
if not substantive:
|
| 122 |
missing.append(_MISSING_ANALYSIS)
|
| 123 |
+
return missing, substantive
|
| 124 |
+
|
| 125 |
+
|
| 126 |
+
async def is_report_ready(
|
| 127 |
+
analysis_id: str | None,
|
| 128 |
+
state: AnalysisState,
|
| 129 |
+
*,
|
| 130 |
+
record_store=None,
|
| 131 |
+
report_store=None,
|
| 132 |
+
) -> ReportReadiness:
|
| 133 |
+
"""Return whether a report can be generated for this analysis, and the gaps if not.
|
| 134 |
+
|
| 135 |
+
`record_store` / `report_store` are injectable for tests; they default to the
|
| 136 |
+
real Postgres stores.
|
| 137 |
+
"""
|
| 138 |
+
missing, substantive = await report_floor(
|
| 139 |
+
analysis_id, state, record_store=record_store
|
| 140 |
+
)
|
| 141 |
+
|
| 142 |
+
if not substantive:
|
| 143 |
# No analyses to report on → the delta check is moot.
|
| 144 |
return ReportReadiness(ready=not missing, missing=missing)
|
| 145 |
|
|
@@ -85,6 +85,10 @@ def _chat_cache_key(room_id: str, user_id: str, message: str) -> str:
|
|
| 85 |
# user_id is part of the key so one user's cached answer can never be
|
| 86 |
# replayed to another (R5); room_id stays first so the room-wide clear
|
| 87 |
# endpoint can keep matching on a `chat:{room_id}:*` prefix.
|
|
|
|
|
|
|
|
|
|
|
|
|
| 88 |
return f"{settings.redis_prefix}chat:{room_id}:{user_id}:{message}"
|
| 89 |
|
| 90 |
|
|
|
|
| 85 |
# user_id is part of the key so one user's cached answer can never be
|
| 86 |
# replayed to another (R5); room_id stays first so the room-wide clear
|
| 87 |
# endpoint can keep matching on a `chat:{room_id}:*` prefix.
|
| 88 |
+
# LIMITATION (T-G): the key omits conversation history, so a repeated message
|
| 89 |
+
# replays its cached answer even if the conversation has since moved on. Only
|
| 90 |
+
# the stateless `chat` intent is cached, so the blast radius is small — but a
|
| 91 |
+
# history-aware key (hash of last-N turns) would close it. Flagged to Harry.
|
| 92 |
return f"{settings.redis_prefix}chat:{room_id}:{user_id}:{message}"
|
| 93 |
|
| 94 |
|
|
@@ -9,6 +9,11 @@ Generation reads persisted AnalysisRecords + Problem Statement, makes one LLM ca
|
|
| 9 |
(the executive summary), and persists an immutable versioned artifact. The
|
| 10 |
ReportGenerator + ReportStore are process singletons (the generator caches its LLM
|
| 11 |
chain warm across requests, like ChatHandler).
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 12 |
"""
|
| 13 |
|
| 14 |
from fastapi import APIRouter, HTTPException, Query, status
|
|
@@ -28,21 +33,19 @@ _generator = ReportGenerator()
|
|
| 28 |
_store = ReportStore()
|
| 29 |
|
| 30 |
|
| 31 |
-
async def
|
| 32 |
-
"""
|
| 33 |
-
|
| 34 |
-
The Analysis State stores the PS as free text (the PS skill validates it);
|
| 35 |
-
map it into the report's structured `ProblemStatement.objective`. Never-throw:
|
| 36 |
-
a missing state row (legacy room) or read error degrades to an empty PS so the
|
| 37 |
-
report still generates from its records.
|
| 38 |
-
"""
|
| 39 |
try:
|
| 40 |
from src.agents.state_store import AnalysisStateStore
|
| 41 |
|
| 42 |
-
|
| 43 |
except Exception as e: # noqa: BLE001 — never block report generation on this
|
| 44 |
-
logger.warning("report:
|
| 45 |
-
return
|
|
|
|
|
|
|
|
|
|
|
|
|
| 46 |
if state is None or not state.problem_statement:
|
| 47 |
return ProblemStatement()
|
| 48 |
return ProblemStatement(objective=state.problem_statement)
|
|
@@ -84,11 +87,28 @@ async def generate_report(
|
|
| 84 |
"""Generate, persist, and return a new report version.
|
| 85 |
|
| 86 |
Each call produces a new version (V1, V2, …) that snapshots the records and
|
| 87 |
-
Problem Statement it used. Server-side gate:
|
| 88 |
-
|
|
|
|
|
|
|
| 89 |
"""
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 90 |
try:
|
| 91 |
-
problem_statement =
|
| 92 |
report = await _generator.generate(
|
| 93 |
analysis_id, user_id, problem_statement=problem_statement
|
| 94 |
)
|
|
|
|
| 9 |
(the executive summary), and persists an immutable versioned artifact. The
|
| 10 |
ReportGenerator + ReportStore are process singletons (the generator caches its LLM
|
| 11 |
chain warm across requests, like ChatHandler).
|
| 12 |
+
|
| 13 |
+
Note (T-E): AnalysisRecords are only persisted by the slow path, so reports require
|
| 14 |
+
`ENABLE_SLOW_PATH=on`. With it off, no records exist and generation 409s — by design,
|
| 15 |
+
not a bug. POST gates on the same floor as Help's readiness signal (validated goal +
|
| 16 |
+
≥1 substantive analysis) so the button and Help never disagree.
|
| 17 |
"""
|
| 18 |
|
| 19 |
from fastapi import APIRouter, HTTPException, Query, status
|
|
|
|
| 33 |
_store = ReportStore()
|
| 34 |
|
| 35 |
|
| 36 |
+
async def _load_state(analysis_id: str):
|
| 37 |
+
"""Load the AnalysisState (for the floor gate + problem statement). Never-throw."""
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 38 |
try:
|
| 39 |
from src.agents.state_store import AnalysisStateStore
|
| 40 |
|
| 41 |
+
return await AnalysisStateStore().get(analysis_id)
|
| 42 |
except Exception as e: # noqa: BLE001 — never block report generation on this
|
| 43 |
+
logger.warning("report: state load failed", analysis_id=analysis_id, error=str(e))
|
| 44 |
+
return None
|
| 45 |
+
|
| 46 |
+
|
| 47 |
+
def _problem_statement_from(state) -> ProblemStatement:
|
| 48 |
+
"""Map the analysis's free-text problem statement into the report's structured PS."""
|
| 49 |
if state is None or not state.problem_statement:
|
| 50 |
return ProblemStatement()
|
| 51 |
return ProblemStatement(objective=state.problem_statement)
|
|
|
|
| 87 |
"""Generate, persist, and return a new report version.
|
| 88 |
|
| 89 |
Each call produces a new version (V1, V2, …) that snapshots the records and
|
| 90 |
+
Problem Statement it used. Server-side gate: the report **floor** — a validated
|
| 91 |
+
goal + ≥1 substantive analysis — the same floor Help's readiness signal uses, so
|
| 92 |
+
the button and Help can't disagree (T-D). The delta-since-report check is NOT
|
| 93 |
+
applied here: a new version is always allowed (decision 4A).
|
| 94 |
"""
|
| 95 |
+
from src.agents.gate import stub_analysis_state
|
| 96 |
+
from src.agents.report.readiness import report_floor
|
| 97 |
+
|
| 98 |
+
state = await _load_state(analysis_id)
|
| 99 |
+
floor_missing, _ = await report_floor(
|
| 100 |
+
analysis_id, state or stub_analysis_state(problem_validated=False)
|
| 101 |
+
)
|
| 102 |
+
if floor_missing:
|
| 103 |
+
raise HTTPException(
|
| 104 |
+
status_code=status.HTTP_409_CONFLICT,
|
| 105 |
+
detail="Not ready to generate a report — still needs "
|
| 106 |
+
+ ", ".join(floor_missing)
|
| 107 |
+
+ ".",
|
| 108 |
+
)
|
| 109 |
+
|
| 110 |
try:
|
| 111 |
+
problem_statement = _problem_statement_from(state)
|
| 112 |
report = await _generator.generate(
|
| 113 |
analysis_id, user_id, problem_statement=problem_statement
|
| 114 |
)
|