diff --git a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md index dc162bb..d8f01a9 100644 --- a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md +++ b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md @@ -104,6 +104,13 @@ never an error code, so the analysis page cannot be broken by this service. pattern it could not write correctly. This endpoint's prompt is its own, so the right fix here is not to ask for one in the first place. +**Expression summaries refer to columns as `column 1`, `column 2`**, numbered +from one in the order the values appear, and in no other form. The labels are +user-supplied text this service never receives, so the caller is the only side +that can name them -- and can only substitute them into the prose if the +wording is fixed. Anything else ("the first column", "the leftmost sample") +reaches the reader as written. + **`disclosure` on `start` is the tier the summary was actually built from**, which is not always the one requested. If `identifiers` was asked for and the unmatched identifiers could not be retrieved, the summary is the aggregate one diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index d1c9432..b62ea20 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -127,12 +127,12 @@ carried. Everything in this phase can be built and tested before that lands. - [x] T031b Key the rate limiter on `human_sub` when present, falling back to the caller identity, in `src/api/analysis_summary.py` — per-person throttling rather than per-proxy-address. Also limit per analysis token: twenty summaries of one analysis is not a scientist - [x] T031c Test that a `human` claim with a stale `human_iat` is refused with **zero model calls**, in `tests/api/test_analysis_summary.py` — the freshness bound is the half most likely to be dropped, because the claim being present looks like success - [x] T032 [US1] Verify the assertion in `src/util/caller_token.py` -- `human_presence_reason` checks `human` and a 30-minute `human_iat`, refusing before any model call -- [ ] T033 [P] Test that a request without the assertion is refused and makes **zero model calls**, counted on a patched graph rather than inferred from timing, in `tests/api/test_analysis_summary.py` (SC-004) +- [x] T033 [P] Test that a request without the assertion is refused and makes **zero model calls**, counted on a patched graph rather than inferred from timing, in `tests/api/test_analysis_summary.py` (SC-004) ## Phase 9: Polish -- [ ] T034 [P] Bound the summary in `src/api/analysis_summary.py` as the answer endpoint is, so a stuck upstream cannot hold a connection -- [ ] T035 [P] Log an abandoned summary stream in `src/api/analysis_summary.py`, as the answer endpoint does, so a caller that starts summaries it does not want is visible +- [x] T034 [P] Bound the summary in `src/api/analysis_summary.py` as the answer endpoint is, so a stuck upstream cannot hold a connection. **The bound existed since Phase 3 and was untested until now**; the test also pins that a truncated summary is never stored, which would otherwise be served forever +- [x] T035 [P] Log an abandoned summary stream in `src/api/analysis_summary.py`, as the answer endpoint does, so a caller that starts summaries it does not want is visible - [ ] T036 Run the [quickstart](./quickstart.md) scenarios against beta with a real analysis token and record the outcome, including first-token timing - [ ] T037 Tell the website session the endpoint exists, what it does not yet do, and the `gone` outcome they must handle — only once it is live on beta, not when it merges diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index 3654459..2628446 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -108,6 +108,18 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: # True when the hit rests on too few entities to be evidence, # whatever the p-value says. "fragile": _fragile(p), + # The per-column values, in column order. Carried explicitly + # because the allow-list keeping them is not the same as the + # prompt receiving them -- they were allow-listed and dropped + # here, so the expression reading asked the model to describe + # behaviour across columns using data it had never been + # given, and it invented both the trends and a fourth column + # of a three-column analysis. + **( + {"exp": p["entities"]["exp"]} + if isinstance(p.get("entities", {}).get("exp"), list) + else {} + ), } for p in pathways ], @@ -115,6 +127,14 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: # Stated rather than left for the model to notice, because "nothing went # wrong" is the case a model most readily embellishes into a caveat. out["all_identifiers_matched"] = unmatched == 0 + + # How many columns there are, stated rather than left to be counted off + # an array. A model asked to describe behaviour across columns will name + # one that does not exist -- measured: a three-column analysis was + # summarised as rising "from column 1 to column 4". + columns = {len(p["exp"]) for p in out["pathways"] if isinstance(p.get("exp"), list)} + if len(columns) == 1: + out["expression_columns"] = columns.pop() for optional in ("resourceSummary", "speciesSummary", "warnings", "expression"): if optional in payload: out[optional] = payload[optional] @@ -162,8 +182,16 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "highlighted pathways behave *across* those columns -- rising, " "falling, mixed -- rather than treating the result as a single " "enrichment. **The columns are unlabelled here and you must not " - "guess what they are**: say 'the first column' and so on, never a " - "condition, timepoint or sample name." + "guess what they are**: never a condition, timepoint or sample name. " + "Each pathway's `exp` holds its value per column, in order, and " + "`expression_columns` says how many there are. **Those are the only " + "columns that exist** -- never mention a column number beyond it, " + "and never describe a trend you cannot read off `exp`. " + "Refer to them in exactly this form -- `column 1`, `column 2`, " + "numbered from one -- and in no other form, because the interface " + "holds the real labels and substitutes them by matching that exact " + "wording. 'The first column' or 'the leftmost sample' will not be " + "matched and will reach the reader as written." ), "SPECIES_COMPARISON": ( "This is a species comparison. The findings are **inferred by " diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index 9d63439..e4a37f4 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -239,3 +239,61 @@ def test_no_type_instruction_claims_another_types_reading() -> None: assert "orthology" not in TYPE_INSTRUCTION["EXPRESSION"] assert "columns" not in TYPE_INSTRUCTION["SPECIES_COMPARISON"] assert "columns" not in TYPE_INSTRUCTION["OVERREPRESENTATION"] + + +def test_expression_columns_have_one_stable_reference_form() -> None: + # The website holds the real column labels and we never do, so it + # substitutes them into our prose -- which only works if our wording is + # fixed. They said they would rather show labels alongside than splice + # on brittle matching, so the wording is pinned instead: `column 1`, + # `column 2`, numbered from one. + from analysis.summarise import TYPE_INSTRUCTION + + expression = TYPE_INSTRUCTION["EXPRESSION"] + assert "`column 1`" in expression + assert "numbered from one" in expression + assert "in no other form" in expression + # And the forms that would break their matching are named as wrong. + assert "The first column" in expression + + +def test_expression_values_reach_the_prompt_not_just_the_allow_list() -> None: + # The bug this pins: `exp` was allow-listed in `disclosure.py` and then + # dropped by `prompt_input`, so the expression reading asked the model to + # describe behaviour across columns using data it had never been given. + # Measured against a real three-column analysis, it invented the trends + # and a fourth column. + # + # Both layers had tests and both passed. Neither tested the path between + # them, which is where the data was lost. + payload = _payload(1e-9, 1e-8) + for pathway, values in zip( + payload["pathways"], ([0.7, 0.25, 0.25], [1.1, -0.4, 2.0]), strict=True + ): + pathway["entities"]["exp"] = values + out = prompt_input(payload) + assert out["pathways"][0]["exp"] == [0.7, 0.25, 0.25] + assert out["pathways"][1]["exp"] == [1.1, -0.4, 2.0] + + +def test_the_column_count_is_stated_rather_than_counted_off_an_array() -> None: + # A model asked to describe behaviour across columns will name one that + # does not exist. Stating the count is the same fix as D9's: if a number + # can be derived wrongly, derive it here. + payload = _payload(1e-9) + payload["pathways"][0]["entities"]["exp"] = [0.7, 0.25, 0.25] + assert prompt_input(payload)["expression_columns"] == 3 + + +def test_no_column_count_is_claimed_when_the_pathways_disagree() -> None: + # Inventing a count from inconsistent data would be the same error one + # level up. Absent is correct; a guess is not. + payload = _payload(1e-9, 1e-8) + payload["pathways"][0]["entities"]["exp"] = [0.7, 0.25] + payload["pathways"][1]["entities"]["exp"] = [1.1, -0.4, 2.0] + assert "expression_columns" not in prompt_input(payload) + + +def test_a_result_with_no_expression_values_claims_no_columns() -> None: + assert "expression_columns" not in prompt_input(_payload(1e-9)) + assert "exp" not in prompt_input(_payload(1e-9))["pathways"][0] diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 2fbc516..da2514f 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -9,10 +9,12 @@ no model call happens without a person -- none of which depends on content. """ +import asyncio import json +import re import time -from collections.abc import AsyncIterator -from typing import Any +from collections.abc import AsyncGenerator, AsyncIterator +from typing import Any, cast import jwt import pytest @@ -23,12 +25,17 @@ from analysis.client import Fetched from analysis.store import SummaryStore -from api.analysis_summary import router +from api.analysis_summary import SummaryRequest, analysis_summary, router from util.caller_token import DEFAULT_AUDIENCE from util.rate_limit import SlidingWindowLimiter PREFIX = "/chat/guest/api" +# Shaped like a real analysis token. S106 flags any string passed to an +# argument named `token`; an analysis token addresses a user's result but is +# not a credential. +SAMPLE_TOKEN = "MjAyNjA5MTkxODExNDJfMTE" # noqa: S105 + RESULT: dict[str, Any] = { "summary": {"type": "OVERREPRESENTATION", "fileName": "smith_unpublished.txt"}, "identifiersNotFound": 2, @@ -607,3 +614,136 @@ async def _gone(_token: str) -> Fetched: _events(_post(public, caller_token=_token(private)).text)[-1][1]["state"] == "gone" ) + + +class _HangingModel(_Counter): + """Stands in for a model that has stopped answering.""" + + async def astream(self, _messages: Any) -> AsyncIterator[Any]: + self.calls += 1 + yield type("Chunk", (), {"content": "starting"})() + # Finite, so a regression fails in five seconds rather than hanging + # the suite for the full timeout. + await asyncio.sleep(5) + yield type("Chunk", (), {"content": "never arrives"})() + + +def test_a_stuck_model_still_ends_the_stream( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # T034. FR-008 says a failure must be a terminal state the caller can + # render, never a broken panel -- which means never an open connection + # either. An analysis page must not hang because this service did. + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: _HangingModel()) + monkeypatch.setattr("api.analysis_summary.SUMMARY_TIMEOUT_SECONDS", 0.25) + private, public = keys + + started = time.monotonic() + response = _post(public, caller_token=_token(private)) + elapsed = time.monotonic() - started + + assert response.status_code == 200 + assert _events(response.text)[-1][1]["state"] == "failed" + assert elapsed < 2, f"stream ran {elapsed:.1f}s; the bound did not fire" + + +def test_a_stuck_summary_is_not_stored( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # The partial text of a timed-out generation must not become the summary + # served forever after. `put` refuses empty text, but this one is not + # empty -- it is worse, being a plausible fragment that ends mid-sentence. + store = SummaryStore() + monkeypatch.setattr("api.analysis_summary._store", store) + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: _HangingModel()) + monkeypatch.setattr("api.analysis_summary.SUMMARY_TIMEOUT_SECONDS", 0.25) + private, public = keys + _post(public, caller_token=_token(private)) + assert len(store) == 0, "a truncated summary was stored" + + +def test_an_abandoned_summary_stream_is_recorded( + keys: tuple[str, str], + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + # T035. A caller that hangs up mid-summary leaves no other trace: the + # request 200s, tokens flow, and then nothing more happens -- identical + # to a healthy stream in every signal. The website already hit this on + # the answer route, where a keystroke unmounted their panel mid-answer. + # + # Driven through the response iterator rather than a client, because the + # point is to close it mid-stream and a TestClient will not. + from types import SimpleNamespace + + class _SlowModel(_Counter): + async def astream(self, _messages: Any) -> AsyncIterator[Any]: + self.calls += 1 + for index in range(50): + await asyncio.sleep(0.01) + yield type("Chunk", (), {"content": f"t{index} "})() + + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: _SlowModel()) + private, public = keys + request = SimpleNamespace( + app=SimpleNamespace(state=SimpleNamespace(caller_token_key=public)) + ) + + async def drive() -> None: + response = await analysis_summary( + SummaryRequest( + token=SAMPLE_TOKEN, + caller_token=_token(private), + disclosure="aggregate", + ), + request, # type: ignore[arg-type] + ) + iterator = cast("AsyncGenerator[str, None]", response.body_iterator) + seen = 0 + async for _chunk in iterator: + seen += 1 + if seen == 4: + break + await iterator.aclose() + + with caplog.at_level("INFO", logger="api.analysis_summary"): + asyncio.run(drive()) + + messages = [record.getMessage() for record in caplog.records] + assert any( + "abandoned" in message for message in messages + ), f"no record of the abandoned stream; logged: {messages}" + + +def test_expression_values_reach_the_model_on_the_served_path( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # End to end, because the bug lived in the join between two layers that + # each had passing tests: the allow-list kept `exp` and `prompt_input` + # dropped it. Asserted on what the model receives, which is the only + # place the join is visible. + monkeypatch.setitem(RESULT["summary"], "type", "EXPRESSION") + monkeypatch.setitem(RESULT["pathways"][0]["entities"], "exp", [0.7, 0.25, 0.25]) + monkeypatch.setitem(RESULT["pathways"][1]["entities"], "exp", [1.1, -0.4, 2.0]) + sent: list[Any] = [] + + class _Recording(_Counter): + async def astream(self, messages: Any) -> AsyncIterator[Any]: + sent.append(messages) + async for chunk in super().astream(messages): + yield chunk + + recording = _Recording() + private, public = keys + with pytest.MonkeyPatch.context() as patch: + patch.setattr("api.analysis_summary.get_llm", lambda *a, **k: recording) + _post(public, caller_token=_token(private)) + + assert sent, "the model was never called, so this proves nothing" + prompt = json.dumps(sent, default=str) + assert "0.25" in prompt, "the per-column values never reached the model" + # The payload is JSON inside JSON, so the inner quotes are escaped. + # Matching on the unescaped form silently never matches -- which is how + # this assertion first passed review while testing nothing. + assert "expression_columns" in prompt + assert re.search(r'expression_columns\\?":\s*3', prompt), prompt[-200:]