Run a gene set analysis from a file in the chat - #292
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Not a tool the agent calls. The run takes minutes, 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_modelis computed and sent nowhere.It is computed rather than skipped so the disclosure rules stay exercised by the tests. When a summary is added it must be handed the bounded allow-listed view, behind the existing warning — not the result.
Where the decisions live
gsa/chat.pyandgsa/chainlit_flow.pyhold them as functions over plain values: what counts as a usable reply, what the person is told, what happens when the service accepts a job and then dies.run_analysistakes its four chat operations as arguments — ask, say, revise, hand over a file — so it can be driven without a browser. The Chainlit handler is the only code that knows those are Chainlit, and what is left for a browser is whether the wiring matches.Three things worth calling out
The label count is strict; the separators are not. Commas, semicolons and tabs all work, and case is folded for grouping while the user's own spelling is kept — the labels come back in the service's output and reading it against their own 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.tomlgoes from the template's 500 MB to 20 MB, andupload.pyenforces 20 MB again. The first stops a browser sending it; only the second stops the server accepting it. The template also shipsaccept = ["*/*"], which invites every other kind of file into a chat that can do nothing with them — now TSV/CSV only, one file.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, andprune_resultsalready bounds how long they stay.Adversarial review
The progress line said "Done." on every path, including failure — set in a
finally. A user whose analysis died was told it had finished, one line above the message explaining that it had not. It is removed now, andrun_analysisreports the outcome itself on every path.for_modelbeing unused read as an oversight rather than a decision. It now says which it is, so the next person does not "fix" it by wiring the result into a prompt.test_a_rejected_file_is_explained_and_deletedtest_it_delivers_a_summary_and_a_filetest_an_analysis_that_fails_after_starting_is_reported27 new tests,
./checks.shclean.What this does not do
The public-dataset path (
submit_public_dataset) is built and tested but has no chat route: reaching it needs the model to pick a dataset and a factor, which is conversation design rather than plumbing. That inverts the spec's priorities and the spec now records why — the upload path turned out to be the simpler one, because a file arrives with its own sample names and nothing needs disambiguating.Wants a click-through on beta before it is called done.
🤖 Generated with Claude Code
Adversarial review of the finished branch
The upload survived the errors I had not thought of. Each branch deleted the file for itself, which covered every case I had in mind and none of the others.
FileNotFoundErrorstraight out ofvalidate, uncaught, past the handler. The user would have seen a raw error and the flow would have reported nothing. Reproduced before fixing, with an attachment pointing at a path that does not exist.AskUserMessagethat 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.The deletion is now a
finallyaround the whole flow rather than a line in each branch.discardis safe to call twice, so the branches that already delete it still do; what changed is that the paths nobody enumerated are covered.An unreadable file is also reported as something to retry rather than as the user's mistake — "that is not an expression matrix" is both wrong and unactionable when the real problem is that the upload did not land.
Also checked: a binary file with a
.tsvname is refused cleanly rather than crashing. Somebody will attach a spreadsheet.Verification
Replacing the
finallyfails four tests — including the two that passed before this commit. That 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.shclean.