Skip to content

feat(web): let the user keep an analysis instead of re-running it - #2

Open
42piratas wants to merge 16 commits into
alexwbend:mainfrom
42piratas:feat/save-last-analysis
Open

feat(web): let the user keep an analysis instead of re-running it#2
42piratas wants to merge 16 commits into
alexwbend:mainfrom
42piratas:feat/save-last-analysis

Conversation

@42piratas

@42piratas 42piratas commented Aug 28, 2026

Copy link
Copy Markdown

Read #1 first — the diff below includes it.

This branch is built on top of #1, and the save UI hooks straight into the
displayResults() and category-tab code that #1 rewrites. Both touch routes.py,
index.html, app.py and tests/test_web.py, so it cannot be rebased onto main
as it stands.

GitHub will not let me base a PR on #1, because a base has to be a branch in this
repo and #1's branch lives on my fork. So this PR targets main and shows all 16
commits — 15 of them are #1, already under review there. The one commit that is
new here is the last one, feat(web): let the user keep an analysis instead of re-running it.
Once #1 is merged this will collapse to that single commit on its
own; no action needed from you.

The problem

A whole-genome run is about half an hour, and the results only ever live in the tab
that ran it. Reload the page, close the laptop, or open a second tab, and the only
way back is to upload the file and wait again.

What this does

A Save results on this computer button next to Export writes the analysis to
~/.allelio/last_analysis.json. On the next visit a banner offers to open it, or to
delete it.

It is opt-in — nothing is written unless the button is pressed — and there is no code
path that sends it anywhere. The file is the user's genotypes, so:

  • written with mkstemp + os.replace, so it is mode 0600 rather than whatever the
    umask says, and a crash mid-write leaves the previous save intact instead of half a file
  • fsync before the rename, so a power cut cannot land the rename and lose the contents
  • a truncated or non-object file reads as "nothing saved" rather than wedging the page
    on a 500 forever
  • both the encode and the decode run off the event loop — a 15 MB json.dump on it
    stalls every other request for the duration
  • DELETE answers 200 only when the file is really gone. Reporting success over a
    genome still sitting on the disk is the one lie this feature cannot afford

Why there is a Host check in here too

Saving the analysis means it can be fetched back, and that needs a boundary this app
did not have. Binding to 127.0.0.1 is not one on its own: a page on a domain whose
DNS re-resolves to 127.0.0.1 is same-origin by the browser's reckoning, and CORS
never enters into it. Without a check, any page the user happened to be visiting could
read the saved genome out of GET /api/saved/data.

So the app now rejects Host headers it does not recognise — loopback always, plus
whatever --host was given, plus anything ALLELIO_ALLOWED_HOSTS names.

Two details that shape the code:

  • Starlette compares the Host header with the port already stripped, splitting on
    ":" to do it. So no allow-list entry carries a port, and no IPv6 literal can ever
    match one — serve leaves those off the list and prints localhost rather than a
    URL that answers 400.
  • A bind address is not a name a browser sends. 0.0.0.0 — and 0, 0.0, 0x0 and
    an empty --host, which all bind the same way — get the same treatment, with a
    warning that reaching the server from another machine means naming that machine.

A malformed allow-list pattern raises at import rather than on the first request, long
after the URL has been printed.

Tests

tests/test_web.py goes from 107 to 133. The new ones cover saving being opt-in, the
round trip, the 0600 mode, a directory that already exists being left alone, a
truncated save, JSON that parses but is not an analysis, a delete that could not remove
the file, a failed save keeping the previous one, a rebound domain being refused, and
which hosts do and do not end up on the allow-list.

Three front-end fixes have no automated coverage, because there is no JS test
infrastructure in the repo — the stale category filter on a fresh set of results, the
banner's behaviour when the saved file cannot be read, and the confirm on delete. Those
were checked end to end instead, against a real 16 MB genotype file: a full run of
62,057 findings, saved, reloaded, restored in 4.1s with every card, the summary and all
50 explanations matching the live run, the category tabs still filtering afterwards, and
the delete both refused on cancel and honoured on confirm.

