duocthu/coordination/CODEX_RAG_CODE_REVIEW_2026-08-06.md

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:

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:

{"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:

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:

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.