Checkpoint frontend UI/UX overhaul and ingestion embed benchmark work
This commit is contained in:
@@ -0,0 +1,321 @@
|
||||
# Code-only review: current RAG runtime and prompts — 2026-08-06
|
||||
|
||||
## Scope
|
||||
|
||||
This review is based on the implementation currently present in the working
|
||||
tree, not on claims or completion status in planning/progress Markdown files.
|
||||
No application, ingestion, prompt, test, or infrastructure code was changed.
|
||||
|
||||
Reviewed paths:
|
||||
|
||||
- `apps/ai-service/{bootstrap.py,config.py,routers/rag.py}`
|
||||
- `apps/ai-service/rag/{routing,service,answer,grounding,prompt,conversation,conversational,reasoning,understanding,agent}.py`
|
||||
- `apps/ai-service/adapters/{qdrant,embedding,bedrock_converse,bedrock_claude,postgres}.py`
|
||||
- `apps/web/app/api/chat/route.ts`
|
||||
- relevant AI-service and ingestion tests
|
||||
|
||||
Checks run:
|
||||
|
||||
- `cd ingestion && python -m pytest -q`
|
||||
- observed: `296 passed`, 142 deprecation warnings, 57.71 s
|
||||
- `cd apps/ai-service && python -m pytest -q`
|
||||
- observed: `118 passed, 3 skipped`, 15.30 s
|
||||
- local, no-network probes of `grounding.verify`, conversation overflow, and
|
||||
the real 684-drug `CatalogDrugResolver`
|
||||
|
||||
## Verdict
|
||||
|
||||
The ingestion/index boundary has several strong safety properties: schema
|
||||
validation, deterministic point ids, provenance, section-filtered retrieval,
|
||||
whole-section paging/order, and quarantine of visually uncertain blocks.
|
||||
|
||||
The answer-time RAG is not yet safe as a medical release. The highest risks are
|
||||
above vector retrieval: client-controlled policy labels, a grounding verifier
|
||||
that does not verify clinical claims, two competing orchestration paths, and a
|
||||
new LLM understander whose catalog whitelist does not guarantee correct entity
|
||||
resolution.
|
||||
|
||||
## Positive implementation findings
|
||||
|
||||
1. Qdrant section retrieval scrolls all pages and sorts by `part_index`; it does
|
||||
not silently treat a top-k subset as a complete contraindication/dose section.
|
||||
2. Loader validation rejects unknown schema versions and missing physical or
|
||||
printed-page ranges.
|
||||
3. Point ids derive from `chunk_id`, making repeated loads idempotent.
|
||||
4. Table/formula attachments retain page, block id, bbox and crop metadata;
|
||||
`VERIFY_PDF` evidence is not passed to generation.
|
||||
5. Domain modules depend on protocols rather than importing Bedrock or Qdrant
|
||||
SDKs directly.
|
||||
6. Numeric verification preserves decimal separators exactly, correctly
|
||||
rejecting conversions such as `2 g` to `2000 mg`.
|
||||
|
||||
These are useful controls, but they do not compensate for the runtime findings
|
||||
below.
|
||||
|
||||
## Findings
|
||||
|
||||
### F-01 — Critical — `grounding.verify` does not verify claim-to-evidence
|
||||
|
||||
`rag/grounding.py::verify` builds one global set of numeric tokens from all
|
||||
evidence. An answer passes when every answer number occurs somewhere in that
|
||||
set and every citation index is in range. It does not require a citation, does
|
||||
not bind a number to the cited block, and does not check nonnumeric clinical
|
||||
claims.
|
||||
|
||||
Observed local probes:
|
||||
|
||||
```text
|
||||
claim_bia:
|
||||
answer = "Metformin chữa ung thư [1]."
|
||||
evidence = "Metformin dùng điều trị đái tháo đường."
|
||||
result = grounded=True
|
||||
|
||||
so_sai_nguon:
|
||||
answer = "Liều 500 mg [1]."
|
||||
evidence 1 = "Không dùng khi suy thận."
|
||||
evidence 2 = "Liều 500 mg mỗi ngày."
|
||||
result = grounded=True
|
||||
|
||||
khong_citation:
|
||||
answer = "Liều 500 mg."
|
||||
evidence = "Liều 500 mg mỗi ngày."
|
||||
result = grounded=True
|
||||
```
|
||||
|
||||
`GroundedAnswerService` then falls back to returning all retrieved citation
|
||||
cards when generated text cites none. That can make an unsupported statement
|
||||
look sourced.
|
||||
|
||||
Required correction:
|
||||
|
||||
- require at least one valid citation for every generated clinical sentence;
|
||||
- validate numeric tokens against the blocks actually cited by that sentence,
|
||||
not the union of all evidence;
|
||||
- add a claim-to-evidence/entailment check or restrict high-risk answers to
|
||||
extractive spans;
|
||||
- reject rather than attach all sources when generated text has no citations.
|
||||
|
||||
### F-02 — Critical — medical scope and intent are controlled by the client
|
||||
|
||||
The web BFF sends every query as:
|
||||
|
||||
```json
|
||||
{"subject_scope":"human","intent":"fact_lookup"}
|
||||
```
|
||||
|
||||
The FastAPI request model accepts these values and `QueryRoutingService` uses
|
||||
them as the policy gate. Therefore recommendation or out-of-scope wording is
|
||||
not independently detected by the server. The product is human-only; the
|
||||
correct behavior for any other scope is refusal, but a client label must not be
|
||||
the mechanism that enforces that boundary.
|
||||
|
||||
Required correction: derive and enforce policy server-side. Client labels may
|
||||
be hints or authenticated metadata, never the sole safety decision.
|
||||
|
||||
### F-03 — High — two incompatible RAG front ends coexist
|
||||
|
||||
`rag/understanding.py` and `rag/agent.py` implement the new framed path, but
|
||||
`bootstrap.py` still constructs `CatalogDrugResolver`, `QueryRoutingService`
|
||||
and `ConversationalLoopService`. `routers/rag.py` still calls the old answer
|
||||
service. No current test imports `RagAgent`, `LlmQueryUnderstander`,
|
||||
`QueryFrame`, or `retrieve_framed`.
|
||||
|
||||
The current resolver was probed against the real alias artifact:
|
||||
|
||||
```text
|
||||
aspirinol -> acid_acetylsalicylic_aspirin, score=0.875, resolved
|
||||
amoxicillin -> amoxicilin, score=0.95238, resolved
|
||||
warfarin + aspirin -> ambiguous
|
||||
```
|
||||
|
||||
The conversational path only accepts exact resolver matches, while the
|
||||
single-turn path accepts threshold-fuzzy matches. Safety therefore changes
|
||||
depending on whether `conversation_id` is supplied.
|
||||
|
||||
Required correction: select one orchestrator, expose one request contract,
|
||||
wire it into bootstrap/router, and remove the obsolete path after parity tests.
|
||||
|
||||
### F-04 — High — the LLM catalog whitelist does not guarantee drug identity
|
||||
|
||||
The new understander validates that returned `drug_id` values exist in the
|
||||
684-drug catalog. This guarantees only that the output id is syntactically
|
||||
valid. It does not prove that the id is supported by the user's text. An LLM
|
||||
can still map an invented or unrelated name to any real catalog id while
|
||||
obeying the output whitelist.
|
||||
|
||||
The prompt tells the model to put unknown names in `unknown_drugs`, but
|
||||
`_parse()` has no independent text-to-alias validation of that decision.
|
||||
Consequently, “only catalog ids are accepted” must not be described as a
|
||||
structural prevention of fake-name substitution.
|
||||
|
||||
Required correction: deterministically generate/validate candidate entities
|
||||
from verified aliases and spelling rules, then let the LLM disambiguate only
|
||||
within that bounded candidate set. Unknown-vs-known must have adversarial
|
||||
regression cases and a fail-closed path.
|
||||
|
||||
### F-05 — High — runtime does not validate collection/corpus/model identity
|
||||
|
||||
The ingestion loader writes a sidecar manifest containing corpus SHA, model id,
|
||||
dimensions and input kind. AI-service startup does not read it. It configures a
|
||||
Cohere query embedder and checks only vector dimensions at query time.
|
||||
|
||||
Two unrelated embedding models can both produce 1024-dimensional vectors;
|
||||
Qdrant will return plausible-looking but meaningless results without an error.
|
||||
A stale corpus collection is likewise accepted.
|
||||
|
||||
Required correction: startup/readiness must compare the sidecar manifest with
|
||||
the configured query model, dimensions, input kind, expected corpus version and
|
||||
point count; mismatch must keep the service unready.
|
||||
|
||||
### F-06 — High — conversation overflow is discarded before summarisation
|
||||
|
||||
`ConversationState.append()` truncates `recent` to the configured window.
|
||||
`overflow()` subsequently checks whether the already-truncated tuple exceeds
|
||||
that same window, which can never occur.
|
||||
|
||||
Observed probe after eight appended turns:
|
||||
|
||||
```text
|
||||
recent=6, turn_count=8, overflow=0
|
||||
```
|
||||
|
||||
The summary path therefore receives no dropped turns. Production bootstrap
|
||||
also uses `InMemoryConversationStore`, so restart loses state and multiple
|
||||
workers can hold different histories for the same conversation id.
|
||||
|
||||
Required correction: capture evicted turns before truncation or return them
|
||||
from append; persist structured focus/history in a shared store before using
|
||||
multiple workers.
|
||||
|
||||
### F-07 — High — intended RAG capabilities are not connected end-to-end
|
||||
|
||||
In the new agent, `dosing_calc` falls through to ordinary single-drug
|
||||
retrieval; the tested calculator is not called. `symptom_to_drug` reports that
|
||||
reverse lookup is not ready. The interaction branch exists only in the new
|
||||
agent, which is not wired. The live `ConversationalLoopService` calls the
|
||||
answer engine directly rather than running the bounded reasoning loop.
|
||||
|
||||
Required correction: each `turn_type` needs an explicit, tested node and an
|
||||
end-to-end API test proving that the node is reached. Do not expose a turn type
|
||||
until its execution path exists.
|
||||
|
||||
### F-08 — Medium/High — provider/call budgets do not bound the live request
|
||||
|
||||
`TurnBudget` declares a 20-second wall-clock limit, but the live route does not
|
||||
thread it through provider calls. One request may make a sufficiency call and a
|
||||
generation call; the Converse adapter allows a 60-second read timeout per call
|
||||
and retries. Qdrant and PostgreSQL add independent waits.
|
||||
|
||||
Required correction: enforce an end-to-end request deadline and pass remaining
|
||||
time to every dependency. A budget object that is not on the production path is
|
||||
documentation, not a limit.
|
||||
|
||||
### F-09 — Medium — trace persistence is a response dependency
|
||||
|
||||
After producing an answer, the router synchronously opens a new PostgreSQL
|
||||
connection and inserts the trace. A trace database outage raises before the API
|
||||
response is returned, discarding an otherwise valid safe answer. There is no
|
||||
connection pool or explicit bounded trace failure policy.
|
||||
|
||||
Required correction: decide explicitly whether tracing is fail-open or
|
||||
fail-closed, pool connections, bound the operation, and test database outage.
|
||||
|
||||
### F-10 — High — green unit tests do not exercise the pending production path
|
||||
|
||||
The AI suite passes 118 tests but skips three integration tests unless
|
||||
`RUN_INTEGRATION=1`. There are no tests referencing the new understander/agent.
|
||||
The existing evaluation runner constructs an in-memory lexical retriever and
|
||||
the old resolver rather than executing the same dependency graph as the live
|
||||
HTTP service.
|
||||
|
||||
Required correction: create a release suite that drives the production
|
||||
orchestrator from raw user turn to response, with fixed Qdrant fixtures or a
|
||||
known test collection, and asserts drug id, section, evidence ids, decision,
|
||||
citations and claim grounding.
|
||||
|
||||
## Prompt review
|
||||
|
||||
### What is good
|
||||
|
||||
The answer prompt clearly says the model is not a knowledge source, forbids
|
||||
outside medical knowledge, requires exact copying of numeric strings, preserves
|
||||
population/route conditions, requires per-claim citations, and asks for
|
||||
clarification rather than listing multiple dose bands. The answer JSON envelope
|
||||
is parsed fail-closed. The Anthropic adapter can request a server-enforced JSON
|
||||
schema; the Converse adapter compensates with JSON isolation and downstream
|
||||
parsing.
|
||||
|
||||
### Prompt/runtime mismatches
|
||||
|
||||
1. Prompt rules are stronger than the verifier. Per-claim citations and correct
|
||||
population association are requested but not enforced in code (F-01).
|
||||
2. `_check_sufficiency()` skips whenever there is fewer than two evidence
|
||||
blocks. One hydrated parent/section can contain many paediatric, renal or
|
||||
indication-specific dose bands, so evidence count is not a valid proxy for
|
||||
ambiguity.
|
||||
3. When the model returns `evidence_sufficient=false` without a clarification,
|
||||
`_generate()` returns no answer and the service falls back to the entire
|
||||
extractive section. This can do exactly what the prompt forbids: dump several
|
||||
dose bands and leave the reader to choose.
|
||||
4. The prompt does not explicitly delimit untrusted user instructions or state
|
||||
that instructions appearing inside the question/evidence are data, not
|
||||
commands. Prompt injection alone would be less serious with a strong claim
|
||||
verifier; with F-01 it can produce unsupported nonnumeric claims that pass.
|
||||
5. `FRAME_SCHEMA` in the new understander is descriptive rather than a complete
|
||||
JSON Schema, and Converse cannot enforce it server-side. Parsing validates
|
||||
enumerated drug ids/section keys but not cross-field coherence such as
|
||||
`dosing_calc` without required inputs or an interaction with fewer than two
|
||||
drugs.
|
||||
6. The understander sends the full catalog on every turn. Before release, token
|
||||
count, latency and truncation behavior must be measured; a deterministic
|
||||
candidate shortlist would be safer and cheaper.
|
||||
7. The answer prompt hard-codes the audience as doctors and pharmacists. This
|
||||
is appropriate only if the UI/product access policy matches that audience;
|
||||
“human medicine” and “professional user” are different constraints.
|
||||
|
||||
## Required regression cases before release
|
||||
|
||||
At minimum, add cases for:
|
||||
|
||||
- fabricated nonnumeric clinical statement with a valid citation;
|
||||
- correct number copied from the wrong evidence block/citation;
|
||||
- generated clinical answer with no citation;
|
||||
- evidence-insufficient response that must refuse/clarify, never dump dosing;
|
||||
- one evidence block containing several population-specific doses;
|
||||
- invented drug near a real alias (`aspirinol`) and unrelated invented names;
|
||||
- two-drug interaction with both ids and both interaction sections;
|
||||
- follow-up after the recent window and after process restart;
|
||||
- wrong embedding model with the same vector dimension;
|
||||
- PostgreSQL/Qdrant/Bedrock timeout and outage behavior;
|
||||
- prompt injection attempts in the user question;
|
||||
- raw HTTP requests with and without `conversation_id` producing the same
|
||||
safety decision.
|
||||
|
||||
## Recommended correction order
|
||||
|
||||
1. Fix F-01 and add adversarial grounding tests; until then, do not label
|
||||
generated answers as claim-grounded.
|
||||
2. Move scope/intent enforcement to the server (F-02).
|
||||
3. Choose and wire one RAG orchestrator, then delete or quarantine the other
|
||||
path (F-03/F-07).
|
||||
4. Bound entity candidates deterministically before LLM disambiguation (F-04).
|
||||
5. Validate runtime manifest/readiness (F-05).
|
||||
6. Fix and persist conversation state (F-06).
|
||||
7. Enforce end-to-end budgets and datastore failure policies (F-08/F-09).
|
||||
8. Run a production-path golden/regression suite and publish failures by
|
||||
severity (F-10).
|
||||
|
||||
## Release gate proposed by this review
|
||||
|
||||
Do not release generated clinical prose until all of the following reproduce:
|
||||
|
||||
- unsupported clinical claims, wrong-source numbers and missing citations are
|
||||
rejected;
|
||||
- raw user text reaches one server-owned understanding/policy path;
|
||||
- fake/unknown drug names cannot be silently substituted with a real drug;
|
||||
- runtime refuses an incompatible corpus/model manifest;
|
||||
- every answer/clarification can be traced to the exact orchestrator branch and
|
||||
evidence ids;
|
||||
- the same end-to-end suite passes with and without conversation state;
|
||||
- integration tests run, rather than skip, in CI/staging.
|
||||
|
||||
Reference in New Issue
Block a user