42piratas and others added 16 commits August 27, 2026 19:20
The web interface could not start, and would not have served a request if
it had. Six defects, all on the `allelio serve` path — the CLI is unaffected.

- `starlette.concurrency.run_in_executor` does not exist and never has.
  The import aborted `allelio serve` before uvicorn bound a port. The name
  is unused in the module; the code calls `loop.run_in_executor`.
- `TemplateResponse(name, {"request": request})` is the removed positional
  form. `/` returned 500.
- `AIEngine.check_connection` is async and was called without `await` in two
  places. `/api/status` then tried to serialize a coroutine and returned 500;
  the upload path treated a truthy coroutine as a live connection.
- `AllelioDB.get_statistics` is not a method — it is `get_stats`. The
  AttributeError was swallowed, so the UI reported 0 ClinVar and 0 GWAS
  entries against a fully populated database.
- `get_variant_warnings` takes one argument and was passed two, failing
  every upload after the analysis had already run.
- The sqlite connection was opened on the event loop thread and then used
  from `run_in_executor`, which sqlite refuses by default.

The template also read `database_ready` and `ollama_connected`, while
/api/status returns `db_ready` and `ollama_available`, so the status panel
read "Database Not Set Up" whatever the real state was.

Verified against a 630,774-variant 23andMe export: `allelio serve` starts,
`/` renders, `/api/status` reports 2,645,423 ClinVar and 1,191,572 GWAS
entries, and `POST /api/analyze` returns 62,057 results.
Uploading a genome through the web UI looked like it hung. The progress
bar animated to 90% in fifteen seconds and then stopped, while the run
itself takes about ten minutes on a whole genome. Nothing told the user
where it had got to.

- Add /api/progress and have the page poll it, so the bar reports the
  actual stage and count ("Writing explanations - 27 of 50").
- Run the explanations through explain_variants_batch, which was already
  in the codebase but unused by the web route. 718s -> 569s.
- Call explain_variant, the method that exists; the route called
  generate_explanation and every explanation came back as a failure
  string.
- generate_summary treated ClinVar/GWAS entries as dicts, and sent the
  model counts without the variants, so it replied that the list "was not
  included in your message". Pass the findings.
- Cap the results list at 200 rows. Drawing all 62,057 locks the browser.
- Keep the last run in ~/.allelio/last_analysis.json and restore it on
  load, with a button back to the upload panel.
…ests

allow_origins=["*"] with allow_credentials meant any page the user
happened to have open could POST their genotype file to localhost and
read the analysis back. The UI is served from this same app; only a dev
server on another loopback port needs CORS at all.

Adds tests/test_web.py: the routes answer, a foreign origin gets no
access-control-allow-origin header, and the two AI entry points are
exercised against a stubbed client — including the assertion that the
summary prompt actually carries the variants.
… AI timeout

The results list reads result.gene, result.significance and result.pubmed_id.
The route sent clinvar_data and gwas_data, which are not fields on
VariantResult, so every card in a 62,000-variant report rendered as
"Gene: Unknown" with a BENIGN badge — including the pathogenic ones.

Sixty seconds also turned out to be short. On llama3.1:8b, the model this
project defaults to, 28 of 50 explanations came back as "Request timed out".
At 300s none do.

Verified end to end in a browser against Ollama: first card now reads
rs80224560 / CFTR / PATHOGENIC, 200 of 200 cards name a gene, 50 of 50
explanations complete.
The tabs filtered on slugs (health_conditions), the analyser labels
results "Health Conditions". Four of the five tabs showed "No results
found for this category" on every report.
Dropping the 200-row slice added while chasing the browser freeze. It does
not reproduce: all 62,057 findings render in under four seconds. Paging a
list this size is worth doing, but it is not part of this fix.
Follow-up on review of this branch.

Security:
- Result cards built the DOM by string interpolation from the uploaded file
  and the model. Nothing validates a genotype except a "--" check, so a
  crafted file executed script in the page. Escape every field, and only
  accept digits for the ClinVar and PubMed IDs that go into a URL.
- The exported HTML report had the same hole, server side.
- Removed the CORS middleware outright. The UI is same-origin with the API,
  so no origin needs to be granted anything; a loopback allowlist still let
  any local process read a genome off the API.

