diff --git a/specs/004-answer-in-user-language/plan.md b/specs/004-answer-in-user-language/plan.md new file mode 100644 index 0000000..93c67cb --- /dev/null +++ b/specs/004-answer-in-user-language/plan.md @@ -0,0 +1,99 @@ +# Implementation Plan: Answer in the User's Language + +**Branch**: `feat/answer-in-user-language` | **Date**: 2026-09-10 | **Spec**: [spec.md](./spec.md) + +## Summary + +Pass `detected_language` to the answer prompt as its own variable, in the two +profiles that ignore it. Nothing else moves. + +The constraint that decides the design: **`input` must stay exactly as it is**, +because `create_retrieval_chain` hands it to the retriever, and `HybridRetriever` +hands it to the query expander. Both must keep seeing the English rephrasing and +nothing else. That is what rules out #140's mechanism and what makes this change +provably free of retrieval risk — the retrieval query is byte-identical before and +after, which a test can assert directly rather than measure statistically. + +## Technical Context + +**Language/Version**: Python 3.12 + +**Primary Dependencies**: `langchain-core` prompts; no new dependency. + +**Testing**: pytest. `bin/retrieval_baseline` is *not* needed — see below. + +**Constraints**: English questions must take exactly today's path (FR-007), with no +added prompt content and no extra model call. + +**Scale/Scope**: Two prompt templates, two call sites, one shared instruction. + +## Constitution Check + +| Article | How this plan satisfies it | +|---|---| +| I — verify the user path | The exit criterion is asking the assembled chain a French question and reading the answer, which is how the bug was found. | +| II — measure, don't argue | Retrieval is unchanged **by construction**: `input` is not touched. A test asserts the retrieval query is byte-identical, which is stronger than a baseline diff and does not spend an hour of API time. | +| III — characterization tests pin behaviour | A test pins that an English question produces no language instruction at all. | +| IV — fail loudly | Nothing to fail: an absent language falls back to today's behaviour, which is correct English output. | +| V — derive from the source of truth | The language comes from `BaseState`, where the detector already put it. Nothing re-detects. | +| VI — bias to doing over filing | Two contributed PRs are resolved rather than left open. | + +**No violations.** + +## Implementation Stages + +### Stage 1 — React-to-Me answers in the detected language + +Add a language instruction to the reactome answer prompt as a template variable, and +pass `state["detected_language"]` at the call site in `generate_answer`. + +The instruction carries #140's nomenclature rule, which is the part of that PR worth +keeping: gene symbols, protein names, pathway names, `R-HSA-*` identifiers and URLs +stay in English. + +**What an English question pays.** No extra model call, and a byte-identical +retrieval query — but it does gain the instruction in its answer prompt, saying to +answer in English. + +An earlier draft of this plan claimed English "costs nothing", which contradicted the +spec's own FR-007 and was simply untrue: a sentence added to every prompt is a +change, however small. FR-007 has been corrected to say what is actually guaranteed. +The cost is one sentence against roughly 3,200 tokens of retrieved context; the +alternative is a branch whose common path only non-English users exercise. + +**Exit criteria**: a French question is answered in French with nomenclature intact; +the retrieval query is byte-identical to today's; an English question still gets a +well-formed English answer. + +### Stage 2 — Plant Reactome, identically + +The same change to `plantreactome/prompt.py` and its call site. It is a separate +stage only because it is a separate deployment and can be verified separately. + +**Exit criteria**: same three, against the plantreactome profile. + +### Stage 3 — Close #125 and #140 + +Close both with credit, saying specifically what each contributed: #140's target and +nomenclature rule, #125's mechanism. Say plainly why #140's mechanism was not taken, +with the measured number — half the retrieved context changes. + +#125's hallucination-grading and web-search work is **not** resolved by this and must +not be described as such; it belongs to #123. + +**Exit criteria**: both closed with credit; the spec records the outcome. + +## Complexity Tracking + +| Decision | Simpler alternative rejected | Why | +|---|---|---| +| A prompt variable | Append to `input`, as #140 does | Measured: about half the fused documents change. The instruction reaches the retriever and the query expander, neither of which should see it. | +| Instruct the generator | Translate the finished answer | D1. An extra model call and its latency on every non-English message, plus translation errors over a scientific answer, to gain a nomenclature protection the prompt already achieves. | +| Always pass the language | Branch on "is it English" | A branch means two paths, one of which is rarely exercised. Passing "English" is the same code doing the same thing. | +| No baseline run | `bin/retrieval_baseline` before and after | The retrieval query is unchanged by construction; a test asserting that is exact, where a baseline diff would only show noise and cost an hour. | + +## Out of Scope + +- Verifying the answer really is in the requested language. +- The corpus, embeddings, or the detector. +- #125's hallucination grader and web search — #123's decision. diff --git a/specs/004-answer-in-user-language/spec.md b/specs/004-answer-in-user-language/spec.md index 286a902..011fccd 100644 --- a/specs/004-answer-in-user-language/spec.md +++ b/specs/004-answer-in-user-language/spec.md @@ -236,8 +236,14 @@ the Reactome deployment is the larger audience. - **FR-005**: Scientific nomenclature — gene symbols, protein names, pathway names, `R-HSA-*` identifiers — MUST NOT be translated. - **FR-006**: URLs and citation links MUST NOT be translated or altered. -- **FR-007**: An English question MUST follow exactly the current path, with no - additional model call and no additional prompt content. +- **FR-007**: An English question MUST cost no additional model call, and MUST reach + the retriever with a byte-identical query. + + It does gain a short instruction in the *answer* prompt, saying to answer in + English. That is a real change for every current user and is stated rather than + hidden. The alternative — branching on "is this English" — creates two paths where + the common one is untested by anyone who only ever asks in English, and the + instruction is a sentence against roughly 3,200 tokens of retrieved context. - **FR-008**: The language MUST reach the answer prompt as its own input, not concatenated into another field. @@ -259,7 +265,8 @@ the Reactome deployment is the larger audience. what the same question produces today. - **SC-004**: In a non-English answer, every gene symbol, pathway name and `R-HSA-*` identifier appears in English, and every Reactome URL resolves. -- **SC-005**: An English question costs the same number of model calls as today. +- **SC-005**: An English question costs the same number of model calls as today, and + its answer remains in English and well-formed. ## Decisions for the team diff --git a/specs/004-answer-in-user-language/tasks.md b/specs/004-answer-in-user-language/tasks.md new file mode 100644 index 0000000..014947d --- /dev/null +++ b/specs/004-answer-in-user-language/tasks.md @@ -0,0 +1,52 @@ +--- +description: "Task list for answering in the user's language" +--- + +# Tasks: Answer in the User's Language + +**Input**: [plan.md](./plan.md), [spec.md](./spec.md) + +**Tests**: Included. FR-003 and FR-007 are both "nothing changed" claims, and an +unasserted claim of that kind is worth nothing. + +## Phase 1: Foundational + +- [x] T001 Add a shared language instruction constant carrying the nomenclature rule (gene symbols, protein names, pathway names, `R-HSA-*`, URLs stay English) in `src/agent/tasks/language_instruction.py`, crediting @bleedblack1 for the wording + +## Phase 2: User Story 1 — React-to-Me answers in the detected language (P1) + +**Independent test**: ask the assembled chain a French question; read the answer. + +- [x] T002 [US1] Add a `{detected_language}` variable and the instruction to `src/retrievers/reactome/prompt.py` +- [x] T003 [US1] Pass `state["detected_language"]` in `generate_answer` in `src/agent/profiles/react_to_me.py`, leaving `input` untouched +- [x] T004 [P] [US1] Test in `tests/agent/test_answer_language.py`: the retrieval query is byte-identical with and without a language (FR-003) +- [x] T005 [P] [US1] Test in `tests/agent/test_answer_language.py`: the prompt receives the language as its own variable, never concatenated into `input` (FR-008) +- [x] T006 [P] [US1] Test in `tests/agent/test_answer_language.py`: an English question adds no extra model call and reaches the retriever with a byte-identical query (FR-007) +- [x] T006a [US1] Ask the real chain an English question and confirm the answer is still English and well-formed — the instruction is new prompt content for every existing user (SC-005) +- [x] T007 [US1] Ask the real chain a French question against the Release95 bundle; confirm the answer is French and gene symbols, `R-HSA-*` IDs and URLs are unchanged (Article I) + +## Phase 3: User Story 3 — Plant Reactome, identically (P2) + +- [x] T008 [US3] Same change to `src/retrievers/plantreactome/prompt.py` and its call site in `src/agent/profiles/plantreactome.py` +- [x] T009 [P] [US3] Test that both profiles use the same shared instruction, so they cannot drift + +## Phase 4: Polish & Cross-Cutting + +- [x] T010 Perturbation check: revert the call site and confirm the French test fails +- [x] T011 Run `ruff check`, `ruff format --check`, `mypy`, `pytest` +- [ ] T012 Close #140 with credit to @bleedblack1 — the target and the nomenclature rule are kept; the mechanism is not, with the measured number +- [ ] T013 Close #125 with credit to @bhavyakeerthi3 for the mechanism, noting its hallucination-grading work belongs to #123 and is untouched +- [ ] T014 Record the outcome in `specs/004-answer-in-user-language/spec.md` + +## Dependencies + +```text +T001 -> Phase 2 (T002-T007) -> Phase 3 (T008-T009) -> Phase 4 +``` + +Phase 3 depends on Phase 2 only for the shared constant's final shape. + +## Implementation Strategy + +**MVP is Phase 2.** React-to-Me is the deployment with users. Phase 3 is the same +change to a second profile and could ship separately. diff --git a/src/agent/profiles/plantreactome.py b/src/agent/profiles/plantreactome.py index 368b337..8ebe7be 100644 --- a/src/agent/profiles/plantreactome.py +++ b/src/agent/profiles/plantreactome.py @@ -80,6 +80,10 @@ async def call_model( result: dict[str, Any] = await self.plantreactome_rag.ainvoke( { "input": state["rephrased_input"], + # A separate variable, never concatenated into `input`: + # create_retrieval_chain passes `input` alone to the retriever, so + # anything folded into it reaches BM25 and the query expander. + "detected_language": state["detected_language"], "chat_history": ( state["chat_history"] if state["chat_history"] diff --git a/src/agent/profiles/react_to_me.py b/src/agent/profiles/react_to_me.py index bad9ef8..345663f 100644 --- a/src/agent/profiles/react_to_me.py +++ b/src/agent/profiles/react_to_me.py @@ -158,6 +158,10 @@ async def generate_answer( result: dict[str, Any] = await rag.ainvoke( { "input": state["rephrased_input"], + # A separate variable, never concatenated into `input`: + # create_retrieval_chain passes `input` alone to the retriever, so + # anything folded into it reaches BM25 and the query expander. + "detected_language": state["detected_language"], "chat_history": ( state["chat_history"] if state["chat_history"] diff --git a/src/agent/tasks/language_instruction.py b/src/agent/tasks/language_instruction.py new file mode 100644 index 0000000..38a6ec9 --- /dev/null +++ b/src/agent/tasks/language_instruction.py @@ -0,0 +1,26 @@ +"""The instruction that makes an answer come back in the user's language. + +Shared by every answer prompt so the two deployments cannot drift apart. + +The wording is @bleedblack1's from #140, which got the hard part right: the +retrieved context is English and must stay English, but scientific nomenclature +inside the answer must not be translated either. `SET`, `MAX` and `CAT` are gene +symbols and also ordinary English words, and `R-HSA-9612973` means nothing +translated. + +What is not taken from #140 is where it put this. That PR appends it to `input`, +which `create_retrieval_chain` hands straight to the retriever -- verified in +`langchain_classic`: `retrieval_docs = (lambda x: x["input"]) | retriever`. Measured +through the whole retriever, that changes about half the fused documents. Here it is +a separate prompt variable, so `input` is untouched and retrieval, including the +query expansion in front of it, is byte-identical. +""" + +LANGUAGE_INSTRUCTION = """Answer in {detected_language}. + +The context you were given is in English because the Reactome knowledgebase is +English-only. Your answer must still be in {detected_language}. + +Keep gene symbols, protein names, pathway names, Reactome identifiers (R-HSA-...) +and URLs exactly as they appear in the context. Do not translate them, even when a +symbol is also an ordinary word.""" diff --git a/src/retrievers/plantreactome/prompt.py b/src/retrievers/plantreactome/prompt.py index 50fcd1f..786d510 100644 --- a/src/retrievers/plantreactome/prompt.py +++ b/src/retrievers/plantreactome/prompt.py @@ -1,5 +1,7 @@ from langchain_core.prompts import ChatPromptTemplate, MessagesPlaceholder +from agent.tasks.language_instruction import LANGUAGE_INSTRUCTION + plantreactome_system_prompt = """ You are an expert in molecular biology with access to the **Plant Reactome Knowledgebase**. Your primary responsibility is to answer the user's questions **comprehensively, mechanistically, and with precision**, drawing strictly from the **Plant Reactome Knowledgebase**. @@ -35,6 +37,7 @@ [ ("system", plantreactome_system_prompt), MessagesPlaceholder(variable_name="chat_history"), + ("system", LANGUAGE_INSTRUCTION), ("user", "Context:\n{context}\n\nQuestion: {input}"), ] ) diff --git a/src/retrievers/reactome/prompt.py b/src/retrievers/reactome/prompt.py index d570cb9..d348939 100644 --- a/src/retrievers/reactome/prompt.py +++ b/src/retrievers/reactome/prompt.py @@ -1,5 +1,7 @@ from langchain_core.prompts import ChatPromptTemplate, MessagesPlaceholder +from agent.tasks.language_instruction import LANGUAGE_INSTRUCTION + reactome_system_prompt = """ You are an expert in molecular biology with access to the **Reactome Knowledgebase**. Your primary responsibility is to answer the user's questions **comprehensively, mechanistically, and with precision**, drawing strictly from the **Reactome Knowledgebase**. @@ -35,6 +37,7 @@ [ ("system", reactome_system_prompt), MessagesPlaceholder(variable_name="chat_history"), + ("system", LANGUAGE_INSTRUCTION), ("user", "Context:\n{context}\n\nQuestion: {input}"), ] ) diff --git a/tests/agent/test_answer_language.py b/tests/agent/test_answer_language.py new file mode 100644 index 0000000..d838ed6 --- /dev/null +++ b/tests/agent/test_answer_language.py @@ -0,0 +1,116 @@ +"""The language reaches the answer prompt and nothing else. + +The whole design rests on one property: `create_retrieval_chain` passes only +`input` to the retriever -- + + retrieval_docs = (lambda x: x["input"]) | retriever + +-- so a language instruction placed anywhere else cannot affect what is retrieved. +#140 placed it inside `input`, and measured through the whole retriever that changed +about half the fused documents. These tests pin the property that makes this +approach different. +""" + +from pathlib import Path +from typing import Any + +import pytest + +pytest.importorskip("langchain", reason="retrieval stack not installed") + +from langchain_core.documents import Document # noqa: E402 +from langchain_core.retrievers import BaseRetriever # noqa: E402 + +from agent.tasks.language_instruction import LANGUAGE_INSTRUCTION # noqa: E402 + +REPO_ROOT = Path(__file__).parent.parent.parent + + +class _RecordingRetriever(BaseRetriever): + """Captures exactly what the chain asks it to retrieve.""" + + seen: list[str] + + def _get_relevant_documents(self, query: str, **kwargs: Any) -> list[Document]: + self.seen.append(query) + return [Document(page_content="TP53 induces apoptosis via BAX.")] + + +def _retrieval_query(**extra: Any) -> str: + """Build the real chain over a recording retriever and return the query it saw.""" + from langchain_classic.chains.combine_documents import ( + create_stuff_documents_chain, + ) + from langchain_classic.chains.retrieval import create_retrieval_chain + from langchain_core.language_models.fake_chat_models import FakeListChatModel + + from retrievers.reactome.prompt import reactome_qa_prompt + + retriever = _RecordingRetriever(seen=[]) + chain = create_retrieval_chain( + retriever=retriever, + combine_docs_chain=create_stuff_documents_chain( + llm=FakeListChatModel(responses=["an answer"]), prompt=reactome_qa_prompt + ), + ) + chain.invoke({"input": "What role does TP53 play in apoptosis?", **extra}) + return retriever.seen[0] + + +def test_the_language_never_reaches_the_retriever() -> None: + """FR-003. The property the whole approach depends on.""" + with_language = _retrieval_query(detected_language="French", chat_history=[]) + + assert with_language == "What role does TP53 play in apoptosis?" + assert "French" not in with_language + assert "CRITICAL" not in with_language, "no instruction prose in the query" + + +def test_the_retrieval_query_is_identical_whatever_the_language() -> None: + """FR-007: byte-identical, not merely similar. + + Stronger than a retrieval baseline and free: if the query cannot differ, the + documents cannot differ, so there is nothing to measure statistically. + """ + queries = { + _retrieval_query(detected_language=lang, chat_history=[]) + for lang in ("English", "French", "Japanese", "German") + } + assert len(queries) == 1, f"the retrieval query varied by language: {queries}" + + +def test_the_instruction_lives_in_the_prompt_not_the_input() -> None: + """FR-008. The difference between this and #140, asserted structurally.""" + for name in ("react_to_me", "plantreactome"): + source = (REPO_ROOT / "src" / "agent" / "profiles" / f"{name}.py").read_text() + assert '"detected_language": state["detected_language"],' in source, name + assert ( + 'state["rephrased_input"],' in source + ), f"{name} must pass the rephrasing alone as input" + + +def test_the_instruction_protects_scientific_nomenclature() -> None: + """#140's contribution, and the reason a naive translation is wrong. + + SET, MAX and CAT are gene symbols and ordinary English words; R-HSA-9612973 + means nothing translated. + """ + text = LANGUAGE_INSTRUCTION.lower() + for term in ("gene symbols", "protein names", "pathway names", "r-hsa", "url"): + assert term in text, f"the instruction must protect {term}" + assert "do not translate" in text + + +def test_both_profiles_share_one_instruction() -> None: + """Two deployments, one wording -- they cannot drift apart. + + Matched on the instruction's distinctive phrase rather than on "R-HSA", which + appears legitimately in both prompts as a citation example. A cruder check + passed on nothing and failed on that. + """ + for name in ("reactome", "plantreactome"): + source = (REPO_ROOT / "src" / "retrievers" / name / "prompt.py").read_text() + assert "LANGUAGE_INSTRUCTION" in source, name + assert ( + "Do not translate them" not in source + ), f"{name} inlines its own copy of the instruction instead of sharing it"