diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index 9acee06..ff714ca 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -17,7 +17,12 @@ from agent.profiles import get_chat_profiles from agent.profiles.base import OutputState from agent.registry import get_graph -from gsa.chainlit_flow import Attachment, matrix_attachment, run_analysis +from gsa.chainlit_flow import ( + Attachment, + matrix_attachment, + result_file_kwargs, + run_analysis, +) from util.chainlit_helpers import ( PrefixedS3StorageClient, is_feature_enabled, @@ -147,8 +152,15 @@ async def run_gsa_analysis(attachment: Attachment) -> None: The flow takes these four as arguments so it can be tested without a browser; this is the only place that knows they are Chainlit. """ - progress = cl.Message(content="Reading your file…") - await progress.send() + # Created on the first update, not up front. + # + # It used to be sent before anything else, so it was the first message + # in the thread -- and Chainlit scrolls the latest user message to the + # top, which put the progress line above the fold. It updated faithfully + # for minutes where nobody could see it; the user saw "Started." and then + # nothing. Created lazily, it lands below "Started.", where they are + # looking. + progress: cl.Message | None = None async def ask_for_grouping() -> str | None: answer = await cl.AskUserMessage( @@ -160,13 +172,17 @@ async def send(text: str) -> None: await cl.Message(content=text).send() async def update_progress(text: str) -> None: - progress.content = text - await progress.update() + nonlocal progress + if progress is None: + progress = cl.Message(content=text) + await progress.send() + else: + progress.content = text + await progress.update() async def send_file(path: Path) -> None: await cl.Message( - content="", - elements=[cl.File(name=path.name, path=str(path), display="inline")], + content="", elements=[cl.File(**result_file_kwargs(path))] ).send() # The progress line is removed rather than marked "Done". @@ -184,7 +200,8 @@ async def send_file(path: Path) -> None: send_file=send_file, ) finally: - await progress.remove() + if progress is not None: + await progress.remove() @cl.on_message diff --git a/src/gsa/chainlit_flow.py b/src/gsa/chainlit_flow.py index 705f0ad..b5aaff0 100644 --- a/src/gsa/chainlit_flow.py +++ b/src/gsa/chainlit_flow.py @@ -14,7 +14,7 @@ """ from pathlib import Path -from typing import Any, Protocol +from typing import Any, Literal, Protocol, TypedDict from gsa import chat from gsa.client import GsaClient @@ -59,6 +59,37 @@ class Attachment(Protocol): path: str +class ResultFile(TypedDict): + """Exactly the `cl.File` arguments set here, so the unpacking type-checks.""" + + name: str + path: str + display: Literal["inline", "side", "page"] + mime: str + + +def result_file_kwargs(path: Path) -> ResultFile: + """Arguments for the `cl.File` that hands the user their results table. + + **`mime` must be explicit.** For an element given by `path`, Chainlit + infers the type with `filetype.guess()`, which reads magic bytes -- and a + TSV is plain text with no signature, so it came out `null`. Chainlit does + not fall back to the extension for paths, only for URLs. The browser then + called `mime.startsWith(...)` on null and the entire chat UI was replaced + by "Cannot read properties of null (reading 'startsWith')". + + That happened at the moment of success, on every analysis, and nothing + but a real browser could see it: the server logged the result as + delivered 11 ms before the page died. + """ + return { + "name": path.name, + "path": str(path), + "display": "inline", + "mime": "text/tab-separated-values", + } + + def matrix_attachment(elements: list[Any] | None) -> Attachment | None: """The first attachment that could be an expression matrix, if any. @@ -161,8 +192,17 @@ async def _run( await send(f"I could not start the analysis: {failure}") return except Exception: + # Not "I could not reach the service. Nothing was run." -- the first + # real run of this feature proved that untrue. The service *was* + # reached: it accepted the job with a 200, and the failure was in + # reading the reply. The analysis may well be running. This branch + # catches failures on both sides of the request, so it must not + # claim to know which. logger.exception("gsa submission failed") - await send("I could not reach the analysis service. Nothing was run.") + await send( + "Something went wrong while starting the analysis, so I cannot " + "follow it or return its results. Please try again in a moment." + ) return await send( @@ -198,5 +238,11 @@ async def _run( # It is computed rather than skipped so the disclosure rules stay # exercised by the tests; if a summary is added later, the bounded view # is what it must be given, not the result. + # Logged either side of delivery. The first real run wrote its result + # and then went silent: websocket frames are not logged, so there was no + # way to tell "sent and not shown" from "never sent". This line is the + # difference. + logger.info("gsa result sending", extra={"analysis": finished.analysis_id}) await send(chat.describe_result(finished)) await send_file(finished.table_path) + logger.info("gsa result delivered", extra={"analysis": finished.analysis_id}) diff --git a/src/gsa/chat.py b/src/gsa/chat.py index 1470371..5e7cfff 100644 --- a/src/gsa/chat.py +++ b/src/gsa/chat.py @@ -117,10 +117,29 @@ def parse_grouping(reply: str, sample_count: int) -> Grouping: def describe_progress(status: AnalysisStatus) -> str: - """One line, safe to send repeatedly as an edit.""" - percent = max(0, min(100, int(status.completed * 100))) - detail = status.description.strip() or "working" - return f"Running the analysis — {percent}% · {detail}" + """One line, safe to send repeatedly as an edit. + + **No percentage.** The first version showed `completed` as a percent, + and against the real service that read "60% · Permutation 1000 / 1000": + ReactomeGSA holds `completed` at 0.6 for the whole permutation phase + while its description counts through it. The description is the + service's own account and never contradicts itself; a number that does + is worse than none. + """ + detail = " ".join(status.description.split()) or "working" + return f"Running the analysis — {detail}" + + +#: Characters with meaning in markdown. Reactome pathway names contain some +#: of them -- measured over a real 2,679-pathway result: `H139Hfs13* PPM1K +#: causes a mild variant of MSUD`, `NOTCH1:M1580_K2555`. Two of either in a +#: cell become emphasis, and a variant identifier like `M1580_K2555` renders +#: as `M1580K2555` with nothing to show it changed. +_MARKDOWN_SPECIAL = "\\`*_[]<>|" + + +def _escape(text: str) -> str: + return "".join(f"\\{ch}" if ch in _MARKDOWN_SPECIAL else ch for ch in text) def describe_result(finished: Finished) -> str: @@ -146,7 +165,7 @@ def describe_result(finished: Finished) -> str: ] for pathway in top[:10]: lines.append( - f"| {pathway['name']} | {pathway['direction']} | {pathway['fdr']:.2g} |" + f"| {_escape(pathway['name'])} | {pathway['direction']} | {pathway['fdr']:.2g} |" ) lines.append("") diff --git a/src/gsa/client.py b/src/gsa/client.py index 451d156..c7eeb97 100644 --- a/src/gsa/client.py +++ b/src/gsa/client.py @@ -76,22 +76,30 @@ class GsaNotReadyError(GsaError): """ -def _identifier_from(payload: Any, kind: str) -> str: +def _identifier_from(body: str, kind: str) -> str: """Read an ID out of a response body, refusing anything else. - Both POSTs answer with the bare ID as a quoted JSON string. `str()` on - an unexpected shape -- an error object, a list -- produces a plausible - string like `{'detail': ...}` that is then used as a path segment and - fails somewhere far from here, as a 404 that looks like a missing - analysis rather than a malformed response. + **Both POSTs answer `text/plain`: the bare identifier, unquoted.** The + swagger says so (`produces: text/plain`, example `Analysis00371643`), + and the live service returns a bare UUID. + + The first version of this function said the opposite -- "the bare ID as a + quoted JSON string" -- and called `response.json()` on it. That was never + measured, and it meant every submission failed: the job was accepted with + a 200, the ID could not be parsed, and the user was told nothing had run. + The upload feature shipped that way and was never able to work. Every + test stubbed `submit()` at the method level, returning a Python string, + so the one line that was wrong was the one line no test executed. + + Quotes are still stripped, so a JSON-quoted body would also be accepted; + what matters is that the result is validated as an identifier before it + becomes a URL path segment. An error body, an HTML page from a proxy, or + an empty response produces a plausible string that would otherwise fail + far from here, as a 404 that reads like a missing analysis. """ - if not isinstance(payload, str): - raise GsaError( - f"expected a {kind}, got {type(payload).__name__}: {payload!r:.120}" - ) - value = payload.strip() + value = body.strip().strip('"').strip() if not _IDENTIFIER.match(value): - raise GsaError(f"{kind} is not a valid identifier: {value!r:.120}") + raise GsaError(f"{kind} is not a valid identifier: {value[:120]!r}") return value @@ -169,11 +177,22 @@ def failed(self) -> bool: class GsaClient: """Thin async client. Holds no state about a running analysis.""" - def __init__(self, url: str | None = None) -> None: + def __init__( + self, + url: str | None = None, + *, + transport: httpx.AsyncBaseTransport | None = None, + ) -> None: self._base = (url or base_url()).rstrip("/") + # Injectable so the HTTP layer itself can be tested. Without it the + # only way to test this client was to stub its methods, which is how + # a `response.json()` on a `text/plain` body reached production. + self._transport = transport async def _get(self, path: str, *, timeout: float = TIMEOUT_SECONDS) -> Any: - async with httpx.AsyncClient(timeout=timeout) as client: + async with httpx.AsyncClient( + timeout=timeout, transport=self._transport + ) as client: response = await client.get(f"{self._base}{path}") if response.status_code == 406: raise GsaNotReadyError(f"GET {path}: analysis is not complete") @@ -194,7 +213,9 @@ async def load_public_dataset(self, resource_id: str, dataset_id: str) -> str: _checked("resource", resource_id) _checked("dataset", dataset_id) body = [{"name": "dataset_id", "value": dataset_id}] - async with httpx.AsyncClient(timeout=TIMEOUT_SECONDS) as client: + async with httpx.AsyncClient( + timeout=TIMEOUT_SECONDS, transport=self._transport + ) as client: response = await client.post( f"{self._base}/data/load/{resource_id}", json=body ) @@ -203,7 +224,7 @@ async def load_public_dataset(self, resource_id: str, dataset_id: str) -> str: f"loading {dataset_id} from {resource_id} returned " f"{response.status_code}: {response.text[:200]}" ) - return _identifier_from(response.json(), "loading id") + return _identifier_from(response.text, "loading id") async def loading_status(self, loading_id: str) -> LoadingStatus: data = await self._get(f"/data/status/{_checked('loading id', loading_id)}") @@ -239,7 +260,9 @@ async def download_matrix(self, dataset_id: str) -> str: accepted values. """ url = f"{self._base}/data/download/{_checked('dataset', dataset_id)}" - async with httpx.AsyncClient(timeout=SUBMIT_TIMEOUT_SECONDS) as client: + async with httpx.AsyncClient( + timeout=SUBMIT_TIMEOUT_SECONDS, transport=self._transport + ) as client: response = await client.get(url, params={"format": "expr"}) if response.status_code != 200: raise GsaError(f"downloading {dataset_id} returned {response.status_code}") @@ -281,13 +304,15 @@ async def submit( } ], } - async with httpx.AsyncClient(timeout=SUBMIT_TIMEOUT_SECONDS) as client: + async with httpx.AsyncClient( + timeout=SUBMIT_TIMEOUT_SECONDS, transport=self._transport + ) as client: response = await client.post(f"{self._base}/analysis", json=body) if response.status_code != 200: raise GsaError( f"submitting returned {response.status_code}: {response.text[:200]}" ) - analysis_id = _identifier_from(response.json(), "analysis id") + analysis_id = _identifier_from(response.text, "analysis id") logger.info( "gsa analysis submitted", extra={ diff --git a/tests/gsa/test_gsa_chainlit_flow.py b/tests/gsa/test_gsa_chainlit_flow.py index ba498b9..b19cef3 100644 --- a/tests/gsa/test_gsa_chainlit_flow.py +++ b/tests/gsa/test_gsa_chainlit_flow.py @@ -268,3 +268,23 @@ async def test_a_binary_file_is_refused_rather_than_crashing( assert chat_.said, "it must say something rather than fail silently" assert not path.exists() + + +class TestTheResultFile: + def test_carries_an_explicit_mime_type(self, tmp_path: Path) -> None: + """Without it, the whole chat UI dies at the moment of success. + + Chainlit infers a path-based element's type from magic bytes, and a + TSV has none, so `mime` was null; the browser then called + `mime.startsWith(...)` on it and replaced the chat with a + JavaScript error. Found by a headless browser against the deployed + image, after every server-side check had passed. + """ + kwargs = chainlit_flow.result_file_kwargs(tmp_path / "reactome-gsa-an-1.tsv") + assert kwargs["mime"].startswith("text/") + + def test_names_the_file_after_the_table(self, tmp_path: Path) -> None: + path = tmp_path / "reactome-gsa-an-1.tsv" + kwargs = chainlit_flow.result_file_kwargs(path) + assert kwargs["name"] == "reactome-gsa-an-1.tsv" + assert kwargs["path"] == str(path) diff --git a/tests/gsa/test_gsa_chat.py b/tests/gsa/test_gsa_chat.py index 332f9f5..9c03e44 100644 --- a/tests/gsa/test_gsa_chat.py +++ b/tests/gsa/test_gsa_chat.py @@ -83,15 +83,26 @@ def test_an_uncounted_gene_total_is_said_in_words(self) -> None: assert "unknown number" in text assert "None" not in text - def test_progress_is_a_single_clamped_line(self) -> None: + def test_progress_is_one_line_carrying_the_services_own_account(self) -> None: line = chat.describe_progress( - AnalysisStatus("running", "Permutation 900 / 1000", 0.9) + AnalysisStatus("running", "Permutation 860 / 1000", 0.6) ) - assert "90%" in line + assert "Permutation 860 / 1000" in line assert "\n" not in line - # The service has reported completion values outside 0..1. - assert "100%" in chat.describe_progress(AnalysisStatus("running", "x", 4.2)) - assert "0%" in chat.describe_progress(AnalysisStatus("running", "x", -1.0)) + + def test_progress_does_not_show_a_percentage_the_service_does_not_update( + self, + ) -> None: + # Measured against the real service: `completed` stays at 0.6 for the + # whole permutation phase. Showing it produced + # "60% · Permutation 1000 / 1000". + line = chat.describe_progress( + AnalysisStatus("running", "Permutation 1000 / 1000", 0.6) + ) + assert "%" not in line + + def test_a_blank_description_still_says_something(self) -> None: + assert "working" in chat.describe_progress(AnalysisStatus("running", " ", 0.1)) def finished(tmp_path: Path) -> Finished: @@ -141,3 +152,61 @@ def test_the_link_is_still_absent_from_what_the_model_sees( # token, and whoever holds it can fetch the unredacted result -- # including the user's own gene identifiers. assert "PathwayBrowser" not in json.dumps(finished(tmp_path).for_model) + + +class TestPathwayNamesSurviveMarkdown: + """Real Reactome names contain markdown syntax. + + Measured over a real 2,679-pathway result: `NOTCH1:M1580_K2555` and + `H139Hfs13* PPM1K ...`. Unescaped, paired `_` or `*` become emphasis and + a variant identifier renders with characters missing -- silently, in a + results table someone may copy into a paper. + """ + + def result_with(self, tmp_path: Path, name: str) -> Finished: + table = tmp_path / "t.tsv" + table.write_text("Pathway\\tName\\n") + return Finished( + analysis_id="an-1", + for_model={ + "no_result": False, + "pathway_count": 1, + "significant_count": 1, + "top_pathways": [ + { + "stId": "R-HSA-1", + "name": name, + "direction": "Up", + "fdr": 1e-5, + "genes": 6, + } + ], + }, + links=[], + table_path=table, + ) + + @pytest.mark.parametrize( + "name", + [ + "Signaling by NOTCH1 t(7;9)(NOTCH1:M1580_K2555) Translocation Mutant", + "H139Hfs13* PPM1K causes a mild variant of MSUD", + "a_b_c and x*y*z", + "left | right", + ], + ) + def test_special_characters_are_escaped(self, tmp_path: Path, name: str) -> None: + text = chat.describe_result(self.result_with(tmp_path, name)) + row = next( + line for line in text.splitlines() if line.startswith("| ") and "Up" in line + ) + for ch in "_*|": + if ch in name: + assert "\\" + ch in row, f"{ch!r} not escaped in {row!r}" + + def test_a_pipe_does_not_add_a_column(self, tmp_path: Path) -> None: + # An unescaped `|` splits the cell and shifts Direction and FDR one + # column right, so the table reports the wrong value in each. + text = chat.describe_result(self.result_with(tmp_path, "left | right")) + row = next(line for line in text.splitlines() if "Up" in line) + assert row.replace("\\|", "").count("|") == 4 diff --git a/tests/gsa/test_gsa_client.py b/tests/gsa/test_gsa_client.py index 5441224..ef271c2 100644 --- a/tests/gsa/test_gsa_client.py +++ b/tests/gsa/test_gsa_client.py @@ -26,12 +26,15 @@ def test_real_identifiers_are_accepted(value: str) -> None: assert gsa_client._checked("dataset", value) == value -def test_an_id_response_must_be_a_string() -> None: - # The service answers a submission with a bare quoted ID. `str()` on an - # error object yields a plausible-looking path segment that fails far - # away, as a 404 that reads like a missing analysis. - with pytest.raises(gsa_client.GsaError, match="expected a analysis id"): - gsa_client._identifier_from({"detail": "Bad Request"}, "analysis id") +def test_an_error_body_is_not_taken_for_an_id() -> None: + # This test used to hand `_identifier_from` a Python dict, because it was + # written against the same wrong belief as the code: that the reply is + # JSON. It is `text/plain`. So the test could only ever confirm the + # assumption, never the service -- which is how the upload feature + # shipped unable to start an analysis. It now gets the *text* of an + # error body, which is what would actually arrive. + with pytest.raises(gsa_client.GsaError, match="not a valid identifier"): + gsa_client._identifier_from('{"detail": "Bad Request"}', "analysis id") def test_a_real_id_response_is_accepted() -> None: diff --git a/tests/gsa/test_gsa_http.py b/tests/gsa/test_gsa_http.py new file mode 100644 index 0000000..465f321 --- /dev/null +++ b/tests/gsa/test_gsa_http.py @@ -0,0 +1,138 @@ +"""The client against HTTP responses shaped like the real service's. + +Every earlier test of this package stubbed `GsaClient` at the method level, +returning Python values. That tested everything except the client itself, +and the client was where the bug was: `submit` called `response.json()` on a +`text/plain` body, so the upload feature could never start an analysis. It +was deployed that way, and 67 tests, CI and the answer sweep all passed. + +Found by a headless browser driving the deployed image, not by any of those. + +These tests go through `httpx.MockTransport`, so the request is built, sent +and parsed by the same code that runs in production. The response bodies are +the service's own: a bare UUID as `text/plain`, measured from +`POST /analysis` on 2026-09-21 and declared in its swagger +(`produces: text/plain`, example `Analysis00371643`). +""" + +import asyncio +import json +from typing import Any + +import httpx +import pytest + +from gsa.client import GsaClient, GsaError + +ANALYSIS_ID = "30790e1a-b5f2-11f1-b43e-026f8a266be5" + + +def client_answering(handler: Any) -> GsaClient: + return GsaClient("https://gsa.test/0.1", transport=httpx.MockTransport(handler)) + + +def submit(client: GsaClient) -> str: + return asyncio.run( + client.submit( + method="PADOG", + dataset_name="uploaded", + dataset_type="rnaseq_counts", + matrix="\tS1\tS2\nENSG1\t1\t2\n", + samples=["S1", "S2"], + analysis_group=["A", "B"], + group1="A", + group2="B", + ) + ) + + +def plain(text: str, status: int = 200) -> httpx.Response: + return httpx.Response( + status, text=text, headers={"content-type": "text/plain; charset=utf-8"} + ) + + +class TestSubmit: + def test_reads_the_bare_id_the_service_actually_returns(self) -> None: + # The bug. This is the body `POST /analysis` sends. + assert submit(client_answering(lambda _: plain(ANALYSIS_ID))) == ANALYSIS_ID + + def test_tolerates_a_trailing_newline(self) -> None: + assert ( + submit(client_answering(lambda _: plain(ANALYSIS_ID + "\n"))) == ANALYSIS_ID + ) + + def test_would_also_accept_a_json_quoted_id(self) -> None: + # Not what the service does today. Accepted so a change of framing + # upstream does not silently break the feature a second time. + assert ( + submit(client_answering(lambda _: plain(json.dumps(ANALYSIS_ID)))) + == ANALYSIS_ID + ) + + @pytest.mark.parametrize( + "body", + [ + "", + '{"detail": "Bad Request"}', + "502 Bad Gateway", + "Analysis/../../status", + ], + ) + def test_refuses_a_body_that_is_not_an_identifier(self, body: str) -> None: + # Each of these would otherwise become a URL path segment and fail + # later as a 404 that reads like a missing analysis. + with pytest.raises(GsaError, match="not a valid identifier"): + submit(client_answering(lambda _, b=body: plain(b))) + + def test_sends_the_matrix_inline_and_the_design_the_service_needs(self) -> None: + seen: dict[str, Any] = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["path"] = request.url.path + seen["body"] = json.loads(request.content) + return plain(ANALYSIS_ID) + + submit(client_answering(handler)) + + assert seen["path"] == "/0.1/analysis" + dataset = seen["body"]["datasets"][0] + assert dataset["data"].startswith("\tS1\tS2") + assert dataset["design"]["comparison"] == {"group1": "A", "group2": "B"} + assert dataset["name"] == "uploaded" + + def test_a_non_200_is_an_error_with_the_status(self) -> None: + with pytest.raises(GsaError, match="500"): + submit( + client_answering(lambda _: plain("Internal Server Error", status=500)) + ) + + +class TestLoad: + def test_reads_the_bare_loading_id(self) -> None: + # `POST /data/load/{resourceId}` is also `text/plain`, and had the + # same bug. It has no chat route yet, so nobody would have found it + # until one was added. + client = client_answering( + lambda _: plain("160b3166-b5f2-11f1-b383-e618a8785587") + ) + loading_id = asyncio.run( + client.load_public_dataset("example_datasets", "EXAMPLE_MEL_RNA") + ) + assert loading_id == "160b3166-b5f2-11f1-b383-e618a8785587" + + +class TestTheJsonEndpointsStayJson: + def test_status_is_still_read_as_json(self) -> None: + # The fix must not over-correct: /status, /data/summary and /result + # really are JSON, measured. + body = { + "id": ANALYSIS_ID, + "status": "running", + "description": "Permutation 60 / 1000", + "completed": 0.6, + } + client = client_answering(lambda _: httpx.Response(200, json=body)) + status = asyncio.run(client.analysis_status(ANALYSIS_ID)) + assert status.status == "running" + assert status.completed == 0.6