From 0780086da15ae82f071e429c9555458495f4d481 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 18 Sep 2026 06:04:53 +0000 Subject: [PATCH] Cite userguide answers by url, so they stop having no sources at all Adam's call, 2026-09-18. A userguide-routed question returned zero citations -- "How do I use the pathway browser?" gave 0 citations and 452 tokens -- because a citation carries `st_id` and the userguide collection indexes documentation pages whose identity is a URL. It is also the fastest class of answer, so the gap was common rather than exotic. A citation now carries exactly one identifier: `st_id` for Reactome and disease-variant sources, `url` for userguide pages, with the other key absent rather than empty so a consumer that understands only `st_id` keeps working by skipping what it does not recognise. No fabricated stable ids. A made-up `R-` would resolve to nothing or to a real but wrong entity, and a reader cannot tell from the link text. Deduplicated by page rather than chunk: the bundle is 98 chunks across 10 pages, so chunk-level citations would repeat one page up to 27 times. The page title is the label. **Only http(s) sources are cited, and that guard was not in the first version.** `source` is a generic LangChain field the CSV loaders set to the file they read, so a Reactome document lacking an `st_id` cited "/home/awright/git/reactome_chatbot/embeddings/openai/..." -- a server path, to a public website. The unit tests used fixtures too clean to contain one; it showed up the first time a real question ran. Measured after: pathway-browser 0 -> 4 citations, GSEA 0 -> 2, and Reactome questions unchanged at 12 with no paths among them. Co-Authored-By: Claude Opus 5 --- .../contracts/answer_endpoint.md | 42 ++++++--- specs/010-search-page-answers/tasks.md | 2 +- src/agent/graph.py | 61 ++++++++++-- src/api/answer.py | 15 ++- tests/agent/test_citation_sources.py | 94 +++++++++++++++++++ 5 files changed, 189 insertions(+), 25 deletions(-) create mode 100644 tests/agent/test_citation_sources.py diff --git a/specs/010-search-page-answers/contracts/answer_endpoint.md b/specs/010-search-page-answers/contracts/answer_endpoint.md index 8735bc7..e4e58ee 100644 --- a/specs/010-search-page-answers/contracts/answer_endpoint.md +++ b/specs/010-search-page-answers/contracts/answer_endpoint.md @@ -99,23 +99,41 @@ are gated on that same event, because retrieval completing is how the answer's tokens are told apart from the query expander's. So sources are complete at the moment the first token arrives, and a panel may render them before any prose. -#### Open: citing userguide pages (not scheduled) +#### Userguide pages are cited by `url` (implemented 2026-09-18) -Proposed by the website session 2026-09-18 and agreed in shape, not scheduled. -A documentation URL does not fit `{st_id, display_name}`, and **a fabricated -`R-` identifier is not an option** -- it would resolve to nothing, or worse to -the wrong entity. The shape that works is an optional sibling, exactly one of the -two present: +A citation carries **exactly one** identifier, and the other key is absent rather +than empty: ``` -{"st_id": "R-HSA-8862803", "display_name": "..."} # unchanged -{"url": "/userguide/pathway-browser", "display_name": "..."} # new +event: citation +data: {"st_id": "R-HSA-8862803", "display_name": "Deregulated CDK5 triggers..."} + +event: citation +data: {"url": "https://reactome.org/userguide/pathway-browser", + "display_name": "The Pathway Browser"} ``` -A consumer that understands only `st_id` keeps working by skipping what it does -not recognise. Worth doing when this contract is next opened; an answer that is -honest about having no sources is much better than an invented one, so this is -not a blocker. +`st_id` resolves at `reactome.org/content/detail/`; `url` is absolute and +used verbatim. A consumer that understands only `st_id` keeps working by skipping +what it does not recognise. + +**No fabricated stable ids.** A made-up `R-` identifier would resolve to nothing +or, worse, to a real but wrong entity, and a reader cannot tell from the link +text. A userguide page is cited as what it is. + +Measured after the change: "How do I use the pathway browser?" went from 0 +citations to 4, and "How do I run a GSEA in Reactome?" to 2. Reactome questions +are unchanged. + +Deduplicated by page, not by chunk -- the userguide bundle is 98 chunks across 10 +pages, so chunk-level citations would repeat one page up to 27 times. The label is +the page title. + +**Only `https://` and `http://` sources are cited.** `source` is a generic +LangChain metadata field and the CSV loaders set it to the file they read, so +without that check a Reactome document lacking an `st_id` cited a local +filesystem path. That was caught by running a real question, not by the unit +tests, which used fixtures too clean to contain one. ### What `token` text contains diff --git a/specs/010-search-page-answers/tasks.md b/specs/010-search-page-answers/tasks.md index 687c9a3..0f4b154 100644 --- a/specs/010-search-page-answers/tasks.md +++ b/specs/010-search-page-answers/tasks.md @@ -47,7 +47,7 @@ state. - [x] T025 [US2] Enforce `aud` on the caller token (asked for by the website, D1); the code refused every token carrying one, since PyJWT rejects `aud` when no audience is expected (PR #240) - [x] T026 Rename human_token -> caller_token everywhere; D1 established the token asserts caller identity, not humanity (PR #240) - [x] T027 Answer the website's cancellation question: a client hang-up raises CancelledError inside the answer generator and produces nothing further, so they need not cancel upstream (PR #240) -- [ ] T028 Decide whether userguide answers cite their pages. Needs an optional `url` sibling to `st_id` in the citation event -- agreed in shape with the website, not scheduled. A fabricated stable id is not an option +- [x] T028 Cite userguide answers by `url` (Adam decided yes, 2026-09-18). Optional `url` sibling to `st_id`, exactly one present; no fabricated stable ids; http(s) only, after a local filesystem path leaked through the first version. - [ ] T020a Decide what to do about non-reproducible retrieval: three runs of one question shared only 4 of 19 citations (Jaccard 0.26) because query expansion is itself a model call. Affects what FR-007 can cache - [x] T018 [US2] Return `state: failed` with no partial answer on any internal error, so the page renders no panel (FR-006) - [x] T011a [US1] Strip inline HTML anchors from the token stream in src/util/anchor_strip.py; the contract promises prose without them and the chat prompt emits them, split across ~20 fragments (PR #236) diff --git a/src/agent/graph.py b/src/agent/graph.py index cc09dd3..4417a55 100644 --- a/src/agent/graph.py +++ b/src/agent/graph.py @@ -6,6 +6,7 @@ from typing import Any, Literal, cast from langchain_core.callbacks.base import Callbacks +from langchain_core.documents import Document from langchain_core.embeddings import Embeddings from langchain_core.language_models.chat_models import BaseChatModel from langchain_core.runnables import RunnableConfig @@ -195,6 +196,50 @@ def resolve_llm_model(llm_config: "LLMConfig | None") -> tuple[str, str, str | N MAX_CITATIONS = 12 + + +def _citation_for(document: "Document") -> "AnswerEvent | None": + """One source, from whichever identifier the collection actually carries. + + Reactome and disease-variant documents have `st_id`, a stable identifier the + caller resolves to a detail page. Userguide documents do not and never will: + they are documentation pages, and their identity is the page URL. + + Until 2026-09-18 that meant a userguide-routed question returned **no + citations at all** -- a good answer with no sources, and the fastest answers + at that. Rather than mint a fake `R-` id, which would resolve to nothing or + to the wrong entity, a userguide source is cited by `url`. + + Exactly one of `st_id` and `url` is set, so a caller that understands only + `st_id` keeps working by skipping what it does not recognise. + """ + metadata = document.metadata + stable_id = metadata.get("st_id") + if stable_id: + return AnswerEvent( + kind="citation", + st_id=str(stable_id), + display_name=str(metadata.get("display_name") or ""), + ) + source = str(metadata.get("source") or "") + # Only a real web URL. `source` is a generic LangChain field, and the CSV + # loaders set it to the file they read -- so without this guard a Reactome + # document without an `st_id` cited + # "/home/awright/git/reactome_chatbot/embeddings/openai/...", leaking a server + # path to the website. Caught by running a real question; the unit tests used + # clean fixtures and never saw it. + if source.startswith(("https://", "http://")): + # Deduplicated by URL rather than by chunk: the userguide bundle is 98 + # chunks across 10 pages, so chunk-level citations would repeat the same + # page up to 27 times. The page title is the right label for a page URL. + return AnswerEvent( + kind="citation", + url=str(source), + display_name=str(metadata.get("page_title") or ""), + ) + return None + + """How many sources one answer may cite. Without a cap this emitted **315** for one question. `HybridRetriever` queries @@ -224,6 +269,7 @@ class AnswerEvent: kind: Literal["token", "citation", "done"] text: str = "" st_id: str = "" + url: str = "" display_name: str = "" state: Literal["answered", "nothing_found", "refused", "failed"] | None = None @@ -385,15 +431,14 @@ async def astream_answer( for document in event["data"].get("output") or []: if len(seen_citations) >= MAX_CITATIONS: break - stable_id = document.metadata.get("st_id") - if not stable_id or stable_id in seen_citations: + citation = _citation_for(document) + if citation is None: + continue + key = citation.st_id or citation.url + if key in seen_citations: continue - seen_citations.add(stable_id) - yield AnswerEvent( - kind="citation", - st_id=str(stable_id), - display_name=str(document.metadata.get("display_name") or ""), - ) + seen_citations.add(key) + yield citation continue if kind != "on_chat_model_stream" or not retrieval_done: diff --git a/src/api/answer.py b/src/api/answer.py index 2c12a5f..af50801 100644 --- a/src/api/answer.py +++ b/src/api/answer.py @@ -144,12 +144,19 @@ async def stream() -> AsyncIterator[str]: tokens_sent += 1 yield _sse("token", {"text": text}) elif event.kind == "citation": + # Exactly one identifier, never both and never an empty + # one: a Reactome source carries `st_id`, a userguide + # page carries `url`. A caller that understands only + # `st_id` skips what it does not recognise, which is why + # the absent key is omitted rather than sent as "". + identifier = ( + {"st_id": event.st_id} + if event.st_id + else {"url": event.url} + ) yield _sse( "citation", - { - "st_id": event.st_id, - "display_name": event.display_name, - }, + {**identifier, "display_name": event.display_name}, ) elif event.kind == "done": state = event.state or "failed" diff --git a/tests/agent/test_citation_sources.py b/tests/agent/test_citation_sources.py new file mode 100644 index 0000000..84125fe --- /dev/null +++ b/tests/agent/test_citation_sources.py @@ -0,0 +1,94 @@ +"""Citations come from whichever identifier a collection actually carries. + +Reactome documents have `st_id`. Userguide documents never will -- they are +documentation pages whose identity is a URL -- and until 2026-09-18 that meant a +userguide-routed question returned no citations at all. Measured then: "How do I +use the pathway browser?" gave 0 citations and 452 tokens. +""" + +from langchain_core.documents import Document + +from agent.graph import _citation_for + + +def _doc(**metadata: str) -> Document: + return Document(page_content="text", metadata=dict(metadata)) + + +def test_a_reactome_document_is_cited_by_stable_id() -> None: + citation = _citation_for(_doc(st_id="R-HSA-8863013", display_name="CDK5 binds p25")) + assert citation is not None + assert (citation.st_id, citation.display_name) == ( + "R-HSA-8863013", + "CDK5 binds p25", + ) + assert citation.url == "", "a Reactome citation must not also carry a url" + + +def test_a_userguide_document_is_cited_by_url() -> None: + citation = _citation_for( + _doc( + source="https://reactome.org/userguide/pathway-browser", + page_title="Pathway Browser", + section_title="Introduction", + ) + ) + assert citation is not None + assert citation.url == "https://reactome.org/userguide/pathway-browser" + assert citation.display_name == "Pathway Browser" + assert citation.st_id == "", "a userguide citation must not carry a stable id" + + +def test_a_stable_id_wins_when_a_document_somehow_has_both() -> None: + """No document should, but the caller's contract is one identifier.""" + citation = _citation_for( + _doc(st_id="R-HSA-1", display_name="Real", source="https://example.invalid") + ) + assert citation is not None + assert citation.st_id == "R-HSA-1" + assert citation.url == "" + + +def test_a_document_with_neither_is_not_cited() -> None: + """Silence beats inventing an identifier that resolves to nothing.""" + assert _citation_for(_doc(page_title="orphan")) is None + + +def test_no_fabricated_stable_id_is_ever_produced() -> None: + """The rule this design exists to keep. + + A made-up `R-` id would resolve to nothing, or worse to a real but wrong + entity, and a reader cannot tell the difference from the link text. + """ + citation = _citation_for( + _doc(source="https://reactome.org/userguide", page_title="Userguide") + ) + assert citation is not None + assert not citation.st_id.startswith("R-") + assert citation.st_id == "" + + +def test_a_local_file_path_is_never_cited_as_a_url() -> None: + """`source` is a generic LangChain field, not necessarily a web address. + + The CSV loaders set it to the file they read. Without a scheme check, a + Reactome document that happened to lack an `st_id` cited + "/home/awright/git/reactome_chatbot/embeddings/openai/..." -- a server path, + sent to a public website. Found by running a real question; the fixtures + above are too clean to have caught it. + """ + for path in ( + "/home/awright/git/reactome_chatbot/embeddings/openai/text-embedding-3-large", + "embeddings/reactome/Release97/reactions.csv", + "file:///etc/passwd", + "", + ): + assert _citation_for(_doc(source=path, page_title="whatever")) is None, path + + +def test_an_http_url_is_still_cited() -> None: + """The guard must not throw out what it exists to let through.""" + for url in ("https://reactome.org/userguide", "http://reactome.org/userguide"): + citation = _citation_for(_doc(source=url, page_title="Userguide")) + assert citation is not None + assert citation.url == url