diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index 7355a62..059a75c 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -279,8 +279,6 @@ async def continue_from_handoff(handoff_id: str) -> None: await cl.Message(content=seed.UNAVAILABLE).send() return - if isinstance(handoff, AnalysisHandoff): - cl.user_session.set("analysis_seeded", True) logger.info( "handoff claimed: %s, tier %s, with data %s", handoff.kind, @@ -535,14 +533,24 @@ async def answer_with_model(content: str, message_id: str) -> None: # data they agreed to show the model provider -- not a search engine. enable_postprocess: bool = is_feature_enabled( config, "postprocessing" - ) and not cl.user_session.get("analysis_seeded", False) - result: OutputState = await get_graph().ainvoke( - content, - chat_profile.lower(), - callbacks=[chainlit_cb, openai_cb], - thread_id=thread_id, - enable_postprocess=enable_postprocess, - ) + ) and not await get_graph().thread_holds_analysis(chat_profile.lower(), thread_id) + try: + result: OutputState = await get_graph().ainvoke( + content, + chat_profile.lower(), + callbacks=[chainlit_cb, openai_cb], + thread_id=thread_id, + enable_postprocess=enable_postprocess, + ) + except asyncio.CancelledError: + raise # Stop: nothing to say. + except Exception: + # Chainlit registers on_message without a task wrapper, which logs an + # exception and sends nothing: a 429, an outage or a context-length + # error left the question unanswered with no word (review, area 2). + logger.exception("answering a chat message failed") + await cl.Message(content=gene_list.FAILED_TO_ANSWER).send() + return assistant_message: cl.Message | None = chainlit_cb.final_stream if assistant_message is not None: # Once streamed: "CDK5:p25" in a citation or the prose was read as a @@ -612,8 +620,11 @@ def solve_key() -> str: ) if verdict.ok: return f"solve:{verdict.nonce}" - forwarded = str(environ.get("HTTP_X_FORWARDED_FOR", "")).split(",")[0].strip() - return f"ip:{forwarded or environ.get('REMOTE_ADDR', '')}" + # Not X-Forwarded-For: the client writes its first hop, so rotating it + # bought a fresh quota per value (review, area 2). Cloudflare sets + # CF-Connecting-IP itself, and overwrites any a client sends. + client_ip = str(environ.get("HTTP_CF_CONNECTING_IP", "")).strip() + return f"ip:{client_ip or environ.get('REMOTE_ADDR', '')}" @cl.on_message diff --git a/src/agent/graph.py b/src/agent/graph.py index 574ef18..8209df9 100644 --- a/src/agent/graph.py +++ b/src/agent/graph.py @@ -402,6 +402,23 @@ async def close_pool(self) -> None: if self.pool: await self.pool.close() + async def thread_holds_analysis(self, profile: str, thread_id: str) -> bool: + """Whether a handoff seeded a reader's analysis into this thread. + + Read from the thread's own history, so it holds for as long as the + history does -- across reconnects and restarts. + """ + if self.graph is None or profile not in self.graph: + return False + state = await self.graph[profile].aget_state( + RunnableConfig(configurable={"thread_id": thread_id}) + ) + history = (state.values or {}).get("chat_history") or [] + return any( + getattr(m, "additional_kwargs", {}).get("reactome_analysis_seed") + for m in history + ) + async def forget_thread(self, thread_id: str) -> None: """Delete a one-shot thread's checkpoints. diff --git a/src/handoff/seed.py b/src/handoff/seed.py index 01cc827..4c7c94c 100644 --- a/src/handoff/seed.py +++ b/src/handoff/seed.py @@ -30,6 +30,10 @@ from handoff.store import DEFAULT_TTL_SECONDS, AnalysisHandoff, Handoff, SearchHandoff from util.markdown import escape, inert_html +#: Set on a seeded analysis turn. `AgentGraph.thread_holds_analysis` looks +#: for it. +ANALYSIS_SEED_MARK = "reactome_analysis_seed" + #: What the reader asked for on the website, stated as what happened. HUMAN_TURN = ( "Summarise my Reactome pathway analysis. (Asked on the analysis results page.)" @@ -119,7 +123,17 @@ def seeded_turn( ) if "verdict" in data: appendix += f"\nInstructions that go with it: {summary_instruction(data)}" - return [HumanMessage(HUMAN_TURN), AIMessage(handoff.summary + appendix)] + # Marked, so "this thread holds a reader's analysis" is read from the + # thread itself. It was a flag in the browser session, which a reconnect + # after an hour or a restart lost while the thread lived on -- and web + # search, rephrased from that history, came back on (review, area 2). + return [ + HumanMessage(HUMAN_TURN), + AIMessage( + handoff.summary + appendix, + additional_kwargs={ANALYSIS_SEED_MARK: True}, + ), + ] def shown_to_reader(handoff: Handoff) -> str: diff --git a/src/util/chainlit_helpers.py b/src/util/chainlit_helpers.py index 15b75d0..2fa2d97 100644 --- a/src/util/chainlit_helpers.py +++ b/src/util/chainlit_helpers.py @@ -33,18 +33,30 @@ async def upload_file( # content_disposition arrived in chainlit 2.1 and is forwarded, not # dropped: it is what makes an attachment download under its original # filename instead of the object key. - object_key = str(self._prefix / object_key) + object_key = self._prefixed(object_key) return await super().upload_file( object_key, data, mime, overwrite, content_disposition ) + def _prefixed(self, object_key: str) -> str: + """The key under the prefix -- once. + + `upload_file` returns the key it stored, prefix included, and the data + layer saves that and hands it back to `delete_file` and + `get_read_url`. Prefixing those again pointed at 'P/P/...': resumed + threads' attachments would not load, and deleting a thread "deleted" + a key that did not exist, which S3 reports as success, leaving the + reader's file in the bucket for good (review, area 2). + """ + if object_key == str(self._prefix) or object_key.startswith(f"{self._prefix}/"): + return object_key + return str(self._prefix / object_key) + async def delete_file(self, object_key: str) -> bool: - object_key = str(self._prefix / object_key) - return await super().delete_file(object_key) + return await super().delete_file(self._prefixed(object_key)) async def get_read_url(self, object_key: str) -> str: - object_key = str(self._prefix / object_key) - return await super().get_read_url(object_key) + return await super().get_read_url(self._prefixed(object_key)) def get_user_id() -> str | None: diff --git a/src/util/markdown.py b/src/util/markdown.py index b9ff86f..5ab413d 100644 --- a/src/util/markdown.py +++ b/src/util/markdown.py @@ -32,11 +32,20 @@ def inert_html(text: str) -> str: #: A colon that the chat's markdown would read as a directive: `remark-directive` #: is in Chainlit's renderer, so the ":p25" in "CDK5:p25" was parsed as markup -#: and dropped -- shown as "CDK5", a break, then the rest. Reactome names are -#: full of these (complexes are written A:B). Never "://" in a URL. -_DIRECTIVE_COLON = re.compile(r"(?<=[A-Za-z0-9]):(?=[A-Za-z])(?!//)") +#: and dropped. Any colon followed by a letter starts one, whatever precedes +#: it -- the first version only escaped colons after a letter or digit, and +#: 1,206 of Release 97's 25,286 colon names still broke ("(ACTA2,ACTG2):ATP", +#: "TNF-alpha:TNFR1"). Measured with the same parser chain: 0 now, in prose, link +#: labels and table cells. URLs ("://"), times and "Note: x" are untouched. +_DIRECTIVE_COLON = re.compile(r"(? str: - """Keep "A:B" literal in rendered markdown; nothing else changes.""" - return _DIRECTIVE_COLON.sub(r"\\:", text) + """Keep "A:B" literal in rendered markdown; code is left alone.""" + parts = _CODE.split(text) + return "".join( + part if index % 2 else _DIRECTIVE_COLON.sub(r"\\:", part) + for index, part in enumerate(parts) + ) diff --git a/tests/conftest.py b/tests/conftest.py index 1ee9f59..c65c302 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,10 @@ import importlib.util +import os + +# Importing chainlit runs load_dotenv on the working directory's .env, which +# put the repo's real configuration -- keys included -- into the test process +# and changed what other tests saw (found 2026-10-03). Point it at nothing. +os.environ["CHAINLIT_ENV_FILE"] = "/nonexistent/.env.tests" import pytest diff --git a/tests/handoff/test_handoff_seed.py b/tests/handoff/test_handoff_seed.py index 678cc56..3a285ee 100644 --- a/tests/handoff/test_handoff_seed.py +++ b/tests/handoff/test_handoff_seed.py @@ -271,3 +271,31 @@ def test_markup_in_a_handed_off_summary_is_shown_as_text() -> None: ) assert re.search(r"(? None: + # The web-search guard reads this mark from the thread itself; a flag in + # the browser session was lost on reconnect (review, area 2). + turn = seed.seeded_turn(handoff(), {"pathways": []}) + assert turn[1].additional_kwargs.get(seed.ANALYSIS_SEED_MARK) is True + + +def test_the_graph_finds_the_mark_in_a_threads_history() -> None: + from types import SimpleNamespace + + from agent.graph import AgentGraph + + def graph_with(history: list[Any]) -> AgentGraph: + class _Compiled: + async def aget_state(self, _config: Any) -> Any: + return SimpleNamespace(values={"chat_history": history}) + + g = object.__new__(AgentGraph) + g.graph = {"react_to_me": _Compiled()} # type: ignore[dict-item] + return g + + seeded = graph_with(seed.seeded_turn(handoff(), {"pathways": []})) + plain = graph_with([HumanMessage("hi"), AIMessage("hello")]) + assert asyncio.run(seeded.thread_holds_analysis("react_to_me", "t")) + assert not asyncio.run(plain.thread_holds_analysis("react_to_me", "t")) + assert not asyncio.run(seeded.thread_holds_analysis("unknown", "t")) diff --git a/tests/util/test_markdown.py b/tests/util/test_markdown.py index 97daed5..c4e7676 100644 --- a/tests/util/test_markdown.py +++ b/tests/util/test_markdown.py @@ -32,3 +32,20 @@ def test_directive_colons_are_escaped_and_nothing_else( def test_names_in_tables_and_handed_off_summaries_get_it_too() -> None: assert "CDK5\\:p25" in escape("CDK5:p25 complex") assert inert_html("**A:B** ") == "**A\\:B** \\" + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + # The first version only escaped a colon after a letter or digit; + # these still broke (review, area 2). + ("(ACTA2,ACTG2):ATP", "(ACTA2,ACTG2)\\:ATP"), + ("TNF-α:TNFR1", "TNF-α\\:TNFR1"), + ("(2xp-2S-SMAD3):SMAD4:SP1", "(2xp-2S-SMAD3)\\:SMAD4\\:SP1"), + # Code is shown verbatim: no backslashes there. + ("`A:B` and A:B", "`A:B` and A\\:B"), + ("```\nA:B\n```", "```\nA:B\n```"), + ], +) +def test_every_directive_shaped_colon_outside_code(text: str, expected: str) -> None: + assert escape_directives(text) == expected diff --git a/tests/util/test_prefixed_storage.py b/tests/util/test_prefixed_storage.py new file mode 100644 index 0000000..698152f --- /dev/null +++ b/tests/util/test_prefixed_storage.py @@ -0,0 +1,60 @@ +"""The S3 storage client prefixes a key once, not on every round trip.""" + +import asyncio +from pathlib import PurePosixPath +from typing import Any + +import pytest +from chainlit.data.storage_clients.s3 import S3StorageClient + +from util.chainlit_helpers import PrefixedS3StorageClient + + +@pytest.fixture +def seen(monkeypatch: pytest.MonkeyPatch) -> list[tuple[str, str]]: + calls: list[tuple[str, str]] = [] + + async def upload(_self: Any, key: str, *_a: Any, **_k: Any) -> dict[str, Any]: + calls.append(("upload", key)) + return {"object_key": key, "url": f"https://bucket/{key}"} + + async def delete(_self: Any, key: str) -> bool: + calls.append(("delete", key)) + return True + + async def read(_self: Any, key: str) -> str: + calls.append(("read", key)) + return f"https://bucket/{key}" + + monkeypatch.setattr(S3StorageClient, "upload_file", upload) + monkeypatch.setattr(S3StorageClient, "delete_file", delete) + monkeypatch.setattr(S3StorageClient, "get_read_url", read) + return calls + + +def _client() -> PrefixedS3StorageClient: + client = object.__new__(PrefixedS3StorageClient) # no boto3, no bucket + client._prefix = PurePosixPath("P") + return client + + +def test_the_stored_key_comes_back_to_the_same_object( + seen: list[tuple[str, str]], +) -> None: + # upload returns the key it stored, prefix included; the data layer saves + # it and later reads and deletes with it. Prefixing again gave 'P/P/...' + # (review, area 2). + client = _client() + stored = asyncio.run(client.upload_file("user/el/counts.tsv", b"x"))["object_key"] + asyncio.run(client.get_read_url(stored)) + asyncio.run(client.delete_file(stored)) + assert seen == [ + ("upload", "P/user/el/counts.tsv"), + ("read", "P/user/el/counts.tsv"), + ("delete", "P/user/el/counts.tsv"), + ] + + +def test_an_unprefixed_key_is_still_prefixed(seen: list[tuple[str, str]]) -> None: + asyncio.run(_client().delete_file("user/el/counts.tsv")) + assert seen == [("delete", "P/user/el/counts.tsv")]