From 6dee0898c3598e770b852a11b7a344e8e7fa5dd9 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 25 Sep 2026 14:52:35 +0000 Subject: [PATCH] The upload feature could never work: four bugs only a browser could see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec 012's upload flow was deployed to beta on 2026-09-22 and nobody had run it. Driving the deployed image with a headless browser -- a private copy of the same image, Turnstile off, against the real ReactomeGSA -- found four bugs, every one past 67 tests, CI, a 15/15 answer sweep and the routing probe. **1. Submission always failed.** Both POST endpoints answer `text/plain`: a bare, unquoted identifier (the swagger says `produces: text/plain`, example `Analysis00371643`). `submit` and `load_public_dataset` called `response.json()` on it. The comment above `_identifier_from` stated the opposite -- "the bare ID as a quoted JSON string" -- which was never measured. Every test stubbed `submit()` at the method level and returned a Python string, so the one line that was wrong was the one line no test ran. Even the test of the parser handed it a dict, because it was written against the same belief. **2. The failure message was untrue.** It said "I could not reach the analysis service. Nothing was run." The service had been reached and had accepted the job with a 200; the failure was in reading the reply, so the analysis was running, orphaned. The generic branch now says only that something went wrong starting it. **3. The whole chat UI died at the moment of success.** For an element given by `path`, Chainlit infers the MIME type with `filetype.guess()`, which reads magic bytes. A TSV has none, so it came out null, and Chainlit only falls back to the extension for URLs. The browser then called `mime.startsWith()` on null and replaced the chat with "Cannot read properties of null (reading 'startsWith')". The server logged the result delivered 11 ms before the page died. `result_file_kwargs` now sets the type explicitly. **4. Progress updated where nobody could see it.** The progress message was sent first, so it was the first message in the thread, and Chainlit scrolls the latest user message to the top. It updated faithfully for minutes above the fold while the user saw "Started." and nothing else. It is now created on the first update, so it lands below "Started.". **Found in review, from real data:** Reactome pathway names contain markdown syntax -- `NOTCH1:M1580_K2555`, `H139Hfs13* PPM1K ...`. Unescaped, paired `_` or `*` become emphasis and a variant identifier loses characters silently, and a `|` shifts Direction and FDR a column right. Names are escaped. Also: `GsaClient` takes an injectable transport, because without one the only way to test it was to stub its methods, which is how #1 shipped. The new `test_gsa_http.py` goes through `httpx.MockTransport` with the service's real response shapes. `chainlit_flow` logs either side of delivering the result, because the first real run went silent after "result written" and websocket frames are not logged: there was no way to tell "sent and not shown" from "never sent". **Found against the real service:** progress read "60% · Permutation 1000 / 1000". ReactomeGSA holds `completed` at 0.6 for the whole permutation phase while its description counts through it, so the percentage is dropped and the service's own description shown. Verified in a browser, first against a fake service replaying the real recorded responses (so an iteration took 11 seconds, not 10 minutes, and showed the delay was never the cause), then against gsa.reactome.org: file attaches, 16 samples read back, labels accepted, analysis starts, progress shows below "Started." and updates in place (one message, six stages against the real service), the result shows 275 of 2,679 pathways significant with a table and the Pathway Browser link, and the table downloads -- HTTP 200, 350 KB, all 2,679 rows and nine columns. Sabotage: `response.json()` back in `submit` fails 7 tests (0 before this change); dropping the MIME type fails its test; dropping the escaping fails 5. Known and left: the user's own uploaded file is deleted once submitted (FR-007, for disk), so the attachment chip in their message becomes a dead link. Co-Authored-By: Claude Opus 5.5 --- bin/chat-chainlit.py | 33 +++++-- src/gsa/chainlit_flow.py | 50 +++++++++- src/gsa/chat.py | 29 +++++- src/gsa/client.py | 63 +++++++++---- tests/gsa/test_gsa_chainlit_flow.py | 20 ++++ tests/gsa/test_gsa_chat.py | 81 ++++++++++++++-- tests/gsa/test_gsa_client.py | 15 +-- tests/gsa/test_gsa_http.py | 138 ++++++++++++++++++++++++++++ 8 files changed, 383 insertions(+), 46 deletions(-) create mode 100644 tests/gsa/test_gsa_http.py 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