Correctness:
- "Conflicting interpretations of pathogenicity" was badged PATHOGENIC and
  pushed into the summary prompt as high impact, on a substring match.
- explain_variants_batch reported progress twice per variant, once with a
  stale count, so the bar counted backwards.
- The summary prompt read .gene on a GWAS entry, which names it mapped_gene.
- The top-50 sort keyed on significance_score, an attribute no result has;
  analyze_variants already orders them.
- The category tabs omitted carrier_status, which the analyser emits.
- pyproject allowed fastapi 0.104, whose starlette predates the
  TemplateResponse(request, name) signature this branch now uses.

Scope:
- Dropped the last-analysis file and its endpoint. Writing a whole genome's
  findings to disk and replaying them for whoever next opens the page is a
  feature with a privacy cost, not a crash fix; it belongs in its own PR.
Picking a tab looked the button up by its position in a second, hardcoded
category list. Anything missing from that list indexed to -1 and threw before
the results were re-rendered, so the tab appeared to do nothing. The tabs now
carry their own category.

That list was also missing two of the six categories the analyser emits:
carrier status and uncategorized. On a real genome that is 21% of the
findings, reachable only under All.
Second pass over the same ground, after review.

- The exported report escaped its rows but not its own two header fields,
  and /api/export takes whatever dict it is handed.
- The summary's pathogenic test disagreed with the badge on the card: it
  had no benign guard, so a combined value could be badged benign in the
  UI and sent to the model as high impact.
- _strong_gwas compared p-values by slicing the exponent out of the
  string form. p_value is a float column and 6,271 GWAS rows hold 0.0,
  which has no exponent at all, so the strongest associations in the
  catalogue were read as the weakest. Compare the number.
- The ClinVar source link keyed on a clinvar_id that is in no payload and
  on no entry, so it rendered for no variant. Point it at the rsID.
- _get_top_categories lost its only caller when the summary prompt
  changed; it was the last thing importing List and VariantResult here.
…inks

The summary and the per-variant explanations are the one thing the AI is here
for, and both arrived as a single run-on block: the model writes paragraphs,
HTML collapses the newlines. pre-wrap on the two elements that hold prose.

Also rel="noopener noreferrer" on the ClinVar and PubMed links.
…ean up the export

ClinVar's "Conflicting classifications of pathogenicity" is 130,833 of the
rsIDs in the shipped dump and sorts to the top of every report. It was drawn
in the same green as a benign call. It now has its own badge, and so does a
trait, which was also borrowing benign's green.

A ClinVar benign call was unreachable for any variant that also had a GWAS
row, because the GWAS check ran first.

The safety layer computes a genetic-counselling warning for BRCA1/2, TP53,
Lynch and APOE. Nothing rendered it. The cards and the exported report do now.

Explanations were gated on the model being reachable, so a machine without
Ollama got empty cards instead of the ClinVar/GWAS fallback that
explain_variant already writes.

The exported report was left in the shared temp directory at 0644 forever
with the user's genotypes in it. It is now 0600 and deleted once sent.
…ging

The multipart filename went straight into a temp path. "../../.zshenv"
resolves out of the temp directory, the route writes the body there and then
unlinks it, and multipart/form-data is CORS-safelisted, so a page on the open
web could do it without a preflight. mkstemp now picks the name — and the
mode, which matters more than it does for the report: this file is the user's
entire genome and it was landing at 0644 in a shared directory.

A GWAS row on its own was badged RISK. That is 37,108 of the 62,057 findings
on a real genome, every one of them sitting under a tab the analyser labelled
Traits.

"Uncertain significance" is 1,236,063 ClinVar rows, the largest class, and it
had no branch at all — so a variant nobody can interpret was drawn as a
harmless trait. It has its own badge now. A protective call reads as benign,
which is the honest colour for it.

explain_variant already folds the counselling warning into the explanation,
so the new warning box repeated it verbatim on exactly the top-50 variants a
user actually reads. It now only fills the gap it was meant for.

Fifty explanations, three at a time, each allowed 300s, can hold the upload
open for over an hour. The batch has a 15-minute ceiling and keeps whatever
finished.
…rning

