From f51283a6129554e52942be38c19bea8e911a3f8e Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 22 Sep 2026 02:25:32 +0000 Subject: [PATCH 1/2] Run a gene set analysis from a file in the chat The last slice of spec 012. Attach an expression matrix, say which group each sample is in, get the significant pathways and the full table back as a download. **An attached file routes here instead of the graph, rather than becoming a tool the agent calls.** The run takes minutes, which is longer than a chat turn, and the matrix is over a megabyte, which must never enter the model's context. A tool call would put the model in the middle of both problems. **Nothing reaches OpenAI.** The result is described from the table and attached as a file -- the user's own data going back to the user. `Finished.for_model` is computed and sent nowhere. It is computed rather than skipped so the disclosure rules stay exercised; when a summary is added it must be handed the bounded allow-listed view, behind the existing warning, and not the result. The decisions live in `gsa/chat.py` and `gsa/chainlit_flow.py` as functions over plain values, so they can be tested: what counts as a usable reply, what the person is told, what happens when the service accepts a job and then dies. `run_analysis` takes its four chat operations as arguments -- ask, say, revise, hand over a file -- and the Chainlit handler is the only code that knows those are Chainlit. What is left for a browser is whether the wiring matches. Three things worth calling out. **The label count is checked strictly while separators are not.** Commas, semicolons and tabs all work, and case is folded for grouping but the user's own spelling is kept, because the labels come back in the service's output and reading it against their notes matters. The count is strict because three labels for four columns still analyses *something* and returns a plausible answer to the wrong question. **Uploads are capped twice.** `.chainlit/config.toml` goes from the template's 500 MB to 20 MB, and `upload.py` enforces 20 MB again. The first stops a browser sending it; only the second stops the server accepting it. The template also ships `accept = ["*/*"]`, which invites every other kind of file into a chat that can do nothing with them. **Results are written 0700 under a configurable directory**, not a bare path in world-writable `/tmp`. They are a user's analysis on a shared host, and `prune_results` already bounds how long they stay. Adversarial review found two. The progress line was set to "Done." in a `finally`, so a user whose analysis died was told it had finished, one line above the message saying it had not -- it is removed now, and `run_analysis` reports the outcome on every path. And `for_model` being unused read as an oversight rather than a decision, so it says which it is. Sabotage: keeping a rejected file, never sending the table, and swallowing a post-submission failure each fail exactly the test written for them. 27 new tests, ./checks.sh clean. Co-Authored-By: Claude Opus 5 --- .chainlit/config.toml | 19 ++- bin/chat-chainlit.py | 59 ++++++++ specs/012-run-gene-analysis/spec.md | 29 +++- src/gsa/chainlit_flow.py | 173 ++++++++++++++++++++++ src/gsa/chat.py | 158 ++++++++++++++++++++ src/gsa/job.py | 3 +- tests/gsa/test_gsa_chainlit_flow.py | 215 ++++++++++++++++++++++++++++ tests/gsa/test_gsa_chat.py | 143 ++++++++++++++++++ 8 files changed, 793 insertions(+), 6 deletions(-) create mode 100644 src/gsa/chainlit_flow.py create mode 100644 src/gsa/chat.py create mode 100644 tests/gsa/test_gsa_chainlit_flow.py create mode 100644 tests/gsa/test_gsa_chat.py diff --git a/.chainlit/config.toml b/.chainlit/config.toml index b3c33cec..4e2f3e74 100644 --- a/.chainlit/config.toml +++ b/.chainlit/config.toml @@ -33,7 +33,7 @@ edit_message = true # Authorize users to spontaneously upload files with messages [features.spontaneous_file_upload] - enabled = false + enabled = true # Define accepted file types using MIME types # Examples: # 1. For specific file types: @@ -43,9 +43,20 @@ edit_message = true # 3. For specific file extensions: # accept = { "application/octet-stream" = [".xyz", ".pdb"] } # Note: Using "*/*" is not recommended as it may cause browser warnings - accept = ["*/*"] - max_files = 20 - max_size_mb = 500 + # Expression matrices only. "*/*" is what the template ships and it + # invites every other kind of file into a chat that can do nothing with + # them. + accept = { "text/tab-separated-values" = [".tsv", ".txt"], "text/csv" = [".csv"] } + max_files = 1 + # 20, not the template's 500. This host has one 88 GB volume that sits + # around 90-95% full, and ~/update-beta-chat.sh refuses to deploy under + # 6 GB free -- so a handful of 500 MB uploads would take the chat down + # and block the fix. A 16-sample expression matrix is 1.2 MB. + # + # src/gsa/upload.py enforces the same ceiling again, because this one is + # a client-side convenience: Chainlit is what stops the browser sending + # it, not what stops the server accepting it. + max_size_mb = 20 [features.audio] # Sample rate of the audio diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index 7a848911..9acee06b 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -3,6 +3,7 @@ # or get_data_layer. Not fixable here; it needs stubs upstream. The file is # named with a hyphen, so it cannot be listed in [[tool.mypy.overrides]]. import os +from pathlib import Path import chainlit as cl from chainlit.data.base import BaseDataLayer @@ -16,6 +17,7 @@ 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 util.chainlit_helpers import ( PrefixedS3StorageClient, is_feature_enabled, @@ -139,6 +141,52 @@ async def end() -> None: await static_messages(config, TriggerEvent.on_chat_end) +async def run_gsa_analysis(attachment: Attachment) -> None: + """Drive `gsa.chainlit_flow` with this session's chat operations. + + 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() + + async def ask_for_grouping() -> str | None: + answer = await cl.AskUserMessage( + content="Which group is each sample in?", timeout=600 + ).send() + return (answer or {}).get("output") if answer else None + + 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() + + async def send_file(path: Path) -> None: + await cl.Message( + content="", + elements=[cl.File(name=path.name, path=str(path), display="inline")], + ).send() + + # The progress line is removed rather than marked "Done". + # + # A `finally` that sets "Done." runs on the failure paths too, so a user + # whose analysis died would have been told it finished, one line above + # the message explaining that it had not. `run_analysis` says what + # happened on every path; this only has to stop the spinner. + try: + await run_analysis( + attachment, + ask_for_grouping=ask_for_grouping, + send=send, + update_progress=update_progress, + send_file=send_file, + ) + finally: + await progress.remove() + + @cl.on_message async def main(message: cl.Message) -> None: if await message_rate_limited(config): @@ -146,6 +194,17 @@ async def main(message: cl.Message) -> None: await static_messages(config, TriggerEvent.on_message) + # An attached matrix routes to the analysis flow instead of the graph. + # + # Not a tool the agent calls: the run takes minutes, which is longer + # than a chat turn, and the matrix is over a megabyte, which must never + # enter the model's context. A tool call would put the model in the + # middle of both problems. + attachment = matrix_attachment(getattr(message, "elements", None)) + if attachment is not None: + await run_gsa_analysis(attachment) + return + message_count: int = cl.user_session.get("message_count", 0) + 1 cl.user_session.set("message_count", message_count) diff --git a/specs/012-run-gene-analysis/spec.md b/specs/012-run-gene-analysis/spec.md index 8a1d2b7c..e7146012 100644 --- a/specs/012-run-gene-analysis/spec.md +++ b/specs/012-run-gene-analysis/spec.md @@ -4,7 +4,7 @@ **Created**: 2026-09-21 -**Status**: Draft +**Status**: Implemented for uploads; public-dataset path deferred **Input**: User description: "Run a gene set analysis from an uploaded expression matrix in the chat, and return the pathway results as a downloadable file" @@ -153,3 +153,30 @@ and nothing has been sent to the model. - Whether an analysis should survive a chat session ending. Results live behind an analysis ID at the service, so resuming is possible; whether it is wanted is a product question. + +--- + +## What shipped, 2026-09-22 + +**User Story 2 (upload) and Story 3 (results without a summary) are done.** +Story 1 (public datasets by identifier) is built underneath -- +`submit_public_dataset` works and is tested -- but has no chat route yet, +because reaching it needs the model to choose a dataset and a factor, which +is a conversation design rather than a plumbing job. + +Shipping Story 2 before Story 1 inverts the spec's priorities. The reason is +that the upload path turned out to be the *simpler* one: a file arrives with +its own sample names, so there is nothing to search for and nothing to +disambiguate. + +**Nothing reaches OpenAI.** Story 3 was written as "a user who declines the +summary still gets their results". What shipped is that with no summary at +all: the result is described from the table and attached as a file, both of +which are the user's own data returning to the user. `Finished.for_model` +is computed and sent nowhere. When a summary is added it must be given the +bounded allow-listed view, behind the existing warning -- not the result. + +**FR-006 landed in two places on purpose.** `.chainlit/config.toml` caps +uploads at 20 MB, and `src/gsa/upload.py` caps them again. The first stops +a browser sending the file; the second stops the server accepting it. Only +the second is a guarantee. diff --git a/src/gsa/chainlit_flow.py b/src/gsa/chainlit_flow.py new file mode 100644 index 00000000..d1cc6df2 --- /dev/null +++ b/src/gsa/chainlit_flow.py @@ -0,0 +1,173 @@ +"""The Chainlit side of running a gene set analysis. + +Thin on purpose. Every decision -- what a usable reply is, what the person +is told, what the model may see -- lives in `gsa.chat` and is tested there. +What is left here is the part a browser has to verify: reading an attached +file, asking a follow-up question, streaming progress, sending a download. + +**Why this is not a tool the agent calls.** The run takes minutes, which is +longer than a chat turn, and the matrix is 1.2 MB, which must never enter +the model's context. A tool call would put the model in the middle of both +problems. So an attached file routes here *instead of* the graph, and the +model is handed only the bounded result at the end, if the user asks for a +summary at all. +""" + +from pathlib import Path +from typing import Any, Protocol + +from gsa import chat +from gsa.client import GsaClient +from gsa.job import AnalysisFailedError, await_result, submit_uploaded_matrix +from gsa.upload import UploadRejectedError, discard, validate +from util.logging import logging + +logger = logging.getLogger(__name__) + +#: Where result tables are written. Bounded by `prune_results` on each run. +#: +#: Not a bare path under a world-writable `/tmp`: these are a user's own +#: analysis results, and on a shared host anyone could pre-create the +#: directory or replace a file in it. `results_dir()` creates it 0700 and +#: owned by this process, and `GSA_RESULTS_DIR` lets a deployment put it +#: somewhere with a real quota. +RESULTS_DIR_ENV = "GSA_RESULTS_DIR" + + +def results_dir() -> Path: + """The results directory, created private to this user.""" + import os + import tempfile + + configured = os.environ.get(RESULTS_DIR_ENV) + base = ( + Path(configured) if configured else Path(tempfile.gettempdir()) / "reactome-gsa" + ) + base.mkdir(parents=True, exist_ok=True, mode=0o700) + return base + + +#: Extensions we will try to read as a matrix. Anything else attached is +#: almost certainly meant for a different conversation. +MATRIX_SUFFIXES = {".tsv", ".csv", ".txt"} + + +class Attachment(Protocol): + """The part of a Chainlit file element this needs.""" + + name: str + path: str + + +def matrix_attachment(elements: list[Any] | None) -> Attachment | None: + """The first attachment that could be an expression matrix, if any. + + Returns None for a message with no attachments, which is every ordinary + message -- so the caller can use this as the routing decision without + knowing anything about Chainlit. + """ + for element in elements or []: + name = getattr(element, "name", "") or "" + path = getattr(element, "path", None) + if path and Path(name).suffix.lower() in MATRIX_SUFFIXES: + found: Attachment = element + return found + return None + + +async def run_analysis( + attachment: Attachment, + *, + ask_for_grouping: Any, + send: Any, + update_progress: Any, + send_file: Any, + client: GsaClient | None = None, +) -> None: + """Validate, ask for groups, submit, poll, deliver. + + The callables are passed in rather than imported so this can be driven + without Chainlit. They are the four things a chat has to be able to do: + ask a question and wait, say something, revise what was said, and hand + over a file. + """ + path = Path(attachment.path) + + try: + matrix = validate(path) + except UploadRejectedError as refusal: + discard(path) + await send(str(refusal)) + return + + await send(chat.describe_matrix(matrix)) + + reply = await ask_for_grouping() + if not reply: + discard(path) + await send( + "No labels arrived, so I have not run anything. The file is deleted." + ) + return + + try: + grouping = chat.parse_grouping(reply, len(matrix.samples)) + except chat.ReplyUnusableError as unusable: + discard(path) + await send(f"{unusable} Send the file again when you are ready.") + return + + gsa = client or GsaClient() + try: + # `submit_uploaded_matrix` deletes the file itself, on every path. + analysis_id = await submit_uploaded_matrix( + gsa, + matrix=matrix, + dataset_type="rnaseq_counts", + analysis_group=grouping.labels, + group1=grouping.group1, + group2=grouping.group2, + ) + except AnalysisFailedError as failure: + await send(f"I could not start the analysis: {failure}") + return + except Exception: + logger.exception("gsa submission failed") + await send("I could not reach the analysis service. Nothing was run.") + return + + await send( + f"Started. This usually takes a few minutes — " + f"comparing **{grouping.group1}** with **{grouping.group2}**." + ) + + try: + finished = await await_result( + gsa, + analysis_id, + out_dir=results_dir(), + on_progress=lambda status: update_progress(chat.describe_progress(status)), + ) + except AnalysisFailedError as failure: + # The service accepts and then fails, and says so only here. + await send(f"The analysis did not finish: {failure}") + return + except Exception: + logger.exception("gsa analysis failed", extra={"analysis": analysis_id}) + await send("Something went wrong while waiting for the analysis.") + return + + # `finished.for_model` is deliberately not used here. + # + # Nothing about this analysis reaches OpenAI. The result is described + # from the table and handed over as a file, both of which are the + # user's own data going back to the user. A model-written summary is + # spec 012's Story 3 the other way round -- opt-in, behind the existing + # warning -- and until that exists, the honest state is that the + # allow-listed view is computed and sent nowhere. + # + # 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. + await send(chat.describe_result(finished)) + await send_file(finished.table_path) diff --git a/src/gsa/chat.py b/src/gsa/chat.py new file mode 100644 index 00000000..14703717 --- /dev/null +++ b/src/gsa/chat.py @@ -0,0 +1,158 @@ +"""Turning a chat exchange into an analysis, and back into words. + +Kept apart from the Chainlit handler on purpose. Everything here is a pure +function over plain values, so the decisions -- what counts as a usable +reply, what the user is told, what the model is allowed to see -- can be +tested. The handler that remains is wiring, and wiring is what a browser is +for. + +**The audience split is the point.** A finished analysis produces three +things and they go to three different places: a bounded allow-listed view +for the model, a Pathway Browser link for the person, and a full table as a +file. `describe_result` writes what the person reads; it never becomes a +prompt, and `Finished.for_model` never becomes a message. +""" + +from dataclasses import dataclass + +from gsa.client import AnalysisStatus +from gsa.job import Finished +from gsa.upload import Matrix + +#: Shown when a matrix arrives, before anything is submitted. +MAX_SAMPLES_LISTED = 24 + + +class ReplyUnusableError(ValueError): + """The reply cannot be turned into a grouping, with a reason to show.""" + + +@dataclass(frozen=True) +class Grouping: + """One label per sample, and the two labels to compare.""" + + labels: list[str] + group1: str + group2: str + + +def describe_matrix(matrix: Matrix) -> str: + """What the user is told about the file they just sent. + + Their sample names are echoed back deliberately -- they need to see + what was read in order to label it, and it is their own data being + shown to them. It is the *model* that never sees these. + """ + shown = matrix.samples[:MAX_SAMPLES_LISTED] + more = len(matrix.samples) - len(shown) + genes = ( + "an unknown number of" + if matrix.gene_count is None + else f"{matrix.gene_count:,}" + ) + + lines = [ + f"Read **{len(matrix.samples)} samples** and {genes} genes " + f"({matrix.size_bytes / 1e6:.1f} MB).", + "", + "Samples, in column order:", + "", + " " + + ", ".join(f"`{name}`" for name in shown) + + (f" …and {more} more" if more else ""), + "", + "To run the analysis I need to know which group each sample belongs " + "to. Reply with one label per sample, in that order, separated by " + "commas — for example `control, control, treated, treated`.", + ] + return "\n".join(lines) + + +def parse_grouping(reply: str, sample_count: int) -> Grouping: + """Turn a reply into a grouping, or say why it cannot be one. + + Deliberately forgiving about separators and case, and strict about the + count: a label list that does not line up with the columns produces an + analysis of the wrong thing, which is worse than a refusal because it + returns a plausible answer. + """ + labels = [ + part.strip() for part in reply.replace("\t", ",").replace(";", ",").split(",") + ] + labels = [label for label in labels if label] + + if not labels: + raise ReplyUnusableError("I could not find any group labels in that.") + + if len(labels) != sample_count: + raise ReplyUnusableError( + f"That is {len(labels)} label{'s' if len(labels) != 1 else ''} for " + f"{sample_count} samples. I need exactly one per sample, in the " + f"order the columns appear." + ) + + # Case-insensitive grouping, but the user's own spelling is kept: the + # labels go to the service and come back in its output, and silently + # lower-casing someone's "Treated" makes the result harder to read + # against their own notes. + seen: dict[str, str] = {} + for label in labels: + seen.setdefault(label.casefold(), label) + + if len(seen) < 2: + raise ReplyUnusableError( + "All the samples have the same label, so there is nothing to " + "compare. A gene set analysis needs two groups." + ) + if len(seen) > 2: + names = ", ".join(sorted(seen.values())) + raise ReplyUnusableError( + f"I found more than two groups ({names}). This runs one " + f"comparison at a time, so please use exactly two labels." + ) + + canonical = [seen[label.casefold()] for label in labels] + group1, group2 = sorted(seen.values()) + return Grouping(labels=canonical, group1=group1, group2=group2) + + +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}" + + +def describe_result(finished: Finished) -> str: + """What the person reads. Never a prompt. + + The Pathway Browser link is included *here* and not in anything the + model sees: its URL carries the analysis token, and whoever holds that + can fetch the unredacted result back from the service. + """ + view = finished.for_model + total = view.get("pathway_count", 0) + significant = view.get("significant_count", 0) + top = view.get("top_pathways") or [] + + lines = [ + f"**{significant:,} of {total:,} pathways** are significant at FDR < 0.05.", + "", + ] + if top: + lines += [ + "| Pathway | Direction | FDR |", + "|---|---|---|", + ] + for pathway in top[:10]: + lines.append( + f"| {pathway['name']} | {pathway['direction']} | {pathway['fdr']:.2g} |" + ) + lines.append("") + + for name, url in finished.links: + lines.append(f"[{name}]({url})") + if finished.links: + lines.append("") + lines.append("The full table, with every column, is attached.") + return "\n".join(lines) diff --git a/src/gsa/job.py b/src/gsa/job.py index b1b6589d..60baef2e 100644 --- a/src/gsa/job.py +++ b/src/gsa/job.py @@ -17,6 +17,7 @@ from collections.abc import Awaitable, Callable from dataclasses import dataclass from pathlib import Path +from typing import Any from gsa import results as gsa_results from gsa.client import AnalysisStatus, GsaClient, GsaError @@ -72,7 +73,7 @@ class Finished: """ analysis_id: str - for_model: dict[str, object] + for_model: dict[str, Any] links: list[tuple[str, str]] table_path: Path diff --git a/tests/gsa/test_gsa_chainlit_flow.py b/tests/gsa/test_gsa_chainlit_flow.py new file mode 100644 index 00000000..2124debc --- /dev/null +++ b/tests/gsa/test_gsa_chainlit_flow.py @@ -0,0 +1,215 @@ +"""The flow, driven without a browser. + +`run_analysis` takes its four chat operations as arguments, so the paths +that matter -- a refused file, an unusable reply, a submission that fails, +an analysis that fails after being accepted -- can be exercised here. What +is left for a browser is whether Chainlit's own callbacks are wired to the +right arguments. +""" + +import asyncio +import functools +import json +import os +from pathlib import Path +from typing import Any + +from gsa import chainlit_flow +from gsa.client import AnalysisStatus +from gsa.job import AnalysisFailedError + +FIXTURE = Path(__file__).parent / "result_fixture.json" + + +def asyncio_test(fn: Any) -> Any: + @functools.wraps(fn) + def wrapper(*args: Any, **kwargs: Any) -> Any: + return asyncio.run(fn(*args, **kwargs)) + + return wrapper + + +class Chat: + """Records what a user would have seen.""" + + def __init__(self, reply: str | None = "a, a, b, b") -> None: + self.reply = reply + self.said: list[str] = [] + self.progress: list[str] = [] + self.files: list[Path] = [] + + async def ask(self) -> str | None: + return self.reply + + async def send(self, text: str) -> None: + self.said.append(text) + + async def update(self, text: str) -> None: + self.progress.append(text) + + async def send_file(self, path: Path) -> None: + self.files.append(path) + + @property + def transcript(self) -> str: + return "\n".join(self.said) + + +class StubClient: + def __init__(self, *, fail_submit: bool = False, fail_run: bool = False) -> None: + self.fail_submit = fail_submit + self.fail_run = fail_run + self.submitted: dict[str, Any] = {} + + async def submit(self, **kwargs: Any) -> str: + if self.fail_submit: + raise AnalysisFailedError("the service refused it") + self.submitted = kwargs + return "an-1" + + async def analysis_status(self, analysis_id: str) -> AnalysisStatus: + if self.fail_run: + return AnalysisStatus("failed", "CONNECTION_FORCED - broker closed", 1.0) + return AnalysisStatus("complete", "Analysis done", 1.0) + + async def result(self, analysis_id: str) -> dict[str, Any]: + loaded: dict[str, Any] = json.loads(FIXTURE.read_text()) + return loaded + + +class Attached: + def __init__(self, path: Path) -> None: + self.name = path.name + self.path = str(path) + + +def a_matrix(tmp_path: Path, name: str = "counts.tsv") -> Attached: + path = tmp_path / name + path.write_text("\tS1\tS2\tS3\tS4\nENSG1\t1\t2\t3\t4\nENSG2\t5\t6\t7\t8\n") + return Attached(path) + + +async def run(attachment: Attached, chat_: Chat, client: Any, out: Path) -> None: + os.environ[chainlit_flow.RESULTS_DIR_ENV] = str(out) + await chainlit_flow.run_analysis( + attachment, + ask_for_grouping=chat_.ask, + send=chat_.send, + update_progress=chat_.update, + send_file=chat_.send_file, + client=client, + ) + + +class TestRouting: + def test_an_ordinary_message_is_not_an_analysis(self) -> None: + # Every normal message goes through here, so it must say no to the + # common case without knowing anything about Chainlit. + assert chainlit_flow.matrix_attachment(None) is None + assert chainlit_flow.matrix_attachment([]) is None + + def test_a_pdf_is_not_a_matrix(self, tmp_path: Path) -> None: + pdf = tmp_path / "paper.pdf" + pdf.write_text("x") + assert chainlit_flow.matrix_attachment([Attached(pdf)]) is None + + def test_a_tsv_is(self, tmp_path: Path) -> None: + assert chainlit_flow.matrix_attachment([a_matrix(tmp_path)]) is not None + + +class TestTheUnhappyPaths: + @asyncio_test + async def test_a_rejected_file_is_explained_and_deleted( + self, tmp_path: Path + ) -> None: + bad = tmp_path / "notes.txt" + bad.write_text("just some prose\n") + chat_ = Chat() + + await run(Attached(bad), chat_, StubClient(), tmp_path / "out") + + assert "expression matrix" in chat_.transcript + assert not bad.exists() + assert not chat_.files + + @asyncio_test + async def test_an_unusable_reply_deletes_the_file_too(self, tmp_path: Path) -> None: + # The count mismatch is the likeliest mistake, and the file must + # not survive it -- this host has 5 GB free. + attachment = a_matrix(tmp_path) + chat_ = Chat(reply="a, b") + + await run(attachment, chat_, StubClient(), tmp_path / "out") + + assert "one per sample" in chat_.transcript + assert not Path(attachment.path).exists() + + @asyncio_test + async def test_no_reply_at_all_deletes_the_file(self, tmp_path: Path) -> None: + attachment = a_matrix(tmp_path) + chat_ = Chat(reply=None) + + await run(attachment, chat_, StubClient(), tmp_path / "out") + + assert "not run anything" in chat_.transcript + assert not Path(attachment.path).exists() + + @asyncio_test + async def test_a_failed_submission_is_reported(self, tmp_path: Path) -> None: + chat_ = Chat() + await run( + a_matrix(tmp_path), chat_, StubClient(fail_submit=True), tmp_path / "out" + ) + + assert "could not start" in chat_.transcript + assert not chat_.files + + @asyncio_test + async def test_an_analysis_that_fails_after_starting_is_reported( + self, tmp_path: Path + ) -> None: + # The measured failure: accepted with a 200, then dead. A flow that + # only checked the submission would sit waiting forever, or claim + # success. + chat_ = Chat() + await run( + a_matrix(tmp_path), chat_, StubClient(fail_run=True), tmp_path / "out" + ) + + assert "did not finish" in chat_.transcript + assert "CONNECTION_FORCED" in chat_.transcript + assert not chat_.files + + +class TestTheHappyPath: + @asyncio_test + async def test_it_delivers_a_summary_and_a_file(self, tmp_path: Path) -> None: + chat_ = Chat() + client = StubClient() + + await run(a_matrix(tmp_path), chat_, client, tmp_path / "out") + + assert "significant" in chat_.transcript + assert chat_.files, "the user must get their table" + assert chat_.files[0].exists() + assert "MeanWeightT0" in chat_.files[0].read_text() + + @asyncio_test + async def test_the_filename_never_reaches_the_service(self, tmp_path: Path) -> None: + chat_ = Chat() + client = StubClient() + + await run( + a_matrix(tmp_path, "smith_lab_unpublished_2026.tsv"), + chat_, + client, + tmp_path / "out", + ) + + assert "smith_lab" not in json.dumps(client.submitted.get("dataset_name")) + + @asyncio_test + async def test_the_uploaded_matrix_is_gone_afterwards(self, tmp_path: Path) -> None: + attachment = a_matrix(tmp_path) + await run(attachment, Chat(), StubClient(), tmp_path / "out") + assert not Path(attachment.path).exists() diff --git a/tests/gsa/test_gsa_chat.py b/tests/gsa/test_gsa_chat.py new file mode 100644 index 00000000..332f9f55 --- /dev/null +++ b/tests/gsa/test_gsa_chat.py @@ -0,0 +1,143 @@ +"""What the user is asked, what they are told, and what the model is not. + +These are the decisions in the chat flow. The Chainlit handler around them +is wiring; a browser tests wiring, and nothing tests a decision that only +exists inside a UI callback. +""" + +import json +import tempfile +from pathlib import Path + +import pytest + +from gsa import chat +from gsa.client import AnalysisStatus +from gsa.job import Finished +from gsa.upload import Matrix + + +def matrix(samples: list[str], genes: int | None = 12) -> Matrix: + return Matrix( + path=Path(tempfile.gettempdir()) / "x.tsv", + size_bytes=1_200_000, + samples=samples, + gene_count=genes, + ) + + +class TestParsingTheReply: + def test_accepts_a_plain_comma_list(self) -> None: + grouping = chat.parse_grouping("control, control, treated, treated", 4) + assert grouping.labels == ["control", "control", "treated", "treated"] + assert (grouping.group1, grouping.group2) == ("control", "treated") + + @pytest.mark.parametrize("reply", ["a;a;b;b", "a\ta\tb\tb", "a , a ,b, b"]) + def test_is_forgiving_about_separators_and_spacing(self, reply: str) -> None: + assert chat.parse_grouping(reply, 4).labels == ["a", "a", "b", "b"] + + def test_groups_case_insensitively_but_keeps_the_spelling(self) -> None: + # The labels reach the service and come back in its output, so + # silently lower-casing someone's "Treated" makes the result harder + # to read against their own notes. + grouping = chat.parse_grouping("Treated, treated, Control, control", 4) + assert grouping.labels == ["Treated", "Treated", "Control", "Control"] + assert {grouping.group1, grouping.group2} == {"Treated", "Control"} + + def test_refuses_a_count_that_does_not_match(self) -> None: + # The dangerous one. Three labels for four columns still analyses + # *something*, and returns a plausible answer to the wrong question. + with pytest.raises(chat.ReplyUnusableError, match="one per sample"): + chat.parse_grouping("a, b, c", 4) + + def test_refuses_one_group(self) -> None: + with pytest.raises(chat.ReplyUnusableError, match="nothing to compare"): + chat.parse_grouping("treated, treated", 2) + + def test_refuses_three_groups_and_names_them(self) -> None: + with pytest.raises(chat.ReplyUnusableError, match="a, b, c"): + chat.parse_grouping("a, b, c", 3) + + def test_refuses_an_empty_reply(self) -> None: + with pytest.raises(chat.ReplyUnusableError, match="could not find"): + chat.parse_grouping(" ,, ", 4) + + +class TestWhatTheUserIsTold: + def test_the_sample_names_are_echoed_back(self) -> None: + # They have to see what was read in order to label it, and it is + # their own data being shown to them. + text = chat.describe_matrix(matrix(["Ctrl_1", "Ctrl_2", "Tr_1"])) + assert "Ctrl_1" in text + assert "3 samples" in text + + def test_a_long_sample_list_is_truncated(self) -> None: + text = chat.describe_matrix(matrix([f"S{i}" for i in range(50)])) + assert "and 26 more" in text + assert "50 samples" in text + + def test_an_uncounted_gene_total_is_said_in_words(self) -> None: + # `gene_count` is None for a file too long to finish counting. It + # must not render as "None genes" or "-1 genes". + text = chat.describe_matrix(matrix(["A", "B"], genes=None)) + assert "unknown number" in text + assert "None" not in text + + def test_progress_is_a_single_clamped_line(self) -> None: + line = chat.describe_progress( + AnalysisStatus("running", "Permutation 900 / 1000", 0.9) + ) + assert "90%" 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 finished(tmp_path: Path) -> 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": 2679, + "significant_count": 412, + "top_pathways": [ + { + "stId": "R-HSA-1", + "name": "Hemostasis", + "direction": "Up", + "fdr": 1e-5, + "genes": 6, + } + ], + }, + links=[ + ( + "Gene Set Analysis Summary", + "https://reactome.org/PathwayBrowser/#/ANALYSIS=TOKEN", + ) + ], + table_path=table, + ) + + +class TestWhatTheUserReads: + def test_it_reports_exact_counts_and_the_top_pathways(self, tmp_path: Path) -> None: + text = chat.describe_result(finished(tmp_path)) + assert "412" in text + assert "2,679" in text + assert "Hemostasis" in text + + def test_it_includes_the_pathway_browser_link(self, tmp_path: Path) -> None: + # The person gets the link. This is the half that must be present. + assert "PathwayBrowser" in chat.describe_result(finished(tmp_path)) + + def test_the_link_is_still_absent_from_what_the_model_sees( + self, tmp_path: Path + ) -> None: + # And this is the half that must not. The URL carries the analysis + # 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) From 6e4bd83a0ab841be22a5a3c4d8ae20a70d101274 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 22 Sep 2026 02:33:35 +0000 Subject: [PATCH 2/2] Adversarial review: the upload survived the errors I had not thought of Each branch deleted the uploaded file for itself, which covered every case I had in mind and none of the ones I had not. **A file Chainlit has not finished writing raised `FileNotFoundError` straight out of `validate`**, uncaught, past the handler -- the user would have seen a raw error and the flow would have reported nothing. Reproduced before fixing: an attachment pointing at a path that does not exist. **An `AskUserMessage` that times out or is interrupted left the matrix on disk.** Ten minutes is a long window, and an unhandled error is exactly when nobody is around to tidy up, on a host with 5 GB free. So the deletion is now a `finally` around the whole flow rather than a line in each branch. `discard` is safe to call twice, so the paths that already delete it can keep doing so; what changed is that the paths nobody enumerated are covered too. An unreadable file is also reported as something to retry rather than as the user's mistake, because "that is not an expression matrix" is wrong and unactionable when the real problem is that the upload did not land. Checked while there: a binary file with a `.tsv` name is refused cleanly rather than crashing -- somebody will attach a spreadsheet. Sabotage: replacing the `finally` fails four tests, including the two that passed before this commit, which is the point -- they were passing because of their own branch's cleanup, not because the file was guaranteed gone. 30 tests in the flow, ./checks.sh clean. Co-Authored-By: Claude Opus 5 --- src/gsa/chainlit_flow.py | 33 +++++++++++++++-- tests/gsa/test_gsa_chainlit_flow.py | 55 +++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/src/gsa/chainlit_flow.py b/src/gsa/chainlit_flow.py index d1cc6df2..705f0adc 100644 --- a/src/gsa/chainlit_flow.py +++ b/src/gsa/chainlit_flow.py @@ -92,13 +92,43 @@ async def run_analysis( over a file. """ path = Path(attachment.path) + try: + await _run( + attachment, path, ask_for_grouping, send, update_progress, send_file, client + ) + finally: + # The upload does not survive this function, whatever happened. + # + # Each path used to delete it for itself, which covered the ones I + # thought of. It did not cover a file Chainlit had not finished + # writing (`FileNotFoundError` straight out of `validate`), or an + # `AskUserMessage` that times out or is interrupted -- and an + # unhandled error is exactly when nobody is around to tidy up. + # `discard` is safe to call twice, so the paths that already delete + # it can keep doing so. + discard(path) + +async def _run( + attachment: Attachment, + path: Path, + ask_for_grouping: Any, + send: Any, + update_progress: Any, + send_file: Any, + client: GsaClient | None, +) -> None: try: matrix = validate(path) except UploadRejectedError as refusal: - discard(path) await send(str(refusal)) return + except OSError: + # Unreadable, vanished, or not a file. The user did nothing wrong + # that they can act on, so do not describe it as their mistake. + logger.exception("could not read an uploaded file") + await send("I could not read that file — please try attaching it again.") + return await send(chat.describe_matrix(matrix)) @@ -113,7 +143,6 @@ async def run_analysis( try: grouping = chat.parse_grouping(reply, len(matrix.samples)) except chat.ReplyUnusableError as unusable: - discard(path) await send(f"{unusable} Send the file again when you are ready.") return diff --git a/tests/gsa/test_gsa_chainlit_flow.py b/tests/gsa/test_gsa_chainlit_flow.py index 2124debc..ba498b98 100644 --- a/tests/gsa/test_gsa_chainlit_flow.py +++ b/tests/gsa/test_gsa_chainlit_flow.py @@ -14,6 +14,8 @@ from pathlib import Path from typing import Any +import pytest + from gsa import chainlit_flow from gsa.client import AnalysisStatus from gsa.job import AnalysisFailedError @@ -213,3 +215,56 @@ async def test_the_uploaded_matrix_is_gone_afterwards(self, tmp_path: Path) -> N attachment = a_matrix(tmp_path) await run(attachment, Chat(), StubClient(), tmp_path / "out") assert not Path(attachment.path).exists() + + +class TestTheUploadNeverSurvives: + """The file is gone when this function returns, whatever happened. + + Each branch used to delete it for itself, which covered the cases I had + thought of and not the ones I had not: a file Chainlit has not finished + writing, or an `AskUserMessage` that times out. An unhandled error is + precisely when nobody is around to tidy up, and this host has 5 GB free. + """ + + @asyncio_test + async def test_a_file_that_is_not_there_is_reported_not_raised( + self, tmp_path: Path + ) -> None: + missing = Attached(tmp_path / "never-written.tsv") + chat_ = Chat() + + await run(missing, chat_, StubClient(), tmp_path / "out") + + assert "could not read that file" in chat_.transcript + # And it does not describe it as the user's mistake, because it is + # not one they can act on. + assert "expression matrix" not in chat_.transcript + + @asyncio_test + async def test_an_exception_from_the_question_still_deletes_it( + self, tmp_path: Path + ) -> None: + attachment = a_matrix(tmp_path) + + class Exploding(Chat): + async def ask(self) -> str | None: + raise TimeoutError("the user never answered") + + with pytest.raises(TimeoutError): + await run(attachment, Exploding(), StubClient(), tmp_path / "out") + + assert not Path(attachment.path).exists() + + @asyncio_test + async def test_a_binary_file_is_refused_rather_than_crashing( + self, tmp_path: Path + ) -> None: + # Somebody will attach a spreadsheet or an image with a .tsv name. + path = tmp_path / "image.tsv" + path.write_bytes(bytes(range(256)) * 50) + chat_ = Chat() + + await run(Attached(path), chat_, StubClient(), tmp_path / "out") + + assert chat_.said, "it must say something rather than fail silently" + assert not path.exists()