The upload feature could never work: four bugs only a browser could see

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

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})

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("")
 

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={

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)