Last commit made "has an explanation" stand in for "the counselling warning
is already inside that explanation". It is not: explain_variant only runs
wrap_with_disclaimer on the path where the model answered. Every fallback —
no Ollama, a timeout, any error — writes an explanation with no warning in
it, so on the machines the README says are supported, the BRCA1 and Lynch
warnings vanished from the card and the report entirely. Test the text.

Last commit also traded over-badging for under-badging. Delegating a GWAS-only
variant to the analyser's category sounds right until you read the test it
does: a literal search for "risk" or "association" in the trait string, which
2,068 of the 553,549 GWAS rsIDs pass. Type 2 diabetes, coronary artery
disease and prostate cancer all came out as a blue TRAIT. Neither guess is
available, so the badge now says association, which is what a GWAS row is.

One variant failing outside explain_variant's own guard used to cost one
explanation; since the per-variant try/except moved out of the route it cost
the whole upload, minutes after the analysis had finished.

The progress bar kept the explanation phase's counts through the last stage,
so a batch cut short by the new deadline left it reading "12 of 50" and
frozen at 29% for the longest step remaining.
…ing where it shows

Fifteen minutes was too tight a ceiling — a full run of this project's own
default model takes longer than that on an M1 Max, so the cap would have cut
a normal run short. Thirty.

Worse, a variant the deadline cut off got an empty string, and an empty
string renders as "No explanation available". _fallback_explanation writes
the gene, the ClinVar call and the GWAS traits, and it only runs inside the
coroutine that was cancelled — so a user with a slow but working Ollama ended
up with strictly less than a user with none. The batch starts from the
fallbacks and lets the finished ones overwrite.

The counselling warning went back on the card in the last commit, into
.result-body, which is display:none until the card is clicked. A BRCA1 card
looked like every other card. It sits under the header now.

The progress bar only moved during the explanation phase, so it read 10%
through everything else — including a summary step that can now run for half
an hour. The stages without a count carry their own width.

The report's cells had no pre-wrap, so a newline-formatted fallback
explanation collapsed into one run-on line in the artefact the user keeps.
A whole-genome run is half an hour, and the results only ever lived in the
tab that ran it. Reload the page, close the laptop, or open a second tab and
the only way back is to upload the file and wait again.

Saving is opt-in and stays on this machine. A "Save results on this computer"
button next to Export writes the analysis to ~/.allelio/last_analysis.json;
on the next visit a banner offers to open it, or to delete it. Nothing is
written unless the button is pressed, and no path sends it anywhere.

The file is the user's genotypes, so it is created with mkstemp and renamed
into place: mode 0600 rather than whatever the umask says, and a crash
mid-write leaves the previous save intact instead of half a file. The read
treats a truncated file as "nothing saved" — 15 MB of JSON makes a bad
shutdown a real case, and it should not wedge the page on a 500 forever.
Both the encode and the decode run off the event loop; a 15 MB json.dump on
it stalls every other request for the duration. Deleting something already
gone answers 200: the user asked for it not to be there, and it is not there.

Saving it also means it can be fetched back, and that needs a boundary this
app did not have. Binding to 127.0.0.1 is not one on its own: a page on a
domain whose DNS re-resolves to 127.0.0.1 is same-origin by the browser's
reckoning, and CORS never enters into it. Without a check, any page the user
happened to be visiting could read the saved genome out of GET
/api/saved/data. So the app now rejects Host headers it does not recognise —
loopback always, plus whatever --host was given and whatever
ALLELIO_ALLOWED_HOSTS names.

Starlette compares the Host header with the port already stripped, splitting
on ":" to do it, so no entry carries a port and no IPv6 literal can ever
match one; `serve` leaves those off the list and says to browse to localhost
rather than printing a URL that answers 400. A bind address is not a name a
browser sends either, so 0.0.0.0 — and 0, 0.0, 0x0 and an empty --host, all
of which bind the same way — get the same treatment, with a warning that
reaching the server from another machine means naming that machine. A
malformed pattern raises at import rather than on the first request, long
after the URL has been printed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@42piratas
42piratas marked this pull request as draft August 28, 2026 14:33
@42piratas
42piratas marked this pull request as ready for review August 28